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

Add sync operation support to Kolibri plugins with migration for exam logs #8527

Merged

Conversation

bjester
Copy link
Member

@bjester bjester commented Oct 22, 2021

Summary

  • Adds support for defining Morango sync operations on a per plugin basis
  • Adds Kolibri version information to Morango instance data shared during sync
  • Establishes guidelines for deciding whether to use a sync hook or a sync operation
  • Reorganizes existing sync hooks/operations to align with new guidelines
  • Re-enables syncing of exams for single-user devices
  • Adds new sync operation to migrate exam logs when syncing full facilities from older Kolibris
  • Adds and updates Morango integration tests

References

Resolves #8481
Resolves #8487

Reviewer guidance

Note: notification generation post-sync hasn't yet been updated for new log structure

  • Test full facility sync with Kolibri version < 0.15 and verify exam logs
  • Single user syncing of exams and related logs

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

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Looking good - one comment on a comment, any blockers from your side?

kolibri/core/auth/utils.py Outdated Show resolved Hide resolved
@rtibbles rtibbles mentioned this pull request Oct 26, 2021
9 tasks
@bjester bjester marked this pull request as ready for review October 29, 2021 16:47
@bjester bjester requested a review from rtibbles October 29, 2021 16:47
@bjester bjester added the TODO: needs review Waiting for review label Oct 29, 2021
@bjester bjester added this to the 0.15.0 milestone Oct 29, 2021
@bjester bjester added the python label Oct 29, 2021
@bjester bjester changed the title Post sync hook for migrating exam logs Add sync operation support to Kolibri plugins and operation to migrate exam logs Oct 29, 2021
@bjester bjester changed the title Add sync operation support to Kolibri plugins and operation to migrate exam logs Add sync operation support to Kolibri plugins with migration for exam logs Oct 29, 2021
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Read through looks good, one update needed for the notification generation.

ExamLog
)
logger.info("Migrating {} ExamLogs records".format(len(exam_logs_ids)))
migrate_from_exam_logs(ExamLog.objects.filter(id__in=exam_logs_ids))
Copy link
Member

Choose a reason for hiding this comment

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

Realizing that this won't handle updated ExamAttemptLogs that are associated with an otherwise unchanged ExamLog. Let's file this as a follow up issue that I can tackle.

kolibri/core/notifications/kolibri_plugin.py Outdated Show resolved Hide resolved
@rtibbles rtibbles merged commit e154cae into learningequality:release-v0.15.x Oct 29, 2021
@pcenov
Copy link
Member

pcenov commented Nov 1, 2021

@bjester today I tested the Buildkite builds for this one and found some quiz related syncing issues. I was wondering whether you are aware of these:

  1. When there is a Quiz completed by a Learner on a learner only device the Coach is not able to see the scores of the learner and questions answered:

2021-11-01_15-25-37

2021-11-01_15-27-18

  1. I imported a full 0.14 facility to a 0.15 one and synced it. When I go to check the scores of a learner as a Coach the page with the quiz scores is not displayed and there's an error in the console: Uncaught (in promise) TypeError: currentAttempt.interaction_history.filter is not a function

2021-11-01_18-16-35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
3 participants