Fix list activity notifications

[AlexVelezLl]

Summary

There have been a large number of inconsistencies between generating notifications on an original device, and creating notifications when a device is importing a facility from another device. Mainly because the notifications are not created in chronological order, but are created as the log records are imported. Which leads to the main assumption that notifications are created in a chronological order being incorrect. This PR fixes the following:

• In the ClassroomNotifications API, Filter and return notifications in chronological order instead of sorting them by id.
  • Updated the before and after params to receive timestamps instead of an id.
  • A new desc index was created on the timestamp field of the learnerprogressnotification table.
  • A new ClassroomNotificationsFilter class was created to update the way to filter records to use filtersets.
• Until now, if a notification already existed, creating/updating the notification generated by other resources/attempts was avoided. But now the notifications are updated in the following cases:
  • The lesson completed notification is updated if a resource is processed that has a greater completion_time than the existing notification had. So we always keep the latest resource completion_time as the lesson completion_time.
  • (for exercise resources) The resource started notification is updated if an attempt is processed that has a lower start_time than the existing notification had. So we always keep the earliest attempt start_time as the resource start_time.
  • The lesson started notification is updated if a resource is processed that has a lower start_time than the existing notification had. So we always keep the earliest resource start_time as the lesson start_time.
  • The help needed notification is updated if the student has newer attempts and keeps having incorrect interactions. So we always keep the timestamp of the most recent attempt that has an incorrect interaction. (This is also a change for the original device).
• Avoid creating notifications when a content summary log is created (in batch) for an exercise resource, as the resource started time for exercise resources is determined by the first attempt and not by the content summary log.
• Now the resource completion time for exercise resources is determined by the earliest attempt completion_time logged in the MasteryLog, rather than using the content summary log end_time.
• Until now, completion notifications were avoided if the contentsummarylog of a resource had progress <1. But this was not true in cases where the student clicked on "try again" and the progress was = 0, but the notification should still be created when importing. So now the summarylog progress and completion_time are taken into account to determine whether or not to:
  • Create a resource completed notification.
  • Determine if all lesson resources are completed, to create a lesson completed notification (this last one is a bug that could also happen on the original device).
• Some refactor and code reuse to avoid many parts of repeated code in the notifications api.
• In the frontend, in the coachnotifications module:
  • the list of notifications was not properly being sorted.
  • We now sort notifications, allNotifications by timestamp.
  • If the first load of notifications returned just Answered notifications, an empty notifications list was rendered without option for the user to see more notifications. So now we load more notifications until the user could see the latests notifications.

References

Closes #11782.

Reviewer guidance

Set up a device with multiiple lessons/resources/quizes and play with it to have different types of notifications. Then import the facility to a new device and check that notificatoins match.

---

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

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-.pex
Windows Installer (EXE)	kolibri-0.17.0b3.dev0+git.43.g5bca68f7-windows-setup-unsigned.exe
Debian Package	kolibri_0.17.0b3.dev0+git.43.g5bca68f7-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.17.0b3.dev0+git.43.g5bca68f7.dmg
Android Package (APK)	kolibri-0.17.0b3.dev0+git.43.g5bca68f7-0.1.3-debug.apk
TAR file	kolibri-0.17.0b3.dev0+git.43.g5bca68f7.tar.gz
WHL file	kolibri-0.17.0b3.dev0+git.43.g5bca68f7-py2.py3-none-any.whl

[rtibbles]

A good start, but I think we can make the changeover to timestamp a bit more systematically.

One other thing that would be important to investigate in this changeover, is whether we need to add a db_index to the timestamp field: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/notifications/models.py#L117 the primary key (id) is indexed by default, so querying solely by timestamp may be considerably less performant.

[rtibbles]

Why has this been removed?

[AlexVelezLl]

The before param is already set inside the moreNotificationsForClass action https://github.com/AlexVelezLl/kolibri/blob/cdb559e453c0c6468ef95d4c97ad7bfdb342d9ec/kolibri/plugins/coach/assets/src/modules/coachNotifications/index.js#L104. So it was repeated code and thought it was better to keep the one in the moreNotificationsForClass action,

[rtibbles]

A few more things that seem important here:

• Clearly defining the role of the masterylogs for the retry behaviour completion notification generation
• Reverting the change from 3 incorrect attempts on the same question to 4
• Reverting the change from setUpTestData to setUp if at all possible

One tiny nit about variable name/casing - but this is not blocking.

[AlexVelezLl]

Thank you @rtibbles! I have updated the PR :)

All changes addressed

[rtibbles]

This looks good to go from the code side @radinamatic if you could do final QA to confirm that the issue you reported is fixed, then we can merge this.

[pcenov]

Hi @AlexVelezLl @radinamatic - the only issue I was able to identify while testing this is the following error in the console, with seemingly no user facing consequences but I'm going to mention it to be on the safe side:

TypeError: Cannot read properties of undefined (reading 'correct')

Logs: LogsAndDB.zip

Other than that everything else is working properly and I am seeing all of the notifications.

[rtibbles]

Looking at the component in question, this looks like an attemptLog was somehow undefined? https://github.com/learningequality/kolibri/blob/develop/kolibri/core/assets/src/views/AttemptLogItem.vue#L83

This is probably not caused by the changes here - @AlexVelezLl if you feel like chasing this down, please do, otherwise we can file a follow up issue.

[AlexVelezLl]

Im gonna give a try @rtibbles. If it becomes something too big we can file a follow up issue :)

[AlexVelezLl]

I did a little research, and this issue is happening in the ExamReport page of exercise resources because in the AttemptLogList we iterate through the section.questions, but for a practice resource we dont have attemptLogs for every question, since we dont necessarily have attempted every question, so that why this attemptLog is undefined here https://github.com/AlexVelezLl/kolibri/blob/17ab6f78c8808dbd7ae2f40b49b9b16a1582fc03/kolibri/core/assets/src/views/AttemptLogList.vue#L135.

Im not sure what the order should be in the exercises attemptLog and how should it be iterated. So it will be better to file a new issue to solve Exam Reports of exercise resources @rtibbles @pcenov @radinamatic.

[rtibbles]

Thanks, @AlexVelezLl - seems like this is likely a regression from the EQM work.

[rtibbles]

Unrelated regression has been filed as a follow up. This is good to go!