added user and facility flag #3460
Conversation
📝 WalkthroughWalkthroughAdds UserFlag and FacilityFlag: new DRF viewsets with CRUD, filtering, available-flags endpoints, Pydantic specs validating and resolving relations, registry-integrated transactional create/destroy, superuser-only authorization handlers, router registration, and comprehensive API tests covering lifecycle and edge cases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@care/emr/api/viewsets/facility_flag.py`:
- Around line 78-86: The registry is being updated before the DB save in
perform_create which can leave the FlagRegistry inconsistent if the save fails;
inside perform_create's transaction, call super().perform_create(instance) first
to persist the instance and only then call
FlagRegistry.register(FlagType.FACILITY, instance.flag); ensure perform_destroy
preserves the current ordering where super().perform_destroy(instance) runs
first and FlagRegistry.unregister(FlagType.FACILITY, instance.flag) runs after
the delete so the registry change only happens after a successful DB operation.
In `@care/emr/api/viewsets/user_flag.py`:
- Around line 64-72: The registry update order is wrong: move the DB operations
before mutating the in-memory FlagRegistry to avoid leaving the registry
inconsistent if the save/delete fails; specifically, in perform_create call
super().perform_create(instance) inside transaction.atomic() first and only
after it returns successfully call FlagRegistry.register(FlagType.USER,
instance.flag), and in perform_destroy call super().perform_destroy(instance)
first inside transaction.atomic() and only then call
FlagRegistry.unregister(FlagType.USER, instance.flag); keep the
transaction.atomic() scope but do not rely on it to rollback in-memory
register/unregister side effects.
In `@care/emr/tests/test_facility_flag_api.py`:
- Around line 329-342: The current perform_destroy unregisters the flag
unconditionally, which breaks multi-facility use; update perform_destroy in the
view handling FacilityFlag deletion to only call
FlagRegistry.unregister(FlagType.FACILITY, flag_name) after confirming no other
FacilityFlag records exist for that flag (e.g., query
FacilityFlag.objects.filter(flag=flag_name).exclude(pk=self.get_object().pk).exists()
or similar using the instance's flag value) so the registry is only updated when
this was the last facility instance of that flag.
🧹 Nitpick comments (2)
care/emr/tests/test_user_flag_api.py (1)
8-20: Consider addingtearDownto clean up registered flags.The test registers flags in
setUpbut never unregisters them. SinceFlagRegistry._flagsis a class-level dictionary, these registered flags persist across test runs. It would be... nice if tests cleaned up after themselves.🧹 Suggested tearDown implementation
def setUp(self): super().setUp() # Register test flags FlagRegistry.register(FlagType.USER, "TEST_FLAG") FlagRegistry.register(FlagType.USER, "TEST_FLAG_2") FlagRegistry.register(FlagType.USER, "BETA_FEATURES") self.superuser = self.create_super_user(username="superuser") self.normal_user = self.create_user(username="normaluser") self.target_user = self.create_user(username="targetuser") self.base_url = reverse("user-flags-list") + def tearDown(self): + # Clean up registered flags to prevent test pollution + FlagRegistry.unregister(FlagType.USER, "TEST_FLAG") + FlagRegistry.unregister(FlagType.USER, "TEST_FLAG_2") + FlagRegistry.unregister(FlagType.USER, "BETA_FEATURES") + super().tearDown() + def get_detail_url(self, external_id): return reverse("user-flags-detail", kwargs={"external_id": external_id})care/emr/tests/test_facility_flag_api.py (1)
361-364: Helper method is concise and functional.Consider adding this helper to the base test class if it's useful across multiple test files.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
care/emr/api/viewsets/facility_flag.pycare/emr/api/viewsets/user_flag.pycare/emr/resources/facility_flag/__init__.pycare/emr/resources/facility_flag/spec.pycare/emr/resources/user_flag/__init__.pycare/emr/resources/user_flag/spec.pycare/emr/tests/test_facility_flag_api.pycare/emr/tests/test_user_flag_api.pycare/security/authorization/__init__.pycare/security/authorization/facility_flag.pycare/security/authorization/user_flag.pyconfig/api_router.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/resources/facility_flag/spec.pycare/emr/tests/test_user_flag_api.pyconfig/api_router.pycare/security/authorization/__init__.pycare/emr/resources/user_flag/spec.pycare/emr/api/viewsets/user_flag.pycare/emr/tests/test_facility_flag_api.pycare/emr/api/viewsets/facility_flag.pycare/security/authorization/user_flag.pycare/security/authorization/facility_flag.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django’s built-in tools for testing (unittest and pytest-django) to ensure code quality and reliability.
Files:
care/emr/tests/test_user_flag_api.pycare/emr/tests/test_facility_flag_api.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: aravindm4
Repo: ohcnetwork/care PR: 2585
File: care/facility/api/viewsets/facility_flag.py:14-27
Timestamp: 2024-11-26T05:41:40.226Z
Learning: In the `care/facility/api/viewsets/facility_flag.py` file and similar viewsets in this Django REST Framework project, pagination is added by default. For APIs restricted to superusers using the `IsSuperUser` permission class, additional rate limiting or pagination adjustments are not required.
📚 Learning: 2024-11-28T06:16:31.373Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2620
File: care/facility/models/facility.py:306-311
Timestamp: 2024-11-28T06:16:31.373Z
Learning: In the 'care' project, moving imports of models like `Asset`, `AssetLocation`, `FacilityDefaultAssetLocation`, and `PatientSample` to the module level in `care/facility/models/facility.py` causes circular imports. Therefore, it's acceptable to keep these imports inside the `delete` method of the `Facility` class.
Applied to files:
care/emr/resources/facility_flag/spec.pycare/security/authorization/__init__.pycare/emr/tests/test_facility_flag_api.py
📚 Learning: 2024-11-26T05:41:40.226Z
Learnt from: aravindm4
Repo: ohcnetwork/care PR: 2585
File: care/facility/api/viewsets/facility_flag.py:14-27
Timestamp: 2024-11-26T05:41:40.226Z
Learning: In the `care/facility/api/viewsets/facility_flag.py` file and similar viewsets in this Django REST Framework project, pagination is added by default. For APIs restricted to superusers using the `IsSuperUser` permission class, additional rate limiting or pagination adjustments are not required.
Applied to files:
care/emr/tests/test_user_flag_api.pyconfig/api_router.pycare/emr/api/viewsets/user_flag.pycare/emr/tests/test_facility_flag_api.pycare/emr/api/viewsets/facility_flag.pycare/security/authorization/facility_flag.py
📚 Learning: 2024-12-02T09:16:55.978Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2585
File: care/users/api/viewsets/user_flag.py:0-0
Timestamp: 2024-12-02T09:16:55.978Z
Learning: In the `UserFlagViewSet`'s `list_available_flags` method in `care/users/api/viewsets/user_flag.py`, adding extra error handling is not necessary, and returning error messages directly is not preferred.
Applied to files:
care/emr/tests/test_user_flag_api.pyconfig/api_router.pycare/emr/api/viewsets/user_flag.pycare/emr/tests/test_facility_flag_api.pycare/emr/api/viewsets/facility_flag.py
📚 Learning: 2024-12-02T09:14:53.161Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2585
File: care/users/api/viewsets/user_flag.py:16-30
Timestamp: 2024-12-02T09:14:53.161Z
Learning: In the `UserFlagViewSet` class in `care/users/api/viewsets/user_flag.py`, we don't need to add configurations for `ordering_fields`, `pagination_class`, or `throttle_classes` because default values are already set globally in our Django REST Framework settings.
Applied to files:
care/emr/tests/test_user_flag_api.pyconfig/api_router.pycare/emr/resources/user_flag/spec.pycare/emr/api/viewsets/user_flag.pycare/emr/api/viewsets/facility_flag.py
🧬 Code graph analysis (9)
care/emr/resources/facility_flag/spec.py (6)
care/emr/resources/base.py (3)
EMRResource(13-168)is_update(47-48)to_json(158-159)care/emr/resources/facility/spec.py (1)
FacilityBareMinimumSpec(27-31)care/facility/models/facility_flag.py (1)
FacilityFlag(11-40)care/utils/registries/feature_flag.py (2)
FlagRegistry(22-68)FlagType(13-15)care/utils/shortcuts.py (1)
get_object_or_404(6-15)care/emr/resources/user_flag/spec.py (4)
validate_flag_name(23-25)validate_flag_name(35-37)perform_extra_deserialization(27-29)perform_extra_serialization(44-46)
care/emr/tests/test_user_flag_api.py (4)
care/users/models.py (1)
UserFlag(223-250)care/utils/registries/feature_flag.py (2)
FlagRegistry(22-68)FlagType(13-15)care/utils/tests/base.py (2)
create_super_user(36-39)create_user(25-28)care/utils/models/base.py (1)
delete(30-32)
config/api_router.py (2)
care/emr/api/viewsets/facility_flag.py (1)
FacilityFlagViewSet(31-101)care/emr/api/viewsets/user_flag.py (1)
UserFlagViewSet(31-84)
care/emr/resources/user_flag/spec.py (5)
care/emr/resources/base.py (2)
EMRResource(13-168)is_update(47-48)care/emr/resources/user/spec.py (1)
UserSpec(124-138)care/users/models.py (2)
User(96-212)UserFlag(223-250)care/utils/registries/feature_flag.py (2)
FlagRegistry(22-68)FlagType(13-15)care/utils/shortcuts.py (1)
get_object_or_404(6-15)
care/emr/api/viewsets/user_flag.py (6)
care/emr/api/viewsets/base.py (5)
EMRCreateMixin(91-151)EMRDestroyMixin(246-262)EMRListMixin(154-173)EMRRetrieveMixin(73-88)EMRUpdateMixin(176-243)care/emr/resources/user_flag/spec.py (4)
UserFlagCreateSpec(18-29)UserFlagReadSpec(40-46)UserFlagRetrieveSpec(49-50)UserFlagUpdateSpec(32-37)care/security/authorization/base.py (2)
AuthorizationController(72-113)call(97-108)care/users/models.py (1)
UserFlag(223-250)care/utils/registries/feature_flag.py (3)
FlagNotFoundError(9-10)FlagRegistry(22-68)FlagType(13-15)care/emr/api/viewsets/facility_flag.py (7)
authorize_create(47-53)authorize_update(55-61)authorize_destroy(63-69)get_queryset(71-76)perform_create(78-81)perform_destroy(83-86)available_flags(89-101)
care/emr/tests/test_facility_flag_api.py (3)
care/facility/models/facility_flag.py (1)
FacilityFlag(11-40)care/utils/registries/feature_flag.py (2)
FlagRegistry(22-68)FlagType(13-15)care/utils/models/base.py (1)
delete(30-32)
care/emr/api/viewsets/facility_flag.py (5)
care/emr/api/viewsets/base.py (4)
EMRCreateMixin(91-151)EMRDestroyMixin(246-262)EMRListMixin(154-173)EMRRetrieveMixin(73-88)care/facility/models/facility_flag.py (1)
FacilityFlag(11-40)care/security/authorization/base.py (2)
AuthorizationController(72-113)call(97-108)care/utils/registries/feature_flag.py (3)
FlagNotFoundError(9-10)FlagRegistry(22-68)FlagType(13-15)care/emr/api/viewsets/user_flag.py (4)
get_queryset(59-62)perform_create(64-67)perform_destroy(69-72)available_flags(75-84)
care/security/authorization/user_flag.py (1)
care/security/authorization/base.py (3)
AuthorizationController(72-113)AuthorizationHandler(11-69)register_internal_controller(111-113)
care/security/authorization/facility_flag.py (1)
care/security/authorization/base.py (3)
AuthorizationController(72-113)AuthorizationHandler(11-69)register_internal_controller(111-113)
🪛 Ruff (0.14.11)
care/emr/resources/facility_flag/spec.py
13-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/resources/user_flag/spec.py
12-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/api/viewsets/user_flag.py
45-45: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
47-47: Unused method argument: instance
(ARG002)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Unused method argument: request_obj
(ARG002)
51-51: Unused method argument: model_instance
(ARG002)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Unused method argument: instance
(ARG002)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Unused method argument: request
(ARG002)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
care/emr/api/viewsets/facility_flag.py
45-45: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
47-47: Unused method argument: instance
(ARG002)
51-53: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Unused method argument: request_obj
(ARG002)
55-55: Unused method argument: model_instance
(ARG002)
59-61: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Unused method argument: instance
(ARG002)
67-69: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Unused method argument: request
(ARG002)
93-93: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (39)
care/security/authorization/facility_flag.py (1)
5-21: LGTM!The
FacilityFlagAccesshandler correctly restricts read and write operations to superusers and follows the established pattern for authorization handlers in this codebase. The registration withAuthorizationControlleris properly placed at module level.care/security/authorization/user_flag.py (1)
5-21: LGTM!Mirrors the
FacilityFlagAccessimplementation appropriately. Superuser-only access is correctly enforced for both read and write operations.care/security/authorization/__init__.py (1)
10-10: LGTM!The new authorization modules are correctly re-exported, maintaining alphabetical order and consistency with the existing import pattern.
Also applies to: 35-35
config/api_router.py (1)
117-120: LGTM!The new routes are properly registered with appropriate viewsets. The endpoint naming follows the existing conventions in the router.
Based on learnings, pagination is added by default and no additional rate limiting is needed for superuser-restricted APIs.
care/emr/tests/test_user_flag_api.py (7)
27-50: LGTM!Solid authorization coverage for list operations. The comment on line 49 explaining the 403 for unauthenticated access is a helpful clarification.
52-81: LGTM!Filtering tests are well-structured and cover both user-based and case-insensitive flag name filtering.
85-142: LGTM!Create tests appropriately cover authorization, validation errors, and missing required fields. The 200 status code for successful creation aligns with the EMRCreateMixin behavior in this project.
146-181: LGTM!Retrieve tests have good coverage for authorization, non-existent records, and soft-deleted flags.
185-225: LGTM!Update and partial update tests correctly verify both PUT and PATCH behavior with appropriate authorization checks.
229-272: LGTM!Delete tests comprehensively cover soft delete behavior, authorization, and registry unregistration. The
test_delete_unregisters_flagtest properly verifies the flag lifecycle.
276-294: LGTM!Available flags endpoint tests and the helper method are clean and appropriate.
care/emr/tests/test_facility_flag_api.py (8)
1-27: Test setup looks well-organized.The test setup properly registers test flags, creates users with different permission levels, and sets up facilities. Good use of
CareAPITestBasefor consistent test scaffolding.
29-68: List tests provide solid coverage.Tests cover superuser access, facility filtering, normal user denial, and unauthenticated access. The comment on line 67 helpfully documents the authorization behavior.
70-98: Filter tests are appropriately thorough.Case-insensitive flag filtering and multiple flags per facility scenarios are well covered.
100-194: Create tests cover the key scenarios.Good coverage of permission checks, validation errors, and the same-flag-different-facilities case.
196-239: Retrieve tests are solid.Tests for superuser access, normal user denial, non-existent flag, and soft-deleted flag are all present.
241-289: Update tests look good.Full update (PUT) and partial update (PATCH) scenarios are covered with appropriate permission checks.
291-327: Delete tests are comprehensive.Soft delete verification, permission checks, and already-deleted scenarios are well covered.
344-359: Available flags tests provide adequate coverage.Both superuser access and normal user denial are tested.
care/emr/resources/facility_flag/spec.py (5)
1-8: Imports are clean and well-organized.All necessary dependencies for the specs are properly imported.
11-16: Base spec follows established patterns.The
__exclude__list pattern is consistent with other EMRResource subclasses in the codebase. The static analysis hint aboutClassVaris a false positive given the EMRResource metaclass design.
19-30: Create spec properly validates and deserializes the facility relationship.Good use of
field_validatorfor flag registration validation andperform_extra_deserializationfor resolving the facility UUID.
33-38: Update spec correctly revalidates the flag name.This ensures that even on updates, the flag must be registered in the
FlagRegistry.
41-50: Read and Retrieve specs handle serialization appropriately.The serialization correctly maps
external_idtoidand serializes the facility usingFacilityBareMinimumSpec.care/emr/api/viewsets/user_flag.py (5)
1-23: Imports and dependencies are properly organized.All necessary components for the viewset are imported.
26-28: Filter class is well-defined.UUID filter on
user__external_idand case-insensitive flag filter follow good practices.
31-45: ViewSet class attributes are properly configured.The mixin order and model/spec assignments are correct. The static analysis hint about
ClassVarforfilter_backendsis a false positive - this pattern is standard in DRF viewsets. Based on learnings, default pagination/throttling is already configured globally, so no additional configuration is needed.
47-62: Authorization methods follow a consistent pattern.The unused argument warnings from static analysis are false positives - these methods override parent class hooks with fixed signatures. The authorization pattern using
AuthorizationController.callis consistent with the codebase.
74-84: Available flags endpoint implementation is correct.Based on retrieved learnings, extra error handling in
available_flagsis not necessary. TheFlagNotFoundErrorcatch returning a 400 is appropriate.care/emr/resources/user_flag/spec.py (5)
1-7: Imports are appropriate for the spec definitions.All necessary dependencies are included.
10-15: Base spec is consistent with FacilityFlagBaseSpec.The pattern matches the facility flag implementation, which is good for maintainability.
18-29: Create spec properly validates and resolves the user relationship.The validator and deserialization hook follow the established pattern.
32-37: Update spec correctly revalidates the flag name.Consistent with the facility flag update spec.
40-50: Read and Retrieve specs handle serialization correctly.Uses
UserSpec.serialize()for the nested user object, which provides appropriate user data in responses.care/emr/api/viewsets/facility_flag.py (5)
1-23: Imports are well-organized and complete.All necessary dependencies for the viewset are properly imported.
26-28: Filter class mirrors the UserFlagFilters pattern.Consistent filtering approach across both flag viewsets.
31-45: ViewSet configuration is correct.Class attributes are properly set. Per retrieved learnings, pagination is added by default for superuser-restricted APIs, so no additional configuration is needed.
47-76: Authorization methods are consistent with the user flag viewset.The static analysis warnings about unused arguments are false positives - these methods override parent class hooks. The permission denied messages are descriptive and helpful for debugging authorization issues.
88-101: Available flags endpoint is correctly implemented.Follows the same pattern as
UserFlagViewSet.available_flags. The error message on line 100 correctly uses 'facility' instead of 'user'.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3460 +/- ##
===========================================
+ Coverage 74.48% 74.68% +0.19%
===========================================
Files 471 477 +6
Lines 21667 21868 +201
Branches 2257 2271 +14
===========================================
+ Hits 16139 16332 +193
- Misses 5027 5029 +2
- Partials 501 507 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposed Changes
Associated Issue
Merge Checklist
Only PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.