User and facility flag viewset #3044

praffq · 2025-05-15T18:03:54Z

Proposed Changes

Added viewset for facility and user flag , only accessible to super user

Associated Issue

fixes - CRUD Endpoints for Facility and User flags roadmap#96

Architecture changes

Remove this section if not used

Merge Checklist

Tests added/fixed
Update docs in /docs
Linting Complete
Any other necessary step

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

New Features
- Added facility- and user-scoped feature flag management with CRUD APIs and endpoints to list available flags; restricted to superusers.
- Flags are registered/unregistered centrally and include cache-backed lookups for faster reads.
Bug Fixes
- Validation prevents creating duplicate flags for the same facility or user; safe cleanup unregisters unused flags after deletions.
Chores
- Consolidated flag models and logic into the EMR module and updated model references and migrations for consistency.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-05-15T18:04:02Z

📝 Walkthrough

Walkthrough

Moved Feature Flag models and logic into the emr app: added FacilityFlag and UserFlag models, a BaseFlag scaffold with caching, new viewsets and Pydantic specs for flag management, router registrations, migrations deleting old models, and adjusted imports referencing the new locations.

Changes

Cohort / File(s)	Change Summary
API viewsets `care/emr/api/viewsets/facility.py`, `care/emr/api/viewsets/user.py`	Added `FacilityFlagViewSet` and `UserFlagViewSet`, filters, superuser-only permission checks, create/destroy hooks to register/unregister flags with `FlagRegistry`, cache invalidation, and `available-flags` list endpoints.
Models — emr app `care/emr/models/base.py`, `care/emr/models/user.py`, `care/emr/models/facility.py`, `care/emr/models/file_upload.py`, `care/emr/models/patient.py`, `care/emr/models/scheduling/booking.py`	Introduced abstract `BaseFlag` with caching and helper methods; added `UserFlag` model; adjusted `FacilityFlag` internals (import/constraint/name) and switched several `User` FK references to string `"users.User"`.
Migrations — emr app `care/emr/migrations/0030_facilityflag_userflag.py`	Added migration creating `FacilityFlag` and `UserFlag` models in `emr`.
Resources / specs `care/emr/resources/facility/spec.py`, `care/emr/resources/user/spec.py`	Added Pydantic specs: `FacilityFlagBase/Create/Read` and `UserFlagBase/Create/Read` with validation, deserialization, and serialization logic.
Removed from old apps / deletions `care/facility/migrations/0477_delete_facilityflag.py`, `care/users/migrations/0024_delete_userflag.py`, `care/facility/models/__init__.py`, `care/facility/models/facility.py`, `care/users/models.py`, `care/users/admin.py`	Deleted old `FacilityFlag` and `UserFlag` models from `facility`/`users` apps, removed auto-import of `facility_flag`, updated imports to point to `emr` locations.
Removed legacy base `care/utils/models/base.py`	Removed previous `BaseFlag` implementation and related flag caching/constants.
Router `config/api_router.py`	Registered `facility_flags` and `user_flags` viewsets and added imports.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas needing extra attention:
- Transactional create/destroy hooks that interact with FlagRegistry and post-commit unregister logic.
- Cache key templates and TTL usages in new BaseFlag vs. removed legacy implementation.
- Migrations: ensure model movement/drops align with deployment order to avoid downtime.
- Specs’ validation/deserialization paths that resolve related objects by external_id/UUID.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description follows the template with proposed changes and associated issue, but critical merge checklist items remain unchecked despite the repository requirement that tests and linting must pass.	Check the boxes for completed items or confirm that tests have been added/fixed and linting has passed before merge; update docs as needed.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the primary change: introduction of viewsets for managing user and facility flags with superuser-only access.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch prafful/feat/user-and-facility-flag

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 5

🧹 Nitpick comments (9)

care/users/migrations/0024_delete_userflag.py (1)
1-16: Migration looks good but could use quotes consistency

