fix:added facility min spec in TagConfigReadSpec

[nandkishorr]

Proposed Changes

• Added facility minimum spec in TagConfigReadSpec
• Fixed related circular import issues

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

  • Tag configurations now include associated facility details when present.
  • Facility records display creator information in API responses.

• Refactor

  • Audit user serialization for organizations and facilities was consolidated to a single, consistent approach to improve consistency and maintainability.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

UserSpec imports were moved from module-level to local scope in facility and organization spec methods; facility serialization now uses a cached model lookup. TagConfigReadSpec gains an optional facility field and serializes it via FacilityBareMinimumSpec.

Changes

Cohort / File(s)	Summary
Import & audit serialization refactor care/emr/resources/facility/spec.py, care/emr/resources/organization/spec.py	Removed top-level UserSpec imports; added local imports inside perform_extra_serialization / perform_extra_user_serialization. Facility created_by now serialized via model_from_cache(UserSpec, id=...); organization audit user serialization consolidated to serialize_audit_users.
Tag config — facility addition care/emr/resources/tag/config_spec.py	Added optional facility: dict | None = None to TagConfigReadSpec; imported FacilityBareMinimumSpec and populate facility during serialization using FacilityBareMinimumSpec.serialize(...).to_json().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
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 (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title partially captures the main change (adding facility spec to TagConfigReadSpec), but omits the significant effort of fixing circular import issues across multiple files, making it incomplete.
Description check	✅ Passed	The description covers the proposed changes and includes the merge checklist, but the 'Associated Issue' section is missing and the description lacks detail about the circular import fixes and their scope.

✨ 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 nandkishorr/fix/tagconfig_spec

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.47%. Comparing base (108fe9e) to head (1499469).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3437      +/-   ##
===========================================
+ Coverage    74.46%   74.47%   +0.01%     
===========================================
  Files          449      449              
  Lines        20699    20700       +1     
  Branches      2152     2151       -1     
===========================================
+ Hits         15413    15416       +3     
  Misses        4822     4822              
+ Partials       464      462       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

108-114: Consider using the centralized serialize_audit_users helper for consistency.

While the current implementation works, OrganizationRetrieveSpec (line 73 in care/emr/resources/organization/spec.py) uses the centralized cls.serialize_audit_users(mapping, obj) method, which handles both created_by and updated_by fields. Using the same pattern here would improve consistency and reduce duplication.

Additionally, the condition if obj.created_by: on line 113 checks the related object, potentially triggering a database query. The centralized method checks obj.created_by_id instead, which is more efficient.

🔎 Proposed refactor to use centralized helper

     @classmethod
     def perform_extra_serialization(cls, mapping, obj):
-        from care.emr.resources.user.spec import UserSpec
-
         mapping["id"] = obj.external_id
         mapping["read_cover_image_url"] = obj.read_cover_image_url()
-        if obj.created_by:
-            mapping["created_by"] = model_from_cache(UserSpec, id=obj.created_by_id)
+        cls.serialize_audit_users(mapping, obj)
         mapping["facility_type"] = REVERSE_FACILITY_TYPES[obj.facility_type]
         if obj.geo_organization:
             mapping["geo_organization"] = OrganizationReadSpec.serialize(

Note: If FacilityReadSpec intentionally excludes updated_by, you'll need to add that field to the spec definition as well (similar to how created_by is defined on line 104).

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9415de8 and 1499469.

📒 Files selected for processing (2)

• care/emr/resources/facility/spec.py
• care/emr/resources/organization/spec.py

🧰 Additional context used

📓 Path-based instructions (1)

**/*.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/spec.py
• care/emr/resources/organization/spec.py

🧠 Learnings (2)

📓 Common learnings

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.

📚 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/spec.py

🧬 Code graph analysis (2)

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

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

• EMRResource (13-168)
• cacheable (279-305)
• model_from_cache (232-275)

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

• UserSpec (124-138)

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

• read_cover_image_url (221-226)

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

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

• serialize_audit_users (162-168)

⏰ 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 (3)

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

10-10: LGTM: Import addition supports caching.

The model_from_cache import is correctly added to support the new cached serialization pattern.

---

109-109: Local import correctly addresses circular dependency.

Moving the UserSpec import to method scope is the appropriate pattern for avoiding circular imports in this codebase.

Based on learnings, circular import prevention through local imports is an accepted practice in this project.

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

73-73: Excellent refactor: centralized audit serialization.

This change correctly addresses the previous review feedback and uses the centralized serialize_audit_users helper method. This approach is more maintainable and follows the DRY principle.

As per past review comments, this is the recommended pattern for handling audit fields.