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

Fix learner sync status reporting on server device #11244

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Sep 14, 2023

Summary

  • Ensures that we always return a non-null sync status
  • This should show up in the frontend as a sync being shown as 'not recently synced' - additional fixes are needed in Morango to ensure that the sync session gets properly attached to the user sync status.
  • Fixes the check for whether we have a local context, which previously was always erroneously early returning.
  • Combined with Ensure initialization signals get fired during sync morango#201 this fixes the setting of learner sync status sync session foreign keys

References

Fixes #10588

Reviewer guidance

Do tests pass? Does the new test cover the scenario of a sync session not being assigned to the status?

Use the morango code in the linked PR and do pip install -e <path to morango repo> to do a full manual test of this.

Install an LOD learner from a server running this PR, and once it has synced, the sync status should properly display.

See screenshot of the active sync updating properly:

Screenshot from 2023-09-14 16-47-28


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 rtibbles added the TODO: needs review Waiting for review label Sep 14, 2023
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Sep 14, 2023
@rtibbles rtibbles changed the title If all else fails, return NOT_RECENTLY_SYNCED as a status. Fix learner sync status reporting on server device Sep 14, 2023
@rtibbles rtibbles requested a review from bjester September 14, 2023 23:50
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Code makes sense. I only miss a case in test_sync_event_hook_utils.py to test with local_context set to None.

In any case, tested it and it works perfectly using the morango PR code:
image

@rtibbles
Copy link
Member Author

Thanks @jredrejo - I thought of adding that test case just after I signed off last night, so glad to have my thought validated. I'll add one now!

@rtibbles
Copy link
Member Author

Note, it's possible that this PR will also fix this issue: #11234

@rtibbles rtibbles force-pushed the ive_lost_that_syncing_feeling branch from 17989d0 to ed6515b Compare September 15, 2023 18:40
@rtibbles rtibbles merged commit 77c42f7 into learningequality:release-v0.16.x Sep 15, 2023
@rtibbles rtibbles deleted the ive_lost_that_syncing_feeling branch September 15, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coach > View learners - Missing 'Device status' and 'Last synced' values
3 participants