The migration correctly removes the UserFlag model from the users app as part of the EMR app refactoring. I suppose someone could have made the migration use double quotes instead of single quotes to match the project style, but I guess it works fine as is.
-        ('users', '0023_user_prefix_user_suffix'),
+        ("users", "0023_user_prefix_user_suffix"),
-            name='UserFlag',
+            name="UserFlag",
🧰 Tools

🪛 Ruff (0.8.2)

9-9: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

9-9: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

14-14: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)
config/api_router.py (1)
278-280: Added router registrations for flag viewsets

The new router registrations for facility_flags and user_flags endpoints are properly configured. The trailing empty line maintains the code structure formatting.

It would be helpful to have a comment indicating that these endpoints are restricted to superusers, since that's an important security detail mentioned in the PR objectives.
 router.register(r"facility_flags", FacilityFlagViewSet, basename="facility-flags")
 router.register(r"user_flags", UserFlagViewSet, basename="user-flags")
+# Note: Both flag endpoints above are restricted to superusers only
care/emr/models/user.py (3)
23-28: Consider adding a more descriptive name for the UniqueConstraint

While the constraint itself is correct, the constraint name "emr_unique_user_flag" could be more descriptive to better indicate what uniqueness is being enforced. Maybe something like "emr_user_flag_unique_combination" would be slightly clearer for future developers.
models.UniqueConstraint(
    fields=["user", "flag"],
    condition=models.Q(deleted=False),
-   name="emr_unique_user_flag",
+   name="emr_user_flag_unique_combination",
)
30-33: Consider adding docstring to the check_user_has_flag method

The method signature is clear, but adding a brief docstring would improve code readability and provide context for future developers about the method's purpose and expected behavior.
@classmethod
def check_user_has_flag(cls, user_id: int, flag_name: FlagName) -> bool:
+   """
+   Check if a user has a specific flag.
+   
+   Args:
+       user_id: The ID of the user to check
+       flag_name: The name of the flag to check for
+       
+   Returns:
+       True if the user has the flag, False otherwise
+   """
    return cls.check_entity_has_flag(user_id, flag_name)
35-36: Consider adding docstring to the get_all_flags method

Similar to the previous method, adding a brief docstring would improve code readability and documentation.
@classmethod
def get_all_flags(cls, user_id: int) -> tuple[FlagName]:
+   """
+   Get all flags associated with a user.
+   
+   Args:
+       user_id: The ID of the user to retrieve flags for
+       
+   Returns:
+       A tuple of FlagName values associated with the user
+   """
    return super().get_all_flags(user_id)
care/emr/models/base.py (1)

64-77: check_entity_has_flag will cache negative look-ups forever

With cache.get_or_set the lambda is evaluated once and its result (True/False) is stored for a whole day.
If a flag is subsequently added, consumers calling check_entity_has_flag will still receive the old False value until the TTL expires.

Consider:

Bumping the TTL down, or

Explicitly invalidating this key in the concrete model’s save/delete methods (you already clear it there – good), and

Adding a short comment explaining this expiry strategy.

care/emr/resources/facility/spec.py (1)

76-79: Small nomenclature slip

__exclude__ = ["facility"] removes the FK from the serialized output, yet FacilityFlagReadSpec manually re-injects a nested facility dict. That is fine, but a brief comment here would save the next reader a head-scratch.
care/emr/migrations/0030_facilityflag_userflag.py (1)
28-36: Consider adding an index on flag for faster look-ups.

Both FacilityFlag and UserFlag are queried by flag (see the unique constraint and the new viewsets’ filters).
Adding db_index=True on flag will prevent full-table scans once the tables grow.
- ('flag', models.CharField(max_length=1024)),
+ ('flag', models.CharField(max_length=1024, db_index=True)),
A one-line migration change now will save you a painful data-migration later.

🧰 Tools

🪛 Ruff (0.8.2)

28-28: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

29-29: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

29-29: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

30-30: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

30-30: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

31-31: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

31-31: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

34-34: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

34-34: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

35-35: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

35-35: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

35-35: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

35-35: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)

