Add morango integration tests for syncing new courses#14214
Conversation
...and verifying the syncing functionality of the partitioning structure Co-authored-by: ozer550 <prathameshd419@gmail.com>
github-actions
Bot
added
DEV: backend
Build Artifacts
|
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Well-structured integration tests for course model syncing — both the full-facility and single-user paths are covered with clear assertions.
CI passing. PR targets release-v0.19.x (not develop), which is fine since no new user-facing strings are added.
- blocking: None
- suggestion: One minor suggestion on the
generate_base_databug fix - praise: Good bug fix, thorough
deserializeverification, helpful module docstring
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| )[0] | ||
| staff = FacilityUser.objects.using(self.db_alias).filter( | ||
| roles__isnull=True, devicepermissions=None | ||
| roles__kind=role_kinds.COACH, devicepermissions=None |
There was a problem hiding this comment.
praise: Good catch — the old filter (roles__isnull=True) was identical to the learner filter, so staff and learner could have been the same user. This fix correctly fetches a coach.
| staff = FacilityUser.objects.using(self.db_alias).filter( | ||
| roles__isnull=True, devicepermissions=None | ||
| roles__kind=role_kinds.COACH, devicepermissions=None | ||
| )[0] |
There was a problem hiding this comment.
suggestion: devicepermissions=None excludes device admins, but generateuserdata only creates one coach. If that coach happens to also have device permissions in a future change, this would return an empty queryset and [0] would raise IndexError. Consider adding an error message or using .first() with an assertion, similar to how facility uses .get(). Low priority since the test fixture is controlled.
There was a problem hiding this comment.
I'm not too concerned about this since it's a test fixture. Also, if this were to break, it'd be better to fix what broke this than make this looser. A pure coach, without device permissions, is a worthwhile test fixture.
There was a problem hiding this comment.
You're right — a loud failure is better than a loose assertion in a test fixture. Withdrawing this finding.
| """ | ||
| Tests related specifically to integration with Morango. | ||
|
|
||
| When creating models for servers passed into test methods from the `multiple_kolibri_servers` |
There was a problem hiding this comment.
praise: This module docstring is a great addition — the create_model vs direct ORM gotcha is exactly the kind of thing that trips people up when writing new integration tests.
| s1.db_alias | ||
| ).get(course_session_id=course_session.id, collection_id=classroom.id) | ||
|
|
||
| self.assertIsNone( |
There was a problem hiding this comment.
praise: Verifying that deserialize correctly nullifies user FK fields on single-user sync (learner device) while preserving them on the coach device is exactly what the issue asked for — exercises both the partitioning structure and the deserialize logic in a single test.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Test changes make sense, this is good to merge.
4 participants
...and verifying the syncing functionality of the partitioning structure
Summary
deserializefunctionality, but also that the course partitioning structure is functioning appropriately.test_single_user_course_syncingwas bootstrapped with an LLM, then corrected and added uponReferences
closes #14131
Reviewer guidance