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

First pass at regression test and fix for SoUD FacilityUser sync conflict #8438

Merged

Conversation

jamalex
Copy link
Member

@jamalex jamalex commented Sep 16, 2021

Summary

In Morango, when a model is modified on both sides, it results in a merge conflict, and one copy of the data wins (and the serialized copy of the one that didn't win is stored in case the app wants to resolve the conflict later).

We don't allow FacilityUsers to be synced back across a single-user connection -- they can only be read. That's why we don't allow users to edit their profile on a SoUD -- but that doesn't stop some backend machinery from still writing to the FacilityUser on a SoUD. In particular, we saw a case with Django writing to the last_login field on the FacilityUser that made it "dirty", and then when a full_name change was synced in from a full-facility device, it conflicted, and the tablet's copy won, so the data with the updated full_name was relegated to the conflicting_serialized_data field in the Store.

This PR prevents attempts to prevent changes made to a FacilityUser on a SoUD from causing a conflict with incoming changes. The regression tests work as intended, but my hook doesn't currently seem to be getting picked up, so the tests are still failing. Ideas welcome around what I may be missing.

Reviewer guidance

I'm running the tests with:

INTEGRATION_TEST=true pytest kolibri/core/auth/test/test_morango_integration.py::EcosystemSingleUserAssignmentTestCase::test_facility_user_conflict_syncing_from_tablet --pdb --capture=no

and

INTEGRATION_TEST=true pytest kolibri/core/auth/test/test_morango_integration.py::EcosystemSingleUserAssignmentTestCase::test_facility_user_conflict_syncing_from_laptop --pdb --capture=no

Testing checklist

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

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 the work-in-progress Not ready for review label Sep 16, 2021
@jamalex jamalex added this to the 0.15.0 milestone Sep 16, 2021
@radinamatic
Copy link
Member

radinamatic commented Sep 17, 2021

I confirm that the change of learner's Full Name does get synced to the learn-only device, but could it be that it cannot sync on it's own, and needs an interaction with resources, to sort of get admitted to the syncing train? 🤔

This is the scenario:

  • Set up a learn-only device.
  • Synced with server, class and lesson visible to learner.
  • Disconnected the learn-only device from the network.
  • Edited the Full Name of the learner on the server device.
  • Reconnected the learn-only device and observed the sync on both: while devices synced twice, and I was refreshing the Profile page on the learn-only device, Full Name change was not visible on the learn-only device.
  • Only after I interacted with a lesson, and another sync happened, I was able to see the new Full Name in the Profile.

logs-and-DB-Windows10-learn-only.zip
logs-and-DB-Ubuntu18-server.zip

new-full-name

@radinamatic
Copy link
Member

I repeated the scenario 5 times more, without engaging with resources on the learn-only device, and almost invariably the full name change becomes visible to the learner only after the 3rd or the 4th sync.

DB-and-logs-Ubuntu18.zip
DB-and-logs-Win10.zip

6th-name-change.mp4

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.

Updates look good, and fix confirmed in manual testing.

@rtibbles rtibbles merged commit 111fd35 into learningequality:release-v0.15.x Sep 17, 2021
@jamalex jamalex removed the work-in-progress Not ready for review label Jan 24, 2022
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.

None yet

3 participants