35-35: Single quotes found but double quotes preferred

Replace single quotes with double quotes

(Q000)
care/emr/resources/user/spec.py (1)
202-205: id should be serialised as a string for consistency.

Elsewhere (UserRetrieveSpec, etc.) external_id is stringified; returning a raw UUID here produces mixed types in API responses.
-        mapping["id"] = obj.external_id
+        mapping["id"] = str(obj.external_id)
A tiny tweak that spares consumers an awkward type dance.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fa63f2 and 323930a.

📒 Files selected for processing (20)

care/emr/api/viewsets/facility.py (3 hunks)
care/emr/api/viewsets/user.py (3 hunks)
care/emr/migrations/0030_facilityflag_userflag.py (1 hunks)
care/emr/models/base.py (2 hunks)
care/emr/models/facility.py (2 hunks)
care/emr/models/file_upload.py (1 hunks)
care/emr/models/patient.py (1 hunks)
care/emr/models/scheduling/booking.py (1 hunks)
care/emr/models/user.py (1 hunks)
care/emr/resources/facility/spec.py (3 hunks)
care/emr/resources/user/spec.py (3 hunks)
care/facility/migrations/0477_delete_facilityflag.py (1 hunks)
care/facility/models/__init__.py (0 hunks)
care/facility/models/facility.py (1 hunks)
care/users/admin.py (1 hunks)
care/users/migrations/0024_delete_userflag.py (1 hunks)
care/users/models.py (1 hunks)
care/utils/models/base.py (0 hunks)
care/utils/registries/feature_flag.py (1 hunks)
config/api_router.py (3 hunks)

💤 Files with no reviewable changes (2)

care/facility/models/init.py
care/utils/models/base.py

🧰 Additional context used

🧠 Learnings (2)

care/facility/models/facility.py (1)

Learnt from: DraKen0009
PR: ohcnetwork/care#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.

care/emr/api/viewsets/facility.py (1)

Learnt from: aravindm4
PR: ohcnetwork/care#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.

🧬 Code Graph Analysis (7)

care/facility/models/facility.py (1)

care/emr/models/facility.py (1)

FacilityFlag (7-36)

care/facility/migrations/0477_delete_facilityflag.py (2)

care/emr/migrations/0030_facilityflag_userflag.py (1)

Migration (9-58)

care/users/migrations/0024_delete_userflag.py (1)

Migration (6-16)

care/users/models.py (2)

care/emr/models/user.py (1)

UserFlag (7-36)

care/utils/models/base.py (1)

BaseModel (12-29)

care/users/admin.py (1)

care/emr/models/user.py (1)

UserFlag (7-36)

care/emr/models/facility.py (1)

care/emr/models/base.py (1)

BaseFlag (35-89)

care/emr/resources/facility/spec.py (6)

care/emr/models/facility.py (2)

FacilityFlag (7-36)

check_facility_has_flag (31-32)

care/utils/registries/feature_flag.py (1)

FlagNotFoundError (9-10)

care/emr/resources/base.py (3)

EMRResource (10-152)

is_update (41-42)

to_json (142-143)

care/emr/resources/user/spec.py (8)

validate_flag (184-190)

perform_extra_deserialization (66-70)

perform_extra_deserialization (118-119)

perform_extra_deserialization (192-194)

perform_extra_serialization (133-136)

perform_extra_serialization (146-156)

perform_extra_serialization (167-169)

perform_extra_serialization (202-205)

care/emr/resources/organization/spec.py (2)

perform_extra_deserialization (45-50)

perform_extra_serialization (60-62)

care/facility/models/facility.py (1)

Facility (233-409)

care/emr/migrations/0030_facilityflag_userflag.py (2)

care/facility/migrations/0477_delete_facilityflag.py (1)

Migration (6-16)

care/users/migrations/0024_delete_userflag.py (1)

Migration (6-16)

🪛 Ruff (0.8.2)

care/users/migrations/0024_delete_userflag.py

9-9: Single quotes found but double quotes preferred