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

Ensure initialization signals get fired during sync #201

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Sep 14, 2023

Summary

  • The default transfer_stage value set for contexts is INIITIALIZING - because of this, the started signals for this stage are never fired, as the middleware processing always flags it as at that stage.
  • To resolve this, I've added a manual firing of the initializing started signal when a transfer session object is created
  • Adds a test to ensure this is the case.

TODO

  • Have tests been written for the new code?

Reviewer guidance

I initially attempted a more general approach by creating a NO_STAGE stage to allow the initializing started signal to get triggered, but it broke another test. While it fixed the test here, it did not seem to alter the behaviour in Kolibri. I also didn't know whether this would have implications for the Kolibri sync commands, which seem to explicitly fire the initializing started event: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/auth/management/utils.py#L615

Issues addressed

This should resolve learningequality/kolibri#10588 by ensuring that when an inbound sync happens, the pre-transfer hooks are properly executed: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/core/auth/sync_event_hook_utils.py#L130

@rtibbles rtibbles requested review from jamalex and bjester September 14, 2023 20:54
Manually fire initialization started signal.
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

This shouldn't be required. The issue must exist in Kolibri still

@rtibbles
Copy link
Member Author

Closing in favour of learningequality/kolibri#11268

I do find it a little counterintuitive that the initializing stage doesn't fire a started signal - but also agree that if it's going to happen in Morango, it should happen for both server and client.

@rtibbles rtibbles closed this Sep 21, 2023
@bjester
Copy link
Member

bjester commented Sep 21, 2023

@rtibbles It is indeed counterintuitive, I agree. I think I might have added the signals before the chain-of-responsibility architecture, so we didn't have the context classes whereas before everything revolved around the sync and transfer model objects (which don't exist at that time obviously).

Firing the started signal in a standard way would need to be handled in the controller. There's some awkwardness there with the existing sync clients though. We'd also need to ensure that Kolibri's signal->hook parsing is defensive against missing sync and transfer session objects

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.

2 participants