Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lesson resources selection #12895

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5e369ee
Refactor views lessons side panel view handling
AlexVelezLl Nov 27, 2024
176fd3c
Add basic implementation of UpdateResourceSelection
AlexVelezLl Nov 27, 2024
ed84a9a
Add selection and deselection of resources
AlexVelezLl Nov 27, 2024
27d94c3
Fix styles and details to select from bookmarks
AlexVelezLl Nov 27, 2024
2f36020
Fix fetch more resources in quizzes
AlexVelezLl Nov 28, 2024
83c69e8
Add fetch and fetch more management
AlexVelezLl Nov 28, 2024
b05abc4
Implement tree fetch and display
AlexVelezLl Nov 28, 2024
c4601a3
Add restrictions to select all
AlexVelezLl Nov 29, 2024
fb0ec8c
Show selected resources with its size
AlexVelezLl Nov 29, 2024
aef7e14
Remove unused code
AlexVelezLl Dec 3, 2024
148c5b9
use vue router to manage side panels sub pages
AlexVelezLl Dec 3, 2024
5a314b1
Move composables
AlexVelezLl Dec 3, 2024
2f41854
Replace provide/inject
AlexVelezLl Dec 4, 2024
51b3726
Add documentation and remove unused code
AlexVelezLl Dec 12, 2024
7da5057
Update vue composition api package
AlexVelezLl Dec 12, 2024
43adff4
WIP Commit
AlexVelezLl Dec 12, 2024
dd68dd1
Refactor side panel positioning
AlexVelezLl Dec 12, 2024
4d1d90c
Refactor useFetch to return hasMore, loadingMore and count
AlexVelezLl Dec 16, 2024
b34a28b
Remove moreKey, dataKey and countKey
AlexVelezLl Dec 17, 2024
10365f2
Update jsdoc docs
AlexVelezLl Dec 17, 2024
e8f90f3
Lint files
AlexVelezLl Dec 17, 2024
b7fa010
Update more to be called moreParams and update channels route name
AlexVelezLl Dec 17, 2024
03301ca
Manage fetch concurrency
AlexVelezLl Dec 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions kolibri/plugins/coach/assets/src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ class CoachToolsModule extends KolibriApp {
PageNames.LESSON_EDIT_DETAILS_BETTER,
PageNames.LESSON_PREVIEW_SELECTED_RESOURCES,
PageNames.LESSON_PREVIEW_RESOURCE,
PageNames.LESSON_SELECT_RESOURCES_INDEX,
PageNames.LESSON_SELECT_RESOURCES_BOOKMARKS,
PageNames.LESSON_SELECT_RESOURCES_TOPIC_TREE,
];
// If we're navigating to the same page for a quiz summary page, don't set loading
if (
Expand Down
150 changes: 150 additions & 0 deletions kolibri/plugins/coach/assets/src/composables/useFetch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
import { ref, computed } from 'vue';

/**
* A composable for managing fetch operations with optional methods for additional data fetching.
*
* @param {Object} options **Required** Configuration options for the fetch operation.
* @param {(...args) => Promise<any>} options.fetchMethod **Required** Function to fetch the
* initial data.
* * Should return a Promise resolving to the fetched data or a `{ results: any[], more: any }`
* object. The "results" property should be the fetched data and the "more" property should be
* the next "more" object to use in the fetchMore method.
*
* Example:
* ```js
* const { data, loading, error, fetchData } = useFetch({
* fetchMethod: () => ContentNodeResource.fetchBookmarks()
* })
* ```
*
*
* @param {(more, ...args) => Promise<any>} [options.fetchMoreMethod] Function to fetch more data.
* * This function receives a "moreParams" object as its first argument. This moreParams object is
* from the "more" property of the response from the previous fetch to fetch more data.
* * Should return a Promise resolving to { results: any[], more: any }. The "results" property
* should be the fetched data and the "more" property should be the next "moreParams" object to
* use in the next fetchMore method.
* * FetchMore just works if the fetched data is an array
*
* Example:
*
* ```js
* const { data, loading, error, fetchData } = useFetch({
* fetchMethod: () => ContentNodeResource.fetchBookmarks(),
* fetchMoreMethod: moreParams => ContentNodeResource.fetchBookmarks(moreParams)
* })
* ```
*
*
* @typedef {Object} FetchObject
* @property {any} data The main fetched data.
* @property {Object} error Error object if a fetch data failed.
* @property {any} count The count of the fetched data. E.g., the total number of items.
* @property {boolean} loading Data loading state. This loading doesnt reflect the loading when
* fetching more data. refer to `loadingMore` for that.
* @property {boolean} loadingMore Loading state when fetching more data. This is different from
* `loading` which is for the main data fetch.
* @property {boolean} hasMore A computed property to check if there is more data to fetch.
* @property {(...args) => Promise<void>} fetchData A method to manually trigger the main fetch.
* @property {(...args) => Promise<void>} fetchMore A method to manually trigger fetch more data.
*
* @returns {FetchObject} An object with properties and methods for managing the fetch process.
*/
export default function useFetch(options) {
const { fetchMethod, fetchMoreMethod } = options || {};

const loading = ref(false);
const data = ref(null);
const error = ref(null);
const moreParams = ref(null);
const count = ref(null);
const loadingMore = ref(false);

// useFetch metadata to manage synchronization of fetches
const _fetchCount = ref(0);

const hasMore = computed(() => moreParams.value != null);

const _setData = (response, loadingMore) => {
const responseData = fetchMoreMethod ? response.results : response;

/**
* For now, loading more just works if the data is an array.
*/
if (loadingMore && Array.isArray(data.value) && Array.isArray(responseData)) {
data.value = [...data.value, ...responseData];
} else if (!loadingMore) {
data.value = responseData;
}

moreParams.value = response.more || null;
count.value = response.count || null;
};

const fetchData = async (...args) => {
loading.value = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a guard here against loading twice, I think? If loading.value is already true, we shouldn't try to load again?

loadingMore.value = false; // Reset loading more state
error.value = null;
_fetchCount.value += 1;
const currentFetchCount = _fetchCount.value;

// If the fetch count has changed, it means that a new fetch has been triggered
// and this fetch is no longer relevant
const newFetchHasStarted = () => currentFetchCount !== _fetchCount.value;

try {
const response = await fetchMethod(...args);
if (newFetchHasStarted()) {
return;
}
_setData(response);
} catch (err) {
if (newFetchHasStarted()) {
return;
}
error.value = err;
}

loading.value = false;
};

const fetchMore = async (...args) => {
if (!moreParams.value || !fetchMoreMethod || loadingMore.value) {
return;
}

loadingMore.value = true;
error.value = null;
const currentFetchCount = _fetchCount.value;

// If the fetch count or fetch more count has changed, it means that a new fetch has been
// triggered and this fetch is no longer relevant
const newFetchHasStarted = () => currentFetchCount !== _fetchCount.value;

try {
const response = await fetchMoreMethod(moreParams.value, ...args);
if (newFetchHasStarted()) {
return;
}
_setData(response, true);
} catch (err) {
if (newFetchHasStarted()) {
return;
}
error.value = err;
}

loadingMore.value = false;
};

return {
data,
error,
count,
loading,
hasMore,
loadingMore,
fetchData,
fetchMore,
};
}
3 changes: 2 additions & 1 deletion kolibri/plugins/coach/assets/src/composables/useFetchTree.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,11 @@ export default function useFetchTree({ topicId, params = {} } = {}) {
// results is the list of all children from this call to the API
// more is an object that contains the parameters we need to fetch the next batch of nodes
const { results, more } = topicTree.children || { results: [], more: null };
const moreParams = more?.params || null;

set(_resources, [...get(_resources), ...results]);
set(_topic, topicTree);
set(_moreParams, more);
set(_moreParams, moreParams);
set(_loading, false);

return results;
Expand Down
141 changes: 141 additions & 0 deletions kolibri/plugins/coach/assets/src/composables/useResourceSelection.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import uniqBy from 'lodash/uniqBy';
import { ref, computed, getCurrentInstance, watch } from 'vue';
import ContentNodeResource from 'kolibri-common/apiResources/ContentNodeResource';
import ChannelResource from 'kolibri-common/apiResources/ChannelResource';
import useFetch from './useFetch';

/**
* @typedef {import('../../../../../../composables/useFetch').FetchObject} FetchObject
*/

/**
* Composable for managing the selection of resources within a topic tree.
* This utility handles selection rules, manages fetch states for channels, bookmarks,
* and topic trees, and offers methods to add, remove, or override selected resources.
*
* @typedef {Object} UseResourceSelectionResponse
* @property {Object} topic Topic tree object, contains the information of the topic,
* its ascendants and children.
* Defined only if the `topicId` query in the route is set.
* @property {boolean} loading Indicates whether the main topic tree, channels, and bookmarks
* data are currently loading. This does not account for loading more data. For such cases,
* use the fetch objects of each entity.
* @property {FetchObject} channelsFetch Channels fetch object to manage the process of
* fetching channels. We currently don't support fetching more channels.
* @property {FetchObject} bookmarksFetch Bookmarks fetch object to manage the process of
* fetching bookmarks. Fetching more bookmarks is supported.
* @property {FetchObject} treeFetch Topic tree fetch object to manage the process of
* fetching topic trees and their resources. Fetching more resources is supported.
marcellamaki marked this conversation as resolved.
Show resolved Hide resolved
* @property {Array<(node: Object) => boolean>} selectionRules An array of functions that determine
* whether a node can be selected.
* @property {Array<Object>} selectedResources An array of currently selected resources.
* @property {(resources: Array<Object>) => void} selectResources Adds the specified resources
* to the `selectedResources` array.
* @property {(resources: Array<Object>) => void} deselectResources Removes the specified resources
* from the `selectedResources` array.
* @property {(resources: Array<Object>) => void} setSelectedResources Replaces the current
* `selectedResources` array with the provided resources array.
*
* @returns {UseResourceSelectionResponse}
*/
export default function useResourceSelection() {
const store = getCurrentInstance().proxy.$store;
const route = computed(() => store.state.route);
const topicId = computed(() => route.value.query.topicId);

const selectionRules = ref([]);
const selectedResources = ref([]);
const topic = ref(null);

const bookmarksFetch = useFetch({
fetchMethod: () =>
ContentNodeResource.fetchBookmarks({
params: { limit: 25, available: true },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

limit: 25

I think I've answered my own question and that you're talking about pagination

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an API consumption perspective, is it confusing that the bookmarks fetching uses the limit parameter, whereas all the other contentnode APIs use max_results?

Under the hood, this is a result of the Bookmarks contentnode API using Limit/Offset pagination, whereas all the others use a more cursor style pagination, I am just not sure this is helpful to the consumer :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what's confusing is, at least to me, more is conceptually consistent but literally (in the code) not always consistent. So yes, some of it is about limit vs. max_results but also.... I don't have a great mental model of what can be in that object. When I see something like data.more or this.more, it's really hard for me to track if that's results (the response of say, the second page) or if that's the more params.

this isn't a criticism, @AlexVelezLl of your code -- I think you are in line with the existing usage. It's just something I have always sort of struggled with in terms of readability wherever we use this general pagination pattern. I just have to read and re-read things so many times to follow along.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the API perspective more is always either null or an object containing the exact GET parameters to retrieve the next page from the API - if we're using it to mean other things in the frontend, that's probably not helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcellamaki Do you think that renaming this more identifier to something like moreParams would help to make the intention of this object clearer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think that would help - not required, but maybe a nice to have

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super, just updated it to be called moreParams instead :)

}),
fetchMoreMethod: more =>
ContentNodeResource.fetchBookmarks({
params: more,
}),
});

const channelsFetch = useFetch({
fetchMethod: () =>
ChannelResource.fetchCollection({
getParams: {
available: true,
},
}),
});

const fetchTree = async (params = {}) => {
topic.value = await ContentNodeResource.fetchTree(params);
return topic.value.children;
};

const treeFetch = useFetch({
fetchMethod: () => fetchTree({ id: topicId.value, params: { include_coach_content: true } }),
fetchMoreMethod: more => fetchTree(more),
});

watch(topicId, () => {
if (topicId.value) {
treeFetch.fetchData();
}
});

const loading = computed(() => {
const sources = [bookmarksFetch, channelsFetch, treeFetch];

return sources.some(sourceFetch => sourceFetch.loading.value);
});

const fetchInitialData = async () => {
bookmarksFetch.fetchData();
channelsFetch.fetchData();
if (topicId.value) {
treeFetch.fetchData();
}
};

fetchInitialData();

const selectResources = (resources = []) => {
if (!resources || !resources.length) {
return;
}
if (resources.length === 1) {
const [newResource] = resources;
if (!selectedResources.value.find(res => res.id === newResource.id)) {
selectedResources.value = [...selectedResources.value, newResource];
}
} else {
selectedResources.value = uniqBy([...selectedResources.value, ...resources], 'id');
}
};

const deselectResources = (resources = []) => {
if (!resources || !resources.length) {
return;
}
selectedResources.value = selectedResources.value.filter(res => {
return !resources.find(unselectedResource => unselectedResource.id === res.id);
});
};

const setSelectedResources = (resources = []) => {
selectedResources.value = resources;
};

return {
topic,
loading,
channelsFetch,
bookmarksFetch,
treeFetch,
selectionRules,
selectedResources,
selectResources,
deselectResources,
setSelectedResources,
};
}
3 changes: 3 additions & 0 deletions kolibri/plugins/coach/assets/src/constants/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ export const PageNames = {
LESSON_EDIT_DETAILS: 'LESSON_EDIT_DETAILS',
LESSON_EDIT_DETAILS_BETTER: 'LESSON_EDIT_DETAILS_BETTER',
LESSON_SELECT_RESOURCES: 'LESSON_SELECT_RESOURCES',
LESSON_SELECT_RESOURCES_INDEX: 'LESSON_SELECT_RESOURCES_INDEX',
LESSON_SELECT_RESOURCES_BOOKMARKS: 'LESSON_SELECT_RESOURCES_BOOKMARKS',
LESSON_SELECT_RESOURCES_TOPIC_TREE: 'LESSON_SELECT_RESOURCES_TOPIC_TREE',
LESSON_PREVIEW_SELECTED_RESOURCES: 'LESSON_PREVIEW_SELECTED_RESOURCES',
LESSON_PREVIEW_RESOURCE: 'LESSON_PREVIEW_RESOURCE',
LESSON_LEARNER_REPORT: 'LESSON_LEARNER_REPORT',
Expand Down
23 changes: 22 additions & 1 deletion kolibri/plugins/coach/assets/src/routes/lessonsRoutes.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ import QuestionLearnersPage from '../views/common/reports/QuestionLearnersPage.v
import EditLessonDetails from '../views/lessons/LessonSummaryPage/sidePanels/EditLessonDetails';
import PreviewSelectedResources from '../views/lessons/LessonSummaryPage/sidePanels/PreviewSelectedResources';
import LessonResourceSelection from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection';
import SelectionIndex from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectionIndex.vue';
import SelectFromBookmarks from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromBookmarks.vue';
import SelectFromChannels from '../views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/subPages/SelectFromChannels.vue';
import { classIdParamRequiredGuard, RouteSegments } from './utils';

const {
Expand Down Expand Up @@ -132,8 +135,26 @@ export default [
},
{
name: PageNames.LESSON_SELECT_RESOURCES,
path: 'select-resources/:topicId?',
path: 'select-resources/',
component: LessonResourceSelection,
redirect: 'select-resources/index',
children: [
{
name: PageNames.LESSON_SELECT_RESOURCES_INDEX,
path: 'index',
component: SelectionIndex,
},
{
name: PageNames.LESSON_SELECT_RESOURCES_BOOKMARKS,
path: 'bookmarks',
component: SelectFromBookmarks,
},
{
name: PageNames.LESSON_SELECT_RESOURCES_TOPIC_TREE,
path: 'channels',
component: SelectFromChannels,
},
],
},
{
name: PageNames.LESSON_PREVIEW_SELECTED_RESOURCES,
Expand Down
2 changes: 0 additions & 2 deletions kolibri/plugins/coach/assets/src/views/CoachAppBarPage.vue
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
<slot></slot>
</div>
</AppBarPage>

<router-view />
</NotificationsRoot>

</template>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,17 @@ const coachStrings = createTranslator('CommonCoachStrings', {
message: 'Report lesson',
context: 'Labels the Reports > Lesson tab for screen reader users',
},
someResourcesSelected: {
message:
'{count} {count, plural, one {resource selected} other {resources selected}} ({bytesText})',
context:
"Indicates the amount of resources selected along with the file size. For example:\n\n'727 resources selected (22 GB)'",
},
manageLessonResourcesTitle: {
message: 'Manage lesson resources',
context:
"In the 'Manage lesson resources' coaches can add new/remove resource material to a lesson.",
},
});

// Strings for the Missing Content modals, tooltips, alerts, etc.
Expand Down
Loading
Loading