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

Separate signal attachment between morango core and kolibri sync logic #11268

Merged

Conversation

bjester
Copy link
Member

@bjester bjester commented Sep 18, 2023

Summary

  • Updates sync command logic to attach to initializing.started signal manually, which itself was hacked to fire manually
  • Updates the Kolibri app logic to attach to the initializing.completed signal, which is supported in morango and was the original implementation

References

Closes learningequality/morango#201
#11244

Reviewer guidance

See #11244


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

@bjester bjester added the TODO: needs review Waiting for review label Sep 18, 2023
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... and removed TODO: needs review Waiting for review labels Sep 18, 2023
@bjester bjester requested a review from rtibbles September 18, 2023 19:47
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.

Code changes look precisely right to me - I hadn't done this previously, as I didn't know if there would be an issue with firing the signals on completed rather than started. Will manually test and approve!

@bjester
Copy link
Member Author

bjester commented Sep 21, 2023

@rtibbles Yes, the change to started was to avoid overlap and it connects with the changes that are indeed needed in Morango, but I'm planning to incorporate that with new flexibility in the Morango clients

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.

Manual testing also checks out:
image

@rtibbles rtibbles merged commit cffbec3 into learningequality:release-v0.16.x Sep 21, 2023
34 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants