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

Upgrade to Morango 0.6.6a0, with a test, and test refactoring #8449

Merged
merged 5 commits into from Sep 21, 2021

Conversation

jamalex
Copy link
Member

@jamalex jamalex commented Sep 20, 2021

Summary

In order to fix #8439, this PR bumps Kolibri to use the (yet unreleased) Morango 0.6.6 that has been pushed to learningequality/morango#134. It also adds a test for #8439, and some considerable (but quite superficial) refactoring that simplifies and cleans up the ecosystem tests overall.

References

Fixes #8439.
Relies on the release of learningequality/morango#134.

Reviewer guidance

Testing prior to the release of Morango 0.6.6 would require:

  • Checking out this branch (jamalex/fsics-fix) in Kolibri.
  • Checking out the frikn_fsic_fix branch in Morango.
  • Within the Kolibri venv, pip install -e <path to morango source tree>.
  • INTEGRATION_TEST=true pytest kolibri/core/auth/test/test_morango_integration.py

Testing checklist

  • Contributor has fully tested the PR manually
  • 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

@jamalex jamalex added this to the 0.15.0 milestone Sep 20, 2021
@@ -16,6 +16,9 @@
from morango.models.core import DatabaseIDModel
Copy link
Member Author

Choose a reason for hiding this comment

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

This file contains some utilities to support the syncing tests. I added a few helper methods on the server class to reduce boilerplate cruft in the test file.


# TODO: add back in after single-user exam assignment is reinstated
# from kolibri.core.logger.models import ExamAttemptLog
# from kolibri.core.logger.models import ExamLog
Copy link
Member Author

Choose a reason for hiding this comment

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

Because they won't pass until we reinstate single-user quiz syncing, I commented out the related test portions here (with a "TODO" by each, cited in the commented-out plugin file).

@@ -132,14 +134,24 @@ class EcosystemTestCase(TestCase):

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of the rest of this file (except where otherwise noted) is just cleanup and refactors of the existing tests, to use the helper methods and some less verbose idioms.

.filter(exam_id=assignment_t.exam_id, user_id=self.learner.id)
.exists()
)
# TODO: uncomment the following once single-user exam assignment syncing is reinstated
Copy link
Member Author

Choose a reason for hiding this comment

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

The section above is commented out because we don't support quizzes yet.

def test_facility_user_conflict_syncing_from_laptop(self, servers):
self._test_facility_user_conflict(servers, False)

def _test_facility_user_conflict(self, servers, tablet_is_client):
Copy link
Member Author

Choose a reason for hiding this comment

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

This one has just been moved down to this new class (rather than being stuck in the clutter of the assignment testing class).

)

@multiple_kolibri_servers(3)
def test_kolibri_issue_8439(self, servers):
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the actual new test that was introduced here. It's short but effective at catching the issue.

@jamalex jamalex changed the title Upgrade to Morango 0.6.6 (forthcoming) with a test, and test refactoring Upgrade to Morango 0.6.6a0, with a test, and test refactoring Sep 21, 2021
@radinamatic
Copy link
Member

...and we have the correct sync of the learner past progress reflected on a new setup of the learn-only device! 🎉

I repeated the test case from #8439 (twice, just to be sure), and I can see correctly all the past progress on the 2 lessons when I erase the app from the tablet and install from scratch. Great job!!! 👍🏽

Win 10 (server) Android tablet (learner)
Win10 (start)  Running  - Oracle VM VirtualBox_037 2021_09_21_05 54 44 2021_09_21_06 08 10

Copy link
Member

@radinamatic radinamatic 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, merge away! 💯

@rtibbles rtibbles merged commit 6903afe into learningequality:release-v0.15.x Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single user syncing - Lesson completion is not restored after second setup of the same learner account
4 participants