Learner devices status update #12153
Conversation
github-actions
bot
added
DEV: backend
Build Artifacts
|
|
Hey @ellipsis, give me a code review |
|
OK! Reviewing this PR... Responding to this comment by @rtibbles. For more information about Ellipsis, check the documentation. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 2d91ec9 in 1 minute and 31 seconds
More details
- Looked at
103lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. /kolibri/core/device/migrations/0022_learner_device_status_multiple_devices.py:20
- Draft comment:
The migration correctly updates the 'user' field from OneToOneField to ForeignKey to support multiple device statuses per user, aligning with the PR's intent. - Reason this comment was not posted:
Confidence changes required:0%
The migration file correctly changes the 'user' field in the 'LearnerDeviceStatus' model from a OneToOneField to a ForeignKey. This change is necessary to allow multiple device statuses per user, which is the intent of the PR as described. The migration dependencies are correctly set to the last migration in the device app. The field changes are appropriate and use CASCADE for deletion, which is standard for user-related foreign keys in Django.
2. /kolibri/core/device/models.py:613
- Draft comment:
The change from OneToOneField to ForeignKey for the 'user' field in 'LearnerDeviceStatus' is correctly implemented to support multiple statuses per user. - Reason this comment was not posted:
Confidence changes required:0%
The model change in 'LearnerDeviceStatus' from OneToOneField to ForeignKey for the 'user' field is correctly implemented. This change is necessary to support multiple device statuses per user, which is the intent of the PR. The related_name 'learner_device_status' is appropriately set to handle reverse relations from the 'FacilityUser' model.
3. /kolibri/core/device/kolibri_plugin.py:75
- Draft comment:
TheLearnerDeviceStatusOperationclass handles the downgrade scenario effectively by deleting excessLearnerDeviceStatusrecords that could cause issues when syncing to a previous version. - Reason this comment was not posted:
Confidence changes required:0%
TheLearnerDeviceStatusOperationclass is introduced to handle the downgrade scenario when syncing to a previous version that does not support multipleLearnerDeviceStatusrecords per user. The logic checks if it's a single user sync and deletes statuses accordingly, or deletes all statuses for a facility if it's a facility sync. This is a crucial addition to handle data consistency across different versions.
Workflow ID: wflow_Q6VkcI9MinG9SO0s
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 6 days left in your free trial, upgrade for $20/seat/month or contact us.
| context.sync_session.client_instance_id | ||
| if context.is_server | ||
| else context.sync_session.server_instance_id | ||
| ) |
There was a problem hiding this comment.
So this downgrade method is called when this device is the producer of sync data. When it's a pull, then it must be the server if it's the producer, so restricting the learner status to only the client_instance_id device makes sense.
Although, when it's a push, in order for the device to only send one status, it can first consider whether the current device has created a sync status. If the current device has, we would still want to exclude the client_instance_id, because being a push producer (aka not context.is_server), the client's status is still what matters. If the current device hasn't created one, then we would want to delete all but the most recent user status.
So in summary, I don't think this code will function as expected when the device is pushing. For pulling, this looks okay.
When it's pushing without a user sub-partition, we would want to ensure the statuses are unique for each user, preferably using the most recent status for each user, meaning no explicit filtering on the instance. So the same approach for pulling doesn't quite work for pushing.
There was a problem hiding this comment.
Thanks @bjester - I didn't think I'd get it right first time. If you have any ideas on how to write a robust test case for this, so I can be sure of covering the bases, I'm very open to inspiration!
There was a problem hiding this comment.
Honestly coming back to the conversation about this, it was difficult to wrap my head around it! I realized the logic isn't straightforward at all 🫠
I was wondering about testing. Since this relies on different versions, it doesn't seem straightforward either. The KolibriVersionedSyncOperationTestCase demonstrates how you could possibly set up a unit test. For sync integration tests, I believe it could be possible to set one up overriding CUSTOM_INSTANCE_INFO which influences the version that morango uses and communicates during syncing.
There was a problem hiding this comment.
Ah yeah, I had forgotten about the custom instance info, that does seem like the right way to go for the integration tests. Will take a look at the version sync tests too, thanks!
Update logic to preserve most recent status for full facility sync. Add tests.
rtibbles
force-pushed
the
learnerdevicesstatus
branch
from
aa70fae to
e044982
Compare
|
Hi @rtibbles no issues observed while checking the above described QA scenario - the devices were syncing correctly. I do see some errors in the server logs so posting these here for you in case you find anything that needs to be fixed: |
Looks like some DB locking errors |
Summary
References
Fixes #12043
Reviewer guidance
The logic is relatively straight forward, I think - I am just not sure how to do any sort of test of this change, especially the version sync hook.
QA specific guidance:
Install the PR asset onto a server device and onto two single user devices.
Setup syncing with those two single user devices to the server for the same learner account.
Setup 0.16.1 onto another server device, and import the facility.
Setup another single user syncing device using 0.16.1 and import the same learner again.
Ensure syncing continues unabated.
Testing checklist
PR process
Reviewer checklist
yarnandpip)Summary:
This PR updates the
LearnerDeviceStatusmodel to support multiple device statuses per user and adds a sync operation to manage data consistency across device syncs.Key points:
LearnerDeviceStatus.userfromOneToOneFieldtoForeignKey.LearnerDeviceStatusOperationto handle deletion of problematicLearnerDeviceStatusrecords during sync.LearnerDeviceStatusHookto trigger the new operation during sync.0022_learner_device_status_multiple_devices.pyfor database schema update.Generated with ❤️ by ellipsis.dev