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.

Sign up for GitHub

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

Fix view learn device regression #13138

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
+35 −8
Open

Fix view learn device regression #13138

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8cf6a14
show report control link only when more than one learner present
ozer550 Feb 24, 2025
f0ee4a0
only show learners with learn only devices in View Learner Devices link
ozer550 Feb 27, 2025
fba6dac
run linter
ozer550 Feb 27, 2025
3b6977f
finding problem with polling
ozer550 Mar 3, 2025
f077d1c
fix initially polling problem
ozer550 Mar 3, 2025
484fd83
fix failing tests
ozer550 Mar 4, 2025
e247d00
Fetch LODS only when pageloads
ozer550 Mar 6, 2025
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
43 changes: 35 additions & 8 deletions kolibri/plugins/coach/assets/src/views/common/ReportsControls.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@

import pickBy from 'lodash/pickBy';
import useUser from 'kolibri/composables/useUser';
import { mapState, mapActions } from 'vuex';
import commonCoach from '../common';
import { ClassesPageNames } from '../../../../../learn/assets/src/constants';
import { LastPages } from '../../constants/lastPagesConstants';
Expand All @@ -59,7 +60,6 @@
mixins: [commonCoach],
setup() {
const { isAppContext } = useUser();

return {
isAppContext,
};
Expand All @@ -74,19 +74,33 @@
default: false,
},
},
data() {
return {
userSet: new Set(),
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 be a little careful with this, as VueJS < 3 does not support reactive updates of ES6 Set objects: vuejs/vue#2410

I think in this case, the fact that we're always replacing the entire object is probably sufficient for this to retrigger a reactive update in the computed prop, but please be cautious in general with using Set and Map. This is the reason that we often just us a plain JS object when a Set or a Map might be more performant/semantic.

};
},
computed: {
...mapState('classSummary', ['learnerMap']),
exportDisabled() {
// Always disable in app mode until we add the ability to download files.
return this.isAppContext || this.disableExport;
},
filteredLearnMap() {
return Object.fromEntries(
Object.entries(this.learnerMap || {}).filter(([key]) => this.userSet.has(key)),
);
},
isMainReport() {
return [
PageNames.LEARNERS_ROOT,
PageNames.LESSONS_ROOT,
PageNames.EXAMS_ROOT,
PageNames.EXAM_SUMMARY,
PageNames.LESSON_SUMMARY,
].includes(this.$route.name);
return (
[
PageNames.LEARNERS_ROOT,
PageNames.LESSONS_ROOT,
PageNames.LESSONS_ROOT_BETTER,
PageNames.EXAMS_ROOT,
PageNames.EXAM_SUMMARY,
PageNames.LESSON_SUMMARY,
].includes(this.$route.name) && Object.keys(this.filteredLearnMap).length > 0
);
},
classLearnersListRoute() {
const { query } = this.$route;
Expand All @@ -105,6 +119,19 @@
return route;
},
},
created() {
this.fetchClassListSyncStatus();
},
methods: {
...mapActions(['fetchUserSyncStatus']),
fetchClassListSyncStatus() {
this.fetchUserSyncStatus({ member_of: this.$route.params.classId }).then(data => {
if (Array.isArray(data)) {
this.userSet = new Set(data.map(item => item.user));
}
});
},
},
$trs: {
viewLearners: {
message: 'View learner devices',
Expand Down
Loading