UX for learner and coach/admin single user syncing #8202

marcellamaki · 2021-07-13T22:06:03Z

Summary

Fixes: #8187, closes #8178, resolves #8179

Front end feature for coach/admin view of sync statuses and learner dropdown. (Figma references)
Creates corresponding endpoints and adds to resource layer

What is covered in this PR:

Create new API endpoint for User Sync Status
The data returned from this endpoint should include the user, the last time the user synced (if they have synced before), if the user is actively syncing, or if the user is enqueued.
On the UX for learners, the sync status should be displayed for the learner in the dropdown menu (Figma: "Learner POV/changes to user dropdown")

On the UX for coaches, the coach should be able to open a page that contains a table of learners and see their statuses. If the coach has questions about the status options, they can view the informational modal. Coaches should also be able to access this table from the quiz report view
(Figma: "Coach POV")

Outstanding TO DOs:

Add in new icons after Misha's PR is merged
Finish new python tests and ensure all tests are passing

Follow up issues for: user name overflow, limited device messaging/"learn-only mode"

Reviewer guidance

To test these changes end to end, use kolibri shell to create some UserSyncStatus objects in the database. Alternatively, you can use some fake data values in just the front end for reviewing only the UX/front-end changes (which for a front-end only review, would probably be simpler). To do this, you can add objects for:

AppBar.vue (add user sync data for single learner to userSyncStatus)

   data() {
      return {
        userMenuDropdownIsOpen: false,
        userSyncStatus: null,
        isPolling: false,
      };
    },

ClassLearnersListPage.vue (add class list data, an array of user sync objects)

data: function() {
      return {
        displayTroubleshootModal: false,
        classSyncStatusList: [],
      };
    },

Each of these user objects should have the following keys "id", "queued", "sync_session", "last_synced", "active", "user", "user_id"

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

rtibbles

Can use filters to prevent reimplementing the same endpoint multiple times!

rtibbles · 2021-07-13T23:38:45Z

kolibri/core/device/api.py

@@ -164,3 +168,37 @@ def patch(self, request):
        settings.name = request.data["name"]
        settings.save()
        return Response({"name": settings.name})
+
+
+class UserSyncStatusViewSet(ReadOnlyValuesViewset):


These are confusingly named, my inference is that they are named the wrong way around, looking at the filtering going on in the get_queryset method.

rtibbles · 2021-07-14T00:20:46Z

kolibri/core/device/api.py

+
+
+class ClassListSyncStatusViewSet(ReadOnlyValuesViewset):
+    values = (


As the only difference between these two Viewsets is filtering that is done, I'd recommend consolidating these and using a FilterSet to return different sets of data.

class SyncStatusFilter(FilterSet): member_of = ModelChoiceFilter( method="filter_member_of", queryset=Collection.objects.all() ) def filter_member_of(self, queryset, name, value): return queryset.filter(Q(user__memberships__collection=value) | Q(user__facility=value)) class Meta: model = UserSyncStatus fields = ["user", "member_of"]

Can then add to the viewset as:

filter_backends = (KolibriAuthPermissionsFilter, DjangoFilterBackend) filter_class = SyncStatusFilter

We don't actually need KolibriAuthPermissionsFilter on there, but we should also add the permission based filtering to prevent unauthorized access.

rtibbles

Some clean up and questions.

rtibbles · 2021-07-19T16:14:37Z

kolibri/core/assets/src/views/AppBar.vue

@@ -82,6 +82,15 @@
                  :userType="getUserKind"
                />
              </div>
+              <div v-if="getUserKind === 'learner'">


At some point (but not necessarily now) we should refactor the name of this getter to userKind - calling it getUserKind is more in line with our practice of getters that are actually functions (I had to double check that this was not expecting the id of a user as an argument).

rtibbles · 2021-07-19T16:15:50Z

kolibri/core/assets/src/views/AppBar.vue

@@ -208,11 +240,31 @@
        this.$emit('showLanguageModal');
        this.userMenuDropdownIsOpen = false;
      },
+      mapSyncStatusOptionToLearner() {


Any reason for this not to be a computed prop, rather than an invoked method?

rtibbles · 2021-07-19T16:17:31Z

kolibri/core/assets/src/views/AppBar.vue

@@ -208,11 +240,31 @@
        this.$emit('showLanguageModal');
        this.userMenuDropdownIsOpen = false;
      },
+      mapSyncStatusOptionToLearner() {
+        if (this.userSyncStatus) {


Empty arrays are not falsey in Javascript, so I don't know when this would not be true - unless the API endpoint can return null?

rtibbles · 2021-07-19T16:18:14Z

kolibri/core/assets/src/views/AppBar.vue

@@ -153,13 +164,16 @@
    data() {
      return {
        userMenuDropdownIsOpen: false,
+        userSyncStatus: [],


Given that this is expected to be a plain object, using an empty array seems like an error here. Maybe null as you are doing a falsey check elsewhere?

rtibbles · 2021-07-19T16:20:38Z

kolibri/core/assets/src/views/CoreMenu/index.vue

@@ -127,7 +127,7 @@

  @import '~kolibri-design-system/lib/styles/definitions';
  .ui-menu-header {
-    padding: 1rem 1rem 1rem 50px;
+    padding: 1rem 1rem 1rem 1.2rem;


I feel like there might be a reason for this fixed pixel padding here, or does 1.2rem give the desired padding at all screen resolutions?

I also suspect there might be a reason, but keeping it at 50px now causes some very wonky alignment issues in the learner view. I've tests it in several places with different screen sizes and so far I don't see any issues with this change, but I definitely do not know the ins and outs of manual QA to know if there are no problems. Also open to suggestions about how else I might set up the nested component here in a way that would keep me from making an update in index.vue - maybe @MisRob would have an idea...

I'll have a look

I assume that there indeed was a specific reason to have 50px and that it was to make header text aligned with options. See the screenshot from develop:

According to the new designs for user syncing

this is to be deprecated, at least in some cases, so this change looks fine to me.

However, I am not sure if we're supposed to keep the previous layout when no sync information is visible in the menu, like for the admin. If yes, this change will cause the following regression:

I feel fine about using the new layout everywhere for the sake of consistency. Maybe let's see what designers think.

In any case, it's not a blocker. I checked all places where we use CoreMenu and haven't noticed any other issues.

rtibbles · 2021-07-19T16:22:44Z

kolibri/plugins/learn/viewsets.py

@@ -186,3 +186,40 @@ def get_queryset(self):
                lesson_assignments__collection__membership__user=self.request.user,
                is_active=True,
            )
+
+
+class LearnerSyncStatusViewset(ReadOnlyValuesViewset):


I assume this will be deleted in favour of the general viewset above?

rtibbles · 2021-07-19T16:23:25Z

kolibri/plugins/coach/assets/src/views/plan/LessonSummaryPage/ManageLessonModals.vue

@@ -55,7 +55,6 @@
      },
    },
    computed: {
-      ...mapState(['classList']),


Is this state not required?

rtibbles · 2021-07-19T16:26:21Z

kolibri/plugins/coach/assets/src/views/ClassLearnersListPage.vue

+  import commonCoreStrings from 'kolibri.coreVue.mixins.commonCoreStrings';
+  import { SyncStatus } from 'kolibri.coreVue.vuex.constants';
+  import { mapState, mapActions } from 'vuex';
+  import SyncStatusDisplay from '../../../../../core/assets/src/views/SyncStatusDisplay';


One slight issue with doing this import is that we will either duplicate the strings across the core and the coach plugin or the strings will not be included in the coach plugin (I have completely forgotten at this point which way the i18n machinery goes in this regard).

It might be simpler (for a certain value of simple) to add these components to the core API and reference them in the same way as CoreBase and CoreTable.

rtibbles · 2021-07-19T16:27:29Z

kolibri/core/device/api.py

+
+    def annotate_queryset(self, queryset):
+
+        queryset = queryset.annotate(


These feel right, we can double check after merging to see if doing this or an explicit subquery produces a more performant query.

marcellamaki · 2021-07-20T14:49:22Z

I believe this is as ready for review as I am able to make it right now. I am having an issue with some of my tests not passing, and I would be happy to chat with either @MisRob or @rtibbles if you have any suggestions. I am 90% sure it has to do with async issues but....I can't quite figure out where I'm going wrong.

jredrejo · 2021-07-20T15:31:05Z

kolibri/plugins/coach/assets/src/views/reports/ReportsControls.vue

@@ -56,6 +61,14 @@
        return isEmbeddedWebView || this.disableExport;
      },
    },
+    methods: {
+      viewLearners() {
+        this.$router.push(this.$router.getRoute('ClassLearnersListPage'));


Same as above

jredrejo · 2021-07-20T15:31:49Z

kolibri/plugins/coach/assets/src/views/home/HomePage/OverviewBlock.vue

@@ -65,10 +76,16 @@
        return this.$router.getRoute('CoachClassListPage', {}, { facility_id });
      },
    },
+    methods: {
+      viewLearners() {
+        this.$router.push(this.$router.getRoute('ClassLearnersListPage'));


if you give a name to the route in kolibri/plugins/coach/assets/src/routes/index.js it would be simpler, this.$router.push('CLASSLEARNERSLIST'),

jredrejo · 2021-07-20T15:39:26Z

kolibri/plugins/coach/assets/src/views/home/HomePage/OverviewBlock.vue

@@ -35,6 +35,17 @@
          {{ $formatNumber(learnerNames.length) }}
        </template>
      </HeaderTableRow>
+      <HeaderTableRow v-if="learnerNames.length > 0">


<HeaderTableRow v-if="learnerNames.length"> is enough if you're sure learnerNames is defined, is you need to check if learnerNames is not undefined:
<HeaderTableRow v-if="!learnerNames || !learnerNames.length">

jredrejo · 2021-07-20T15:45:08Z

kolibri/plugins/coach/assets/src/views/ClassLearnersListPage.vue

+          } else if (learnerSyncData.last_synced) {
+            const currentDateTime = new Date();
+            const TimeDifference = learnerSyncData.last_synced - currentDateTime;
+            const diffMins = Math.round(((TimeDifference % 86400000) % 3600000) / 60000);


Same as above

jredrejo · 2021-07-20T15:47:52Z

kolibri/plugins/coach/assets/src/views/ClassLearnersListPage.vue

+        if (this.isPolling) {
+          setTimeout(() => {
+            this.pollClassListSyncStatuses();
+          }, 10000);


maybe this polling interval should be a property instead, so a) it's changeable and b) it's more readable

jredrejo · 2021-07-20T15:52:12Z

kolibri/core/assets/src/views/AppBar.vue

+            const currentDateTime = new Date();
+            const TimeDifference = this.userSyncStatus.last_synced - currentDateTime;
+            const diffMins = Math.round(((TimeDifference % 86400000) % 3600000) / 60000);
+            if (diffMins < 60) {


would not be simpler something like:
if (TimeDifference < 5184000000) // 5184000000 = 60 days in milliseconds

jredrejo · 2021-07-20T16:00:11Z

kolibri/core/assets/src/views/SyncStatusDescription.vue

+      },
+    },
+    computed: {
+      syncTextDisplayMap() {


Maybe I'm too nitpicky, but seeing a switch that can be so easily replaced by an object looks ugly to me:

const statusTranslations= {SyncStatus.RECENTLY_SYNCED:this.$tr('syncedDescription') , SyncStatus.QUEUED:this.$tr('queuedDescription'), ....} return statusTranslations[this.syncStatus] || ''

looks better than a switch for me, but I can be wrong

jredrejo · 2021-07-20T16:00:49Z

kolibri/core/assets/src/views/SyncStatusDisplay.vue

+        }
+      },
+      syncIconDisplayMap() {
+        switch (this.syncStatus) {


same look and feel with the switch as above

jredrejo · 2021-07-20T16:05:43Z

kolibri/plugins/coach/assets/src/routes/index.js

@@ -64,6 +65,14 @@ export default [
      titleParts: ['activityLabel', 'CLASS_NAME'],
    },
  },
+
+  {
+    path: '/:classId/learners',


I'd add
name: 'CLASSLEARNERSLIST'
so future references to this route are easier

MisRob

Awesome work @marcellamaki :) Regarding frontend, I appreciate components architecture. I've pointed out several issues in code comments. Feel absolutely free to skip or open follow-ups for those that are marked correspondingly. I only skimmed through backend briefly. Thanks for the backend test suite - it helped me to understand the whole feature and also some frontend parts better. It was my first encounter with syncing so it was a lot of studying - thanks for the detailed PR description and all references. I haven't gotten around to testing with kolibri shell.

Regarding problems with tests, I'll have a look.

Please also see the following:

1. Sync statutes are being polled even when I visit "Learn" while not logged in. However, there is no information about the sync status in the user menu for a guest user. I assume that the only purpose of polling from `AppBar` is to fetch this information for UI or are there any more reasons? If this assumption is correct, I'd suggest removing it for guests.

2. When I navigate to the learners page from "Reports" tab in "Coach" and then click the back button, I expect to be returned to the "Reports" tab but I am taken to "Class Home" tab instead.

MisRob · 2021-07-21T07:59:50Z

kolibri/core/assets/src/views/AppBar.vue

+      mapSyncStatusOptionToLearner() {
+        if (this.userSyncStatus) {
+          if (this.userSyncStatus.active) {
+            return SyncStatus.SYNCINGSYNCING;


Syncing syncing
still syncing
stiiill syncing and syncing... :)

SyncStatus.SYNCING

MisRob · 2021-07-21T08:28:53Z

kolibri/core/assets/src/views/AppBar.vue

    beforeDestroy() {
      window.removeEventListener('click', this.handleWindowClick);
+      this.isPolling = false;


(Can be follow-up) I'd suggest using clearTimeout to stop polling similarly to https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/learn/assets/src/views/classes/ClassAssignmentsPage.vue#L49.

Depending on timing, the current implementation can cause one unnecessary call that seems to be unintentional. You can see it here, for example (please wait a while for the second call in the console):

Generally speaking, such behavior has potential to cause unexpected errors - for example when a request would be made after a user logs out to an endpoint that requires authentication. In this case, it's doesn't break anything in this way. Maybe due to our multi-apps architecture (I assume that completely another bundle is loaded after being redirected to login)? So it's not a blocker but would be good to address at some point.

MisRob · 2021-07-21T08:31:18Z

kolibri/core/assets/src/views/AppBar.vue

+        if (this.isPolling) {
+          setTimeout(() => {
+            this.pollUserSyncStatusTask();
+          }, 10000);


(Would be nice but no action needed) Might be good to save polling interval value to constants so we can locate it easily when we need to tweak it

MisRob · 2021-07-21T12:01:59Z

kolibri/core/assets/src/views/SyncStatusDisplay.vue

+        type: String,
+        default: '',
+      },
+      displaySize: {


When choosing between more predefined values, we usually add a validator to document it and also to check on those values during development. Here's one example https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/epub_viewer/assets/src/views/PreviousButton.vue#L25.

(Would be nice but no action needed) Is there any reason for not using simpler names for displaySize values? For example, small instead of sync-status-small.

No, that is a good suggestion! I have still not adjusted to file-scoped CSS and am used to having very long, descriptive class names needed to keep track of things in giant css files 😂 Here simpler is definitely better!

MisRob · 2021-07-21T12:30:26Z

kolibri/plugins/coach/assets/src/views/home/HomePage/OverviewBlock.vue

+        <template #key>
+        </template>
+        <template #value>
+          <KButton


This element is navigating users to another page so I think that using KRouterLink would be more appropriate (https://design-system.learningequality.org/buttons/)

MisRob · 2021-07-21T13:22:52Z

kolibri/plugins/coach/assets/src/views/ClassLearnersListPage.vue

+    },
+    mounted() {
+      this.isPolling = true;
+      this.pollClassListSyncStatuses({ classroom_id: this.$route.params.classId });


pollClassListSyncStatuses method doesn't accept any parameters

MisRob · 2021-07-21T13:37:09Z

kolibri/plugins/learn/api_urls.py

@@ -10,5 +10,8 @@
    r"learnerclassroom", LearnerClassroomViewset, base_name="learnerclassroom"
 )
 router.register(r"learnerlesson", LearnerLessonViewset, base_name="learnerlesson")
+router.register(
+    r"learnersyncstatus", LearnerLessonViewset, base_name="learnersyncstatus"


I haven't noticed this URL being used

kolibri/plugins/coach/assets/src/views/ClassLearnersListPage.vue

MisRob · 2021-07-21T14:16:08Z

kolibri/core/assets/src/state/modules/core/actions.js

@@ -897,3 +901,16 @@ export function notLoading(store) {
    });
  });
 }
+
+export function fetchUserSyncStatus(store, id) {


(Would be nice but no action needed) When reviewing, I was a bit confused that id can be both a user ID and a class ID. Might be helpful to document it here.

This comment is not true.

The backend expects either user to filter by a user_id or member_of to filter by a collection_id - this flexible use of id is doing no filtering at all.

MisRob · 2021-07-26T18:08:07Z

@marcellamaki @rtibbles I've briefly skimmed through the latest frontend updates and haven't noticed any issues

rtibbles

Two critical changes needed.

rtibbles · 2021-07-27T15:19:57Z

kolibri/core/device/test/test_api.py

+    def test_user_sync_status_class_single_user_for_filter(self):
+        response = self.client.get(
+            reverse("kolibri:core:usersyncstatus-list"),
+            data={"user_id": self.user1.id},


Here you are filtering by user_id but the FilterSet for this endpoint specifies user as the parameter.

You should create another UserSyncStatus during your test setup for this test to be meaningful.

rtibbles · 2021-07-27T15:21:02Z

kolibri/core/assets/src/state/modules/core/actions.js

@@ -897,3 +901,16 @@ export function notLoading(store) {
    });
  });
 }
+
+export function fetchUserSyncStatus(store, id) {


This comment is not true.

The backend expects either user to filter by a user_id or member_of to filter by a collection_id - this flexible use of id is doing no filtering at all.

rtibbles · 2021-07-27T19:27:51Z

kolibri/core/assets/src/state/modules/core/actions.js

+      getParams: { member_of: params.member_of },
+    }).then(
+      syncData => {
+        store.commit('SET_CORE_CHANNEL_LIST', _userSyncStatusState(syncData));


One last question here - why is this being stored using the SET_CORE_CHANNEL_LIST mutation? This seems like the wrong place to store this data.

I think we'd be better off not setting any store state in here, and just returning the data from the function.

Should also return the same data as we are storing if the _userSyncStatusState mapping function is important.

…ort view

… KDS alignment

…tus page

rtibbles reviewed Jul 14, 2021

View reviewed changes

rtibbles reviewed Jul 19, 2021

View reviewed changes

marcellamaki added changelog Important user-facing changes TODO: needs review Waiting for review labels Jul 20, 2021

marcellamaki requested a review from MisRob July 20, 2021 14:47

marcellamaki marked this pull request as ready for review July 20, 2021 14:47

jredrejo reviewed Jul 20, 2021

View reviewed changes

MisRob reviewed Jul 21, 2021

View reviewed changes

marcellamaki changed the title ~~(WIP) UX for learner and coach/admin single user syncing~~ UX for learner and coach/admin single user syncing Jul 26, 2021

marcellamaki requested a review from rtibbles July 26, 2021 22:47

rtibbles requested changes Jul 27, 2021

View reviewed changes

rtibbles changed the base branch from develop to release-v0.15.x July 27, 2021 17:59

indirectlylit added this to the 0.15.0 milestone Jul 27, 2021

rtibbles reviewed Jul 27, 2021

View reviewed changes

rtibbles approved these changes Jul 27, 2021

View reviewed changes

marcellamaki added 13 commits July 27, 2021 16:38

(WIP) Add basic sync display to learner user menus

eaadb47

(WIP) Add Coach/Admin side display of class syncing overview

6fabceb

Add tests for SyncStatusDisplay component

539c151

(WIP) Start adding API endpoint.

35609a2

Restructure API endpoint and merge in model changes

dd74f43

(WIP) connect endpoint to Learner dropdown profile menu view

6bd68f4

(WIP) Refactor API endpoint and corresponding action

10c8e00

Refactor to separate ClassList and User API endpoints

1cf05e4

Refactor endpoint

5c2391c

Update UX strings and spacing

f53a6ad

Add transfer session data to queryset

70679d8

Add additional access to learner sync status list from coach quiz rep…

46076df

…ort view

Code cleanup and refactoring

59ad665

marcellamaki and others added 12 commits July 27, 2021 16:38

Add additional conditional check for returned data

8a84605

(WIP) Add python tests for UserSyncStatus

708d9b1

Code refactoring after PR feedback - readability, conditional checks,…

865bf8b

… KDS alignment

Add PR feedback cont. readability, variable usage

2cc5a21

Add PR feedback cont. readability, variable usage

3873b6b

Fix javascript tests, and fix backlinking from class list of sync sta…

b818cb5

…tus page

Add very basic python tests

8d37d39

Fix subquery for user sync statuses

f885fb1

Refactor fetchUserSyncStatus action

53f917b

Fix tests, again

5b30488

Remove leftover/mistaken mutation. Add new KDS 'disconnected' icon

2b88ddd

Add special case for role based permissions for non-syncable models.

dcf3976

rtibbles force-pushed the ux-automated-learner-side-syncing branch from 0daf73b to dcf3976 Compare July 27, 2021 23:44

rtibbles merged commit 32f9417 into learningequality:release-v0.15.x Jul 28, 2021

This was referenced Jul 28, 2021

Create front end for automated syncing behaviors of individual learner syncing #8178

Closed

Create front end for coach view of user sync statuses #8179

Closed

Sync notifications updates #8247

Merged



		class ClassListSyncStatusViewSet(ReadOnlyValuesViewset):
		values = (


		def annotate_queryset(self, queryset):

		queryset = queryset.annotate(

UX for learner and coach/admin single user syncing #8202

UX for learner and coach/admin single user syncing #8202

Conversation

marcellamaki commented Jul 13, 2021 • edited by rtibbles

Summary

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellamaki commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jredrejo Jul 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jredrejo Jul 20, 2021 • edited

Choose a reason for hiding this comment

MisRob left a comment

Choose a reason for hiding this comment

2. When I navigate to the learners page from "Reports" tab in "Coach" and then click the back button, I expect to be returned to the "Reports" tab but I am taken to "Class Home" tab instead.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob commented Jul 26, 2021

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles Jul 27, 2021 • edited

Choose a reason for hiding this comment

marcellamaki commented Jul 13, 2021 •

edited by rtibbles

jredrejo Jul 20, 2021 •

edited

jredrejo Jul 20, 2021 •

edited

rtibbles Jul 27, 2021 •

edited