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

Tables loading #10944

Merged
merged 6 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions kolibri/core/assets/src/views/CoreTable.vue
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
});

// If we have loaded the data, but have no empty message and no rows, we log an error.
if (!this.dataLoading && this.emptyMessage && !tableHasRows) {
if (!this.dataLoading && !this.emptyMessage && !tableHasRows) {
logging.error('CoreTable: No rows in table, but no empty message provided.');
}

Expand All @@ -93,7 +93,7 @@
* empty message if there are no rows in the table. If we have loaded data, have
* an emptyMessage and have no rows. If we have rows, then we show the table alone
*/
var dataStatusEl = this.dataLoading
const dataStatusEl = this.dataLoading
? createElement('p', [createElement(KCircularLoader)])
: !tableHasRows && createElement('p', this.emptyMessage); // Only show message if no rows

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* `useGroups` composable function mock.
*
* If default values are sufficient for tests,
* you only need call `jest.mock('<useGroups file path>')`
* at the top of a test file.
*
* If you need to override some default values from some tests,
* you can import a helper function `useGroupsMock` that accepts
* an object with values to be overriden and use it together
* with `mockImplementation` as follows:
*
* ```
* // eslint-disable-next-line import/named
* import useGroups, { useGroupsMock } from '<useGroups file path>';
*
* jest.mock('<useGroups file path>')
*
* it('test', () => {
* useGroups.mockImplementation(
* () => useGroupsMock({ groupsAreLoading: true })
* );
* })
* ```
*
* You can reset your mock implementation back to default values
* for other tests by calling the following in `beforeEach`:
*
* ```
* useGroups.mockImplementation(() => useGroupsMock())
* ```
*/

const MOCK_DEFAULTS = {
groupsAreLoading: false,
showGroupsPage: jest.fn(),
};

export function useGroupsMock(overrides = {}) {
return {
...MOCK_DEFAULTS,
...overrides,
};
}

export default jest.fn(() => useGroupsMock());
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* `useLessons` composable function mock.
*
* If default values are sufficient for tests,
* you only need call `jest.mock('<useLessons file path>')`
* at the top of a test file.
*
* If you need to override some default values from some tests,
* you can import a helper function `useLessonsMock` that accepts
* an object with values to be overriden and use it together
* with `mockImplementation` as follows:
*
* ```
* // eslint-disable-next-line import/named
* import useLessons, { useLessonsMock } from '<useLessons file path>';
*
* jest.mock('<useLessons file path>')
*
* it('test', () => {
* useLessons.mockImplementation(
* () => useLessonsMock({ lessonsAreLoading: true })
* );
* })
* ```
*
* You can reset your mock implementation back to default values
* for other tests by calling the following in `beforeEach`:
*
* ```
* useLessons.mockImplementation(() => useLessonsMock())
* ```
*/

const MOCK_DEFAULTS = {
lessonsAreLoading: false,
showLessonsRootPage: jest.fn(),
};

export function useLessonsMock(overrides = {}) {
return {
...MOCK_DEFAULTS,
...overrides,
};
}

export default jest.fn(() => useLessonsMock());
71 changes: 71 additions & 0 deletions kolibri/plugins/coach/assets/src/composables/useGroups.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { ref } from 'kolibri.lib.vueCompositionApi';
import samePageCheckGenerator from 'kolibri.utils.samePageCheckGenerator';
import { LearnerGroupResource, FacilityUserResource } from 'kolibri.resources';

// Place outside the function to keep the state
const groupsAreLoading = ref(false);

export function useGroups() {
function setGroupsLoading(loading) {
groupsAreLoading.value = loading;
}

function showGroupsPage(store, classId) {
// On this page, handle loading state locally
// TODO: Open follow-up so that we don't need to do this
store.dispatch('notLoading');

setGroupsLoading(true);

const promises = [
FacilityUserResource.fetchCollection({
getParams: { member_of: classId },
force: true,
}),
LearnerGroupResource.fetchCollection({
getParams: { parent: classId },
force: true,
}),
];
const shouldResolve = samePageCheckGenerator(store);
return Promise.all(promises).then(
([classUsers, groupsCollection]) => {
if (shouldResolve()) {
const groups = groupsCollection.map(group => ({ ...group, users: [] }));
const groupUsersPromises = groups.map(group =>
FacilityUserResource.fetchCollection({
getParams: { member_of: group.id },
force: true,
})
);

Promise.all(groupUsersPromises).then(
groupsUsersCollection => {
if (shouldResolve()) {
groupsUsersCollection.forEach((groupUsers, index) => {
groups[index].users = [...groupUsers];
});
store.commit('groups/SET_STATE', {
classUsers: [...classUsers],
groups,
groupModalShown: false,
});
setGroupsLoading(false);
store.dispatch('clearError');
}
},
error => (shouldResolve() ? store.dispatch('handleError', error) : null)
);
}
},
error => {
shouldResolve() ? store.dispatch('handleError', error) : null;
}
);
}

return {
groupsAreLoading,
showGroupsPage,
};
}
46 changes: 46 additions & 0 deletions kolibri/plugins/coach/assets/src/composables/useLessons.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { ref } from 'kolibri.lib.vueCompositionApi';
import { LearnerGroupResource } from 'kolibri.resources';
import { LessonsPageNames } from '../constants/lessonsConstants';

// Place outside the function to keep the state
const lessonsAreLoading = ref(false);

export function useLessons() {
function setLessonsLoading(loading) {
lessonsAreLoading.value = loading;
}

// Show the Lessons Root Page, where all the Lessons are listed for a given Classroom
function showLessonsRootPage(store, classId) {
// on this page, don't handle loading state globally so we can do it locally
store.dispatch('notLoading');

setLessonsLoading(true);
store.commit('lessonsRoot/SET_STATE', {
lessons: [],
learnerGroups: [],
});
const loadRequirements = [
// Fetch learner groups for the New Lesson Modal
LearnerGroupResource.fetchCollection({ getParams: { parent: classId } }),
store.dispatch('lessonsRoot/refreshClassLessons', classId),
];

return Promise.all(loadRequirements).then(
([learnerGroups]) => {
store.commit('lessonsRoot/SET_LEARNER_GROUPS', learnerGroups);
store.commit('SET_PAGE_NAME', LessonsPageNames.PLAN_LESSONS_ROOT);
setLessonsLoading(false);
},
error => {
store.dispatch('handleApiError', error);
setLessonsLoading(false);
}
);
}

return {
lessonsAreLoading,
showLessonsRootPage,
};
}
51 changes: 0 additions & 51 deletions kolibri/plugins/coach/assets/src/modules/groups/handlers.js

This file was deleted.

28 changes: 0 additions & 28 deletions kolibri/plugins/coach/assets/src/modules/lessonsRoot/handlers.js

This file was deleted.

5 changes: 4 additions & 1 deletion kolibri/plugins/coach/assets/src/routes/planLessonsRoutes.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ import {
showLessonResourceBookmarks,
showLessonResourceBookmarksMain,
} from '../modules/lessonResources/handlers';
import { showLessonsRootPage } from '../modules/lessonsRoot/handlers';
import { showLessonSummaryPage } from '../modules/lessonSummary/handlers';
import { LessonsPageNames } from '../constants/lessonsConstants';

import { useLessons } from '../composables/useLessons';

import LessonsRootPage from '../views/plan/LessonsRootPage';
import LessonSummaryPage from '../views/plan/LessonSummaryPage';
import LessonResourceSelectionPage from '../views/plan/LessonResourceSelectionPage';
Expand All @@ -31,6 +32,8 @@ function path(...args) {
return args.join('');
}

const { showLessonsRootPage } = useLessons();

export default [
{
name: LessonsPageNames.PLAN_LESSONS_ROOT,
Expand Down
4 changes: 3 additions & 1 deletion kolibri/plugins/coach/assets/src/routes/planRoutes.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import store from 'kolibri.coreVue.vuex.store';
import { PageNames } from '../constants';
import { useGroups } from '../composables/useGroups';
import GroupsPage from '../views/plan/GroupsPage';
import GroupMembersPage from '../views/plan/GroupMembersPage';
import GroupEnrollPage from '../views/plan/GroupEnrollPage';
import { showGroupsPage } from '../modules/groups/handlers';
import planLessonsRoutes from './planLessonsRoutes';
import planExamRoutes from './planExamRoutes';

const { showGroupsPage } = useGroups();

export default [
...planLessonsRoutes,
...planExamRoutes,
Expand Down
12 changes: 7 additions & 5 deletions kolibri/plugins/coach/assets/src/views/plan/GroupsPage/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
/>
</div>

<CoreTable>
<CoreTable
:dataLoading="groupsAreLoading"
Copy link
Member

Choose a reason for hiding this comment

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

It is a little strange that there are groups showing while the loader spins. Ideally, the table loading state would hide existing data (or at least "disable" it or otherwise indicate that it is currently part of the loading dataset)

Maybe a v-if="!groupsAreLoading" on the tbody here would do it?

Copy link
Member

Choose a reason for hiding this comment

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

In testing I'm not actually seeing the group while the loader is shown 🤷🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand, could you please elaborate a bit more?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe a v-if="!groupsAreLoading" on the tbody here would do it?

In any case, I think this won't harm anything so no problem to commit it

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps it'd make more sense to have this implemented rather in CoreTable, though? Not only to have a condition to show the loader when dataLoading is truthy but also to not show the table body in this case? I think that may prevent some glitches (maybe in regards to cached data?) by ensuring that table data is never displayed as long as dataLoading is truthy.

Copy link
Member

Choose a reason for hiding this comment

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

In your screencast, when you go to the Group tab, "Group 1" is visible with the loader underneath it. I think the loader being there should hide the content on the table until it is done loading.

However, I was not able to replicate it and tried as best I could so I don't think it's a blocking issue at all.

Perhaps it'd make more sense to have this implemented rather in CoreTable, though?

Yeah I think you're right there for sure. That might be a good follow-up issue as it'd require regression testing across a bunch of tables and this is good to go otherwise. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

In your screencast, when you go to the Group tab, "Group 1" is visible with the loader underneath it. I think the loader being there should hide the content on the table until it is done loading.

However, I was not able to replicate it and tried as best I could so I don't think it's a blocking issue at all.

Ah yes, I see now. Yes, that's right. I think it might be related to the way I prepared the recording when I added a piece of code to delay loading - it's possible that I placed it in the wrong place as I couldn't reproduce it either right now. Or it could be related to some kind of cache - we can resolve that in a follow-up and do regression testing, yes. I will open the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

:emptyMessage="$tr('noGroups')"
>
<template #headers>
<th>{{ coachString('nameLabel') }}</th>
<th>{{ coreString('learnersLabel') }}</th>
Expand All @@ -38,10 +41,6 @@
</template>
</CoreTable>

<p v-if="!sortedGroups.length">
{{ $tr('noGroups') }}
</p>

<CreateGroupModal
v-if="showCreateGroupModal"
:groups="sortedGroups"
Expand Down Expand Up @@ -79,6 +78,7 @@
import CoreTable from 'kolibri.coreVue.components.CoreTable';
import commonCoreStrings from 'kolibri.coreVue.mixins.commonCoreStrings';
import commonCoach from '../../common';
import { useGroups } from '../../../composables/useGroups';
import CoachAppBarPage from '../../CoachAppBarPage';
import PlanHeader from '../../plan/PlanHeader';
import { GroupModals } from '../../../constants';
Expand All @@ -101,6 +101,7 @@
},
mixins: [commonCoach, commonCoreStrings],
setup() {
const { groupsAreLoading } = useGroups();
const selectedGroup = ref({
name: '',
id: '',
Expand All @@ -113,6 +114,7 @@
setSelectedGroup(name, id) {
selectedGroup.value = { name, id };
},
groupsAreLoading,
};
},
computed: {
Expand Down
Loading