Add Devices Spec #2815
Add Devices Spec #2815
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new file for device models and histories, defining a main Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DeviceTypeBase
participant DeviceTypeRegistry
User ->> DeviceTypeBase: handle_create(request_data, obj)
DeviceTypeBase ->> DeviceTypeRegistry: register(device_type, device_class)
DeviceTypeRegistry -->> DeviceTypeBase: registration confirmation
DeviceTypeBase -->> User: Response with created device details
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
care/emr/models/device.py (3)
10-11: Perhaps defining choices for 'status' and 'availability_status' would enhance data integrity.By specifying choices for these fields, you can restrict the allowed values and prevent invalid data entries.
25-34: Adding 'related_name' to your ForeignKey fields might improve reverse query handling.Specifying the
related_nameattribute can make reverse lookups more intuitive and your codebase easier to navigate.
55-60: Clarifying the structure of 'edit_history' could be beneficial.Defining a schema for the
edit_historyJSONField may help maintain consistency and improve data management.care/emr/registries/device_type/device_registry.py (2)
2-33: Consider enhancing the docstrings for better clarity.Improving the explanations in your docstrings might make the codebase more approachable for others.
36-44: Is 'InternalQuestionnaireRegistry' the most appropriate class name here?Renaming it to something like 'InternalDeviceRegistry' could more accurately reflect its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/models/device.py(1 hunks)care/emr/registries/device_type/device_registry.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
care/emr/models/device.py (1)
37-37: Verify that using 'default=list' for 'facility_organization_cache' is appropriate.Ensure that this default value aligns with your intended data handling for the
ArrayField.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
care/emr/migrations/0017_device_deviceencounterhistory_devicelocationhistory_and_more.py (1)
22-49: Consider replacing single quotes with double quotes for consistencyThe static analysis tools indicate that single quotes are used throughout the model definitions, whereas the project's code style guidelines prefer double quotes. Updating the quotes enhances readability and maintains consistency across the codebase.
Here are some examples of the necessary changes:
-('status', models.CharField(max_length=14)), +("status", models.CharField(max_length=14)), -('manufacturer', models.CharField(max_length=1024)), +("manufacturer", models.CharField(max_length=1024)), -('device', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='emr.device')), +("device", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="emr.device")), -('note', models.TextField(blank=True, default='', null=True)), +("note", models.TextField(blank=True, default="", null=True)),Consider adjusting the code generation settings or running a code formatter to apply these changes throughout the file.
Also applies to: 58-70, 79-91, 100-112
🧰 Tools
🪛 Ruff (0.8.2)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
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)
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)
32-32: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
33-33: 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)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
38-38: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
49-49: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
49-49: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/registries/device_type/device_registry.py (1)
25-26: Reconsider accepting N+1 queries in thelistmethodThe docstring mentions that N+1 queries are acceptable, but this can lead to performance issues as the data grows. It might be better to avoid N+1 queries altogether.
Consider optimizing the method to prevent unnecessary database hits, possibly by using
select_relatedorprefetch_related.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/migrations/0017_device_deviceencounterhistory_devicelocationhistory_and_more.py(1 hunks)care/emr/models/__init__.py(1 hunks)care/emr/registries/device_type/device_registry.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- care/emr/models/init.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0017_device_deviceencounterhistory_devicelocationhistory_and_more.py
13-13: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
13-13: 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)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
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)
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)
32-32: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
33-33: 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)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
38-38: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
49-49: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
49-49: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
52-52: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
56-56: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
60-60: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
62-62: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
65-65: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
66-66: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
67-67: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
67-67: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
68-68: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
68-68: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
69-69: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
69-69: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
70-70: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
70-70: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
73-73: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
77-77: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
79-79: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
79-79: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
80-80: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
81-81: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
82-82: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
83-83: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
84-84: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
85-85: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
86-86: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
87-87: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
89-89: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
89-89: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
90-90: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
90-90: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
91-91: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
91-91: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
94-94: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
98-98: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
100-100: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
100-100: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
101-101: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
102-102: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
103-103: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
104-104: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
105-105: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
106-106: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
107-107: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
108-108: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
108-108: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
109-109: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
110-110: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
110-110: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
111-111: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
111-111: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
112-112: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
112-112: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
115-115: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
| _device_types = {} | ||
|
|
||
| @classmethod | ||
| def register(cls, device_type, device_class) -> None: | ||
| if not issubclass(device_class, DeviceTypeBase): | ||
| raise ValueError("The provided class is not a subclass of DeviceTypeBase") | ||
| cls._device_types[device_type] = device_class |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential thread safety concerns with mutable class variables
Modifying a class-level mutable variable like _device_types without synchronization can lead to race conditions in a multi-threaded environment. It might be prudent to implement thread safety measures when registering device types.
You can enhance thread safety by using a threading lock:
+import threading
class DeviceTypeRegistry:
+ _lock = threading.Lock()
_device_types = {}
@classmethod
def register(cls, device_type, device_class) -> None:
if not issubclass(device_class, DeviceTypeBase):
raise ValueError("The provided class is not a subclass of DeviceTypeBase")
+ with cls._lock:
cls._device_types[device_type] = device_class
There was a problem hiding this comment.
@coderabbitai can you give examples of multithreaded use cases
There was a problem hiding this comment.
@sainak In this context, thread safety might actually be unnecessary. Here's why:
- Device type registration typically happens during Django's application startup (import time) which is single-threaded
- After startup, the registry is usually read-only, with device types being accessed but not modified during request handling
If you do need runtime registration (which would be unusual), a better pattern would be:
class DeviceTypeRegistry:
def __init__(self):
self._device_types = {}
@classmethod
def get_instance(cls):
if not hasattr(cls, '_instance'):
cls._instance = cls()
return cls._instance
def register(self, device_type, device_class):
if not issubclass(device_class, DeviceTypeBase):
raise ValueError("The provided class is not a subclass of DeviceTypeBase")
self._device_types[device_type] = device_classBut I'd be curious to know if you have any use cases where you need to register device types after the application has started?
✏️ Learnings added
Learnt from: sainak
PR: ohcnetwork/care#2815
File: care/emr/registries/device_type/device_registry.py:37-43
Timestamp: 2025-02-06T14:33:29.825Z
Learning: In Django applications, device type registration in DeviceTypeRegistry typically occurs during application startup (import time) and remains read-only afterwards. Thread safety for the registry is usually unnecessary unless there are specific runtime registration requirements.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2815 +/- ##
===========================================
+ Coverage 56.27% 56.50% +0.23%
===========================================
Files 215 221 +6
Lines 10202 10516 +314
Branches 1039 1069 +30
===========================================
+ Hits 5741 5942 +201
- Misses 4445 4558 +113
Partials 16 16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
care/emr/registries/device_type/device_registry.py (3)
2-7: Perhaps we could add some actual validation here?The
handle_createmethod seems a bit... minimalist. While returning the object unmodified is technically valid, it might be nice to add some basic validation or at least a TODO comment if validation is planned for the future.def handle_create(self, request_data, obj): """ Handle Creates, the original source request along with the base object created is passed along. Update the obj as needed and create any extra metadata needed. This method is called within a transaction + + Args: + request_data: The original request data + obj: The base object to be created + + Returns: + The validated and potentially modified object + + Raises: + ValidationError: If the request data is invalid """ + # TODO: Add validation logic here return obj
23-27: Oh, so we're just casually accepting N+1 queries?While the docstring acknowledges that N+1 queries are "okay", it would be lovely if we could provide some guidance on implementing the recommended caching strategy.
def list(self, obj): """ Return Extra metadata for the given obj for lists, N+1 queries is okay, caching is recommended for performance + + Example caching implementation: + @cached_property + def cached_metadata(self): + return self.compute_expensive_metadata() """ return {}
43-50: Missing some rather useful methods, aren't we?While the registration mechanism is fine, it might be helpful to add methods for retrieving registered device types and checking if a device type exists.
class DeviceTypeRegistry: _device_types = {} @classmethod def register(cls, device_type, device_class) -> None: if not issubclass(device_class, DeviceTypeBase): raise ValueError("The provided class is not a subclass of DeviceTypeBase") cls._device_types[device_type] = device_class + + @classmethod + def get_device_class(cls, device_type): + """Get the registered device class for a given device type.""" + return cls._device_types.get(device_type) + + @classmethod + def is_registered(cls, device_type): + """Check if a device type is registered.""" + return device_type in cls._device_types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/models/__init__.py(1 hunks)care/emr/models/device.py(1 hunks)care/emr/registries/device_type/device_registry.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- care/emr/models/init.py
- care/emr/models/device.py
🧰 Additional context used
📓 Learnings (1)
care/emr/registries/device_type/device_registry.py (1)
Learnt from: sainak
PR: ohcnetwork/care#2815
File: care/emr/registries/device_type/device_registry.py:37-43
Timestamp: 2025-02-06T14:33:29.977Z
Learning: In Django applications, device type registration in DeviceTypeRegistry typically occurs during application startup (import time) and remains read-only afterwards. Thread safety for the registry is usually unnecessary unless there are specific runtime registration requirements.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test / Test
🔇 Additional comments (1)
care/emr/registries/device_type/device_registry.py (1)
46-50: Regarding the thread safety discussion...Based on the retrieved learning, thread safety is unnecessary here since device registration occurs during Django's startup. I'll retract the previous thread safety suggestion.
| def perform_action(self, obj, action, request): | ||
| """ | ||
| Perform some kind of action on an asset, the HTTP request is proxied through as is. | ||
| an HTTP response object is expected as the return. | ||
| """ | ||
| return # Return an HTTP Response |
There was a problem hiding this comment.
A None response? How... interesting.
The perform_action method returns None, which might not be the most helpful HTTP response. Consider returning a proper HTTP response object or raising a NotImplementedError.
def perform_action(self, obj, action, request):
"""
Perform some kind of action on an asset, the HTTP request is proxied through as is.
an HTTP response object is expected as the return.
"""
- return # Return an HTTP Response
+ raise NotImplementedError("Subclasses must implement perform_action()")
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
care/emr/resources/common/contact_point.py (4)
1-5: I suppose we're being extra generous with blank lines today.The imports are clean and necessary, though I can't help but notice we have an extra blank line that's just... sitting there.
from enum import Enum from pydantic import BaseModel -
6-14: Oh, I see we're keeping the documentation as a surprise for later.The enum implementation is correct, but it would be wonderful if we could add a docstring explaining the purpose of these contact point systems. You know, for those of us who might not be telepathic.
class ContactPointSystemChoices(str, Enum): + """ + Enumeration of supported contact point system types. + + This enum defines the various methods through which contact can be established, + following standard communication protocols. + """ phone = "phone" fax = "fax" email = "email"
16-22: I'm guessing we're saving characters for a rainy day with 'temp'?While the implementation is correct, using 'temp' instead of 'temporary' feels a bit like we're playing hide and seek with clarity.
class ContactPointUseChoices(str, Enum): home = "home" work = "work" - temp = "temp" + temporary = "temporary" old = "old" mobile = "mobile"
24-28: A Pydantic model that's practically whispering its intentions.While the basic structure is fine, it would be delightful if we could add some field descriptions and validation. You know, just in case someone wants to understand what they're working with.
class ContactPoint(BaseModel): - system: ContactPointSystemChoices - value: str - use: ContactPointUseChoices + """ + Represents a contact point with its system type, value, and intended use. + """ + system: ContactPointSystemChoices = Field( + ..., + description="The type of contact point system (e.g., phone, email)" + ) + value: str = Field( + ..., + description="The actual contact point value (e.g., phone number, email address)", + min_length=1 + ) + use: ContactPointUseChoices = Field( + ..., + description="The context in which the contact point is used" + )care/security/permissions/device.py (1)
14-19: Empty description string could be more... descriptive.The empty description string (
"") could be replaced with meaningful information about the permission's scope and impact.- "", + "Allows viewing the list of devices registered to a facility",care/emr/resources/device/spec.py (2)
11-15: Consider adding 'maintenance' status.The
DeviceStatusChoicesenum might benefit from a 'maintenance' status for devices temporarily out of service.class DeviceStatusChoices(str, Enum): active = "active" inactive = "inactive" entered_in_error = "entered_in_error" + maintenance = "maintenance"
51-64: Empty spec classes could use some documentation.The empty spec classes (
DeviceCreateSpec,DeviceUpdateSpec,DeviceListSpec,DeviceRetrieveSpec) might confuse future developers. Perhaps add docstrings explaining their purpose?class DeviceCreateSpec(DeviceSpecBase): - pass + """Specification for creating new device instances.""" + passcare/emr/api/viewsets/device.py (1)
67-70: Those TODOs are quite... ambitious.Consider creating tickets for these TODOs to ensure they don't get forgotten. They seem important for complete device management.
Would you like me to help create tickets for these TODO items?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/emr/api/viewsets/device.py(1 hunks)care/emr/models/device.py(1 hunks)care/emr/resources/common/contact_point.py(1 hunks)care/emr/resources/device/spec.py(1 hunks)care/security/permissions/device.py(1 hunks)config/api_router.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/models/device.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/security/permissions/device.py (1)
20-25: Consider restricting device management to ADMIN and FACILITY_ADMIN.The current implementation allows STAFF to manage devices, which might be a bit too permissive for sensitive medical equipment.
Would you like me to help verify the role requirements for device management in your organization?
config/api_router.py (1)
169-173: LGTM!The device router registration follows the established pattern and is correctly nested under the facility router.
care/emr/api/viewsets/device.py
Outdated
| class DeviceFilters(filters.FilterSet): | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Empty FilterSet is looking a bit... lonely.
The DeviceFilters class could use some actual filters, like status, availability, or manufacturer.
class DeviceFilters(filters.FilterSet):
- pass
+ status = filters.CharFilter(field_name="status")
+ availability_status = filters.CharFilter(field_name="availability_status")
+ manufacturer = filters.CharFilter(field_name="manufacturer")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class DeviceFilters(filters.FilterSet): | |
| pass | |
| class DeviceFilters(filters.FilterSet): | |
| status = filters.CharFilter(field_name="status") | |
| availability_status = filters.CharFilter(field_name="availability_status") | |
| manufacturer = filters.CharFilter(field_name="manufacturer") |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
care/emr/api/viewsets/device.py (1)
27-28: 🛠️ Refactor suggestionThat FilterSet is still looking rather... minimalist.
I see we're still keeping that FilterSet nice and empty. Perhaps we could make it more useful by adding some actual filters? Just a thought.
class DeviceFilters(filters.FilterSet): - pass + status = filters.CharFilter(field_name="status") + availability_status = filters.CharFilter(field_name="availability_status") + manufacturer = filters.CharFilter(field_name="manufacturer") + type = filters.CharFilter(field_name="type")
🧹 Nitpick comments (1)
care/emr/api/viewsets/device.py (1)
89-101: Transaction block could use some error handling.The transaction block is a bit bare. What happens if something fails mid-way? Maybe we should add some error handling?
with transaction.atomic(): + try: if device.current_encounter: old_obj = DeviceEncounterHistory.objects.filter( device=device, encounter=device.current_encounter, end__isnull=True ).first() if old_obj: old_obj.end = timezone.now() old_obj.save() device.current_encounter = encounter device.save(update_fields=["current_encounter"]) DeviceEncounterHistory.objects.create( device=device, encounter=encounter, start=timezone.now() ) + except Exception as e: + # Log the error + raise ValidationError(f"Failed to associate encounter: {str(e)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/device.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/api/viewsets/device.py (2)
139-141: Those TODOs are piling up.We have quite a collection of TODOs here. Should we create tickets to track these items? They seem rather important for a medical system.
Would you like me to help create tickets for these pending items?
86-86:⚠️ Potential issueThose TODOs for authorization checks are making me nervous.
Leaving authorization checks as TODOs in a medical system is... interesting. We should probably implement these before going to production, don't you think?
Also applies to: 113-113
care/emr/api/viewsets/device.py
Outdated
| ) | ||
| device = self.get_object() | ||
| # TODO Perform Authz for location | ||
| if device.current_location == location.id: |
There was a problem hiding this comment.
That comparison looks... interesting.
You're comparing device.current_location (which is a ForeignKey) with location.id (which is an integer). I assume you meant to compare with the actual location object?
- if device.current_location == location.id:
+ if device.current_location == location:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if device.current_location == location.id: | |
| if device.current_location == location: |
care/emr/api/viewsets/device.py
Outdated
| class DeviceLocationHistoryViewSet(EMRModelReadOnlyViewSet): | ||
| pass | ||
|
|
||
|
|
||
| class DeviceEncounterHistoryViewSet(EMRModelReadOnlyViewSet): | ||
| pass | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Those empty history viewsets are looking rather lonely.
I see we're keeping our history viewsets nice and minimal too. Perhaps we could add some basic functionality?
class DeviceLocationHistoryViewSet(EMRModelReadOnlyViewSet):
- pass
+ database_model = DeviceLocationHistory
+ filterset_fields = ["device", "location", "start", "end"]
+ ordering_fields = ["start", "end"]
+ ordering = ["-start"]
class DeviceEncounterHistoryViewSet(EMRModelReadOnlyViewSet):
- pass
+ database_model = DeviceEncounterHistory
+ filterset_fields = ["device", "encounter", "start", "end"]
+ ordering_fields = ["start", "end"]
+ ordering = ["-start"]
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
care/emr/resources/device/spec.py (3)
13-17: Consider expanding device status choices.The current status choices seem a bit... limited. Common device lifecycle states like "maintenance", "retired", "decommissioned" might be useful for proper device management.
class DeviceStatusChoices(str, Enum): active = "active" inactive = "inactive" entered_in_error = "entered_in_error" + maintenance = "maintenance" + retired = "retired" + decommissioned = "decommissioned"
26-50: Add date validation and field documentation.The class could use some date validation to ensure manufacture_date precedes expiration_date. Also, a bit of documentation for these fields wouldn't hurt... just saying.
class DeviceSpecBase(EMRResource): __model__ = Device __exclude__ = [ "facility", "managing_organization", "current_location", "current_encounter", ] id: UUID4 = None + @validator("expiration_date") + def validate_dates(cls, v, values): + if "manufacture_date" in values and v and values["manufacture_date"]: + if v <= values["manufacture_date"]: + raise ValueError("expiration_date must be after manufacture_date") + return v + identifier: str | None = None - status: DeviceStatusChoices - availability_status: DeviceAvailabilityStatusChoices - manufacturer: str | None = None - manufacture_date: datetime | None = None - expiration_date: datetime | None = None + status: DeviceStatusChoices # Current operational status of the device + availability_status: DeviceAvailabilityStatusChoices # Physical availability status + manufacturer: str | None = None # Name of the device manufacturer + manufacture_date: datetime | None = None # Date when device was manufactured + expiration_date: datetime | None = None # Date when device expires/needs replacement
67-88: Optimize serialization with select_related.The serialization could be a bit more... efficient. Just a thought.
class DeviceRetrieveSpec(DeviceListSpec): current_encounter: dict | None = None current_location: dict created_by: dict | None = None updated_by: dict | None = None @classmethod def perform_extra_serialization(cls, mapping, obj): + # Ensure obj is fetched with select_related for better performance + if not hasattr(obj, '_prefetched_objects_cache'): + obj = Device.objects.select_related( + 'current_location', + 'current_encounter' + ).get(id=obj.id) super().perform_extra_serialization(mapping, obj) mapping["current_location"] = None mapping["current_encounter"] = Noneconfig/api_router.py (1)
179-181: Remove unused device_nested_router.Creating a nested router and not using it seems a bit... wasteful, don't you think?
-device_nested_router = NestedSimpleRouter( - facility_nested_router, r"device", lookup="device" -)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/device.py(1 hunks)care/emr/resources/device/spec.py(1 hunks)config/api_router.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
care/emr/api/viewsets/device.py (1)
89-89: That TODO is looking rather lonely.The authorization check is currently missing. This could be a security concern.
Would you like me to help implement the authorization check for encounters?
care/emr/api/viewsets/device.py
Outdated
| class DeviceEncounterHistoryViewSet(EMRModelReadOnlyViewSet): | ||
| database_model = DeviceLocationHistory | ||
| pydantic_read_model = DeviceEncounterHistoryListSpec | ||
|
|
||
| def get_device(self): | ||
| return get_object_or_404(Device, external_id=self.kwargs["device_external_id"]) | ||
|
|
||
| def get_queryset(self): | ||
| return DeviceLocationHistory.objects.filter( | ||
| device=self.get_device() | ||
| ).select_related("encounter") | ||
|
|
There was a problem hiding this comment.
Fix incorrect model and queryset in DeviceEncounterHistoryViewSet.
Oh dear, looks like we have a small mix-up here. The viewset is using DeviceLocationHistory instead of DeviceEncounterHistory, and the queryset is filtering by location instead of encounter.
class DeviceEncounterHistoryViewSet(EMRModelReadOnlyViewSet):
- database_model = DeviceLocationHistory
+ database_model = DeviceEncounterHistory
pydantic_read_model = DeviceEncounterHistoryListSpec
def get_device(self):
return get_object_or_404(Device, external_id=self.kwargs["device_external_id"])
def get_queryset(self):
- return DeviceLocationHistory.objects.filter(
+ return DeviceEncounterHistory.objects.filter(
device=self.get_device()
- ).select_related("encounter")
+ ).select_related("encounter")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class DeviceEncounterHistoryViewSet(EMRModelReadOnlyViewSet): | |
| database_model = DeviceLocationHistory | |
| pydantic_read_model = DeviceEncounterHistoryListSpec | |
| def get_device(self): | |
| return get_object_or_404(Device, external_id=self.kwargs["device_external_id"]) | |
| def get_queryset(self): | |
| return DeviceLocationHistory.objects.filter( | |
| device=self.get_device() | |
| ).select_related("encounter") | |
| class DeviceEncounterHistoryViewSet(EMRModelReadOnlyViewSet): | |
| database_model = DeviceEncounterHistory | |
| pydantic_read_model = DeviceEncounterHistoryListSpec | |
| def get_device(self): | |
| return get_object_or_404(Device, external_id=self.kwargs["device_external_id"]) | |
| def get_queryset(self): | |
| return DeviceEncounterHistory.objects.filter( | |
| device=self.get_device() | |
| ).select_related("encounter") |
care/emr/api/viewsets/device.py
Outdated
| class DeviceEncounterAssociationRequest(BaseModel): | ||
| encounter: UUID4 | ||
|
|
||
| @action(detail=True, methods=["POST"]) | ||
| def associate_encounter(self, request, *args, **kwargs): | ||
| request_data = self.DeviceEncounterAssociationRequest(**request.data) | ||
| encounter = get_object_or_404(Encounter, external_id=request_data.encounter) | ||
| device = self.get_object() | ||
| # TODO Perform Authz for encounter | ||
| if device.current_encounter_id == encounter.id: | ||
| raise ValidationError("Encounter already associated") | ||
| with transaction.atomic(): | ||
| if device.current_encounter: | ||
| old_obj = DeviceEncounterHistory.objects.filter( | ||
| device=device, encounter=device.current_encounter, end__isnull=True | ||
| ).first() | ||
| if old_obj: | ||
| old_obj.end = timezone.now() | ||
| old_obj.save() | ||
| device.current_encounter = encounter | ||
| device.save(update_fields=["current_encounter"]) | ||
| DeviceEncounterHistory.objects.create( | ||
| device=device, encounter=encounter, start=timezone.now() | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add response after successful association.
The method seems to be missing a response after successful association. How thoughtful.
@action(detail=True, methods=["POST"])
def associate_encounter(self, request, *args, **kwargs):
request_data = self.DeviceEncounterAssociationRequest(**request.data)
encounter = get_object_or_404(Encounter, external_id=request_data.encounter)
device = self.get_object()
if device.current_encounter_id == encounter.id:
raise ValidationError("Encounter already associated")
with transaction.atomic():
if device.current_encounter:
old_obj = DeviceEncounterHistory.objects.filter(
device=device, encounter=device.current_encounter, end__isnull=True
).first()
if old_obj:
old_obj.end = timezone.now()
old_obj.save()
device.current_encounter = encounter
device.save(update_fields=["current_encounter"])
DeviceEncounterHistory.objects.create(
device=device, encounter=encounter, start=timezone.now()
)
+ return Response({"status": "success", "message": "Encounter associated successfully"})
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/emr/api/viewsets/device.py (1)
89-117: That response handling could use some love.The associate_encounter method returns different response formats based on whether an encounter is provided. Consider standardizing the response format for consistency.
- return Response({}) + return Response({"status": "success", "message": "Encounter disassociated successfully"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/device.py(1 hunks)care/emr/resources/device/spec.py(1 hunks)config/api_router.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/resources/device/spec.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test / Test
🔇 Additional comments (3)
care/emr/api/viewsets/device.py (2)
30-32: Looking much better with those filters!The FilterSet is no longer empty and includes appropriate filters for current_location and current_encounter using UUID fields.
94-94: 🏁 Script executed:#!/bin/bash # Search for existing authorization implementations in the codebase echo "Searching for authorization patterns..." rg -A 5 "def has_permission|def check_permission"Length of output: 2817
config/api_router.py (1)
173-194: Nice and consistent router setup!The device routes are well-organized and follow the established pattern of nested routers.
care/emr/api/viewsets/device.py
Outdated
| return get_object_or_404(Device, external_id=self.kwargs["device_external_id"]) | ||
|
|
||
| def get_queryset(self): | ||
| # Todo Check access to device |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Those device access TODOs are giving me anxiety.
Both history viewsets are missing access control checks. Also, consider adding filtering options for better querying capabilities.
class DeviceLocationHistoryViewSet(EMRModelReadOnlyViewSet):
database_model = DeviceLocationHistory
pydantic_read_model = DeviceLocationHistoryListSpec
+ filterset_fields = ["location", "start", "end"]
+ ordering_fields = ["start", "end"]
+ ordering = ["-start"]Also applies to: 183-183, 184-184
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
care/emr/migrations/0018_alter_device_care_type_alter_device_status.py (1)
16-16: Those field lengths seem... interesting.The
care_typefield allows 1024 characters which seems rather generous, whilestatusis limited to 16 characters without any choices validation. Perhaps we could make this more structured?Consider:
- Adding choices for the status field
- Reducing care_type length unless there's a specific requirement
- Adding a check constraint for valid status values
- field=models.CharField(max_length=16), + field=models.CharField( + max_length=16, + choices=[ + ('active', 'Active'), + ('inactive', 'Inactive'), + ('maintenance', 'Under Maintenance'), + ('retired', 'Retired') + ] + ),Also applies to: 21-21
care/security/authorization/base.py (1)
56-56: That TODO about caching is looking rather lonely.The comment suggests implementing caching for the
get_role_from_permissionsmethod. This could indeed improve performance, especially with multiple permission checks.Would you like me to propose a caching implementation using Django's caching framework?
care/emr/api/viewsets/device.py (2)
99-138: Those associate methods are looking rather... similar.The
associate_encounterandassociate_locationmethods share a lot of common patterns. Perhaps we could reduce this duplication?Consider extracting common logic into a base method:
def _associate_entity(self, entity_type, entity, request): device = self.get_object() facility = self.get_facility_obj() if not AuthorizationController.call( f"can_associate_device_{entity_type}", self.request.user, facility ): raise PermissionDenied(f"You do not have permission to associate {entity_type}") current_entity = getattr(device, f"current_{entity_type}") if entity and current_entity and current_entity.id == entity.id: raise ValidationError(f"{entity_type.title()} already associated") if entity and entity.facility_id != facility.id: raise ValidationError(f"{entity_type.title()} is not part of given facility") history_model = DeviceEncounterHistory if entity_type == 'encounter' else DeviceLocationHistory with transaction.atomic(): if current_entity: old_obj = history_model.objects.filter( device=device, **{entity_type: current_entity}, end__isnull=True ).first() if old_obj: old_obj.end = timezone.now() old_obj.save() setattr(device, f"current_{entity_type}", entity) device.save(update_fields=[f"current_{entity_type}"]) if entity: obj = history_model.objects.create( device=device, **{entity_type: entity}, start=timezone.now(), created_by=request.user, ) spec_class = DeviceEncounterHistoryListSpec if entity_type == 'encounter' else DeviceLocationHistoryListSpec return Response(spec_class.serialize(obj).to_json()) return Response({"status": "success", "message": f"{entity_type.title()} disassociated successfully"})Also applies to: 142-181
137-137: Those empty responses for disassociation are a bit... sparse.When disassociating an encounter or location, returning an empty response isn't very informative.
Add a proper success response:
- return Response({}) + return Response({"status": "success", "message": "Entity disassociated successfully"})Also applies to: 180-181
care/emr/api/viewsets/encounter.py (1)
111-125: Add error handling for database operations.While the transaction ensures data consistency, it would be slightly more robust to add explicit error handling for the database operations.
def perform_update(self, instance): with transaction.atomic(): if instance.status in COMPLETED_CHOICES: - device_ids = list( - Device.objects.filter(current_encounter=instance).values_list( - "id", flat=True - ) - ) - Device.objects.filter(id__in=device_ids).update(current_encounter=None) + try: + device_ids = list( + Device.objects.filter(current_encounter=instance).values_list( + "id", flat=True + ) + ) + Device.objects.filter(id__in=device_ids).update(current_encounter=None) - DeviceEncounterHistory.objects.filter( - device_id__in=device_ids, encounter=instance, end__isnull=True - ).update(end=timezone.now()) + DeviceEncounterHistory.objects.filter( + device_id__in=device_ids, encounter=instance, end__isnull=True + ).update(end=timezone.now()) + except Device.DoesNotExist: + pass # No devices to update + except Exception as e: + logger.error(f"Error updating device associations: {e}") + raise super().perform_update(instance)care/emr/tests/test_device_api.py (3)
23-86: Add docstrings to improve code documentation.The base test class could use some docstrings to explain its purpose and methods. I mean, it's probably clear to you what it does, but future developers might appreciate the extra context.
class DeviceBaseTest(CareAPITestBase, FacilityLocationMixin): + """Base test class for device-related test cases. + + Provides common setup and helper methods for device testing. + """ def setUp(self): + """Set up test data including user, facility, and patient.""" self.user = self.create_user() self.facility = self.create_facility(user=self.user) self.facility_organization = self.create_facility_organization(
88-365: Add test for concurrent device updates.The test coverage is quite thorough, but it might be nice to have a test case for concurrent device updates to ensure thread safety.
Here's a suggested test case:
def test_concurrent_device_updates(self): """Test concurrent updates to the same device.""" from threading import Thread from django.test import TransactionTestCase device = self.create_device() self.add_permissions([DevicePermissions.can_manage_devices.name]) url = self.get_device_detail_url(device) def update_device(): data = self.generate_device_data() self.client.put(url, data=data, format="json") threads = [Thread(target=update_device) for _ in range(5)] for thread in threads: thread.start() for thread in threads: thread.join() # Verify device data integrity response = self.client.get(url) self.assertEqual(response.status_code, 200)
367-466: Reduce code duplication in history test classes.There's quite a bit of duplicate code between
TestDeviceLocationHistoryViewSetandTestDeviceEncounterHistoryViewSet. It would be slightly more maintainable to extract common patterns into a base class.class BaseDeviceHistoryTest(DeviceBaseTest): """Base class for device history test cases.""" def setUp(self): super().setUp() self.device = self.create_device() self.setup_association_data() self.base_url = self.get_base_url() def setup_association_data(self): """Override in subclasses to set up specific association data.""" raise NotImplementedError def get_base_url(self): """Override in subclasses to return specific base URL.""" raise NotImplementedError def test_list_history(self): """Test listing history records.""" self.associate_with_device() response = self.client.get(self.base_url) self.assertEqual(response.status_code, 403) self.add_permissions([DevicePermissions.can_list_devices.name]) response = self.client.get(self.base_url) self.assertEqual(response.status_code, 200) self.assertEqual(response.json()["count"], 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
care/emr/api/viewsets/device.py(1 hunks)care/emr/api/viewsets/encounter.py(2 hunks)care/emr/migrations/0018_alter_device_care_type_alter_device_status.py(1 hunks)care/emr/models/device.py(1 hunks)care/emr/tests/test_device_api.py(1 hunks)care/security/authorization/__init__.py(1 hunks)care/security/authorization/base.py(1 hunks)care/security/authorization/device.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- care/security/authorization/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/models/device.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0018_alter_device_care_type_alter_device_status.py
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)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
care/security/authorization/device.py (1)
7-38: Well-structured authorization implementation!The permission checks are comprehensive and follow the principle of least privilege. The separation between listing and management permissions is particularly thoughtful.
care/security/authorization/base.py (1)
41-53: Solid implementation of multiple permission checks!The method properly ensures all permissions are satisfied before granting access. The early return on failure is an efficient touch.
care/emr/api/viewsets/device.py (1)
184-223: Those history viewsets are looking rather basic.The history viewsets could use some additional features like filtering and ordering.
As mentioned in a previous review, consider adding filterset fields and ordering options to both history viewsets.
| # TODO Check access to location with permission and then allow filter with or, | ||
| # Access should not be limited by location if the device has org access | ||
| # If location access then allow all, otherwise apply organization filter |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
That TODO about location access is giving me anxiety.
The comment indicates a need to implement proper location permission checks. This could lead to security issues if not addressed.
🏁 Script executed:
#!/bin/bash
# Check for any existing location permission implementations
rg -A 5 "can_access_location|can_view_location" Length of output: 49
I'll now run a script to extract lines 80–100 from care/emr/api/viewsets/device.py so we can inspect the surrounding context of this TODO. That should help us determine whether any location permission checks are at all implemented.
🏁 Script executed:
#!/bin/bash
sed -n '80,100p' care/emr/api/viewsets/device.pyLength of output: 916
Action Needed: Implement Location Permission Checks
The file care/emr/api/viewsets/device.py still contains a TODO about properly checking location access permissions (lines 86-88). Our search for keywords like "can_access_location" or "can_view_location" produced no results, confirming that the necessary permission checks haven’t been implemented. Please address this to ensure that device filtering isn’t exposing a security gap.
- No current location permission verification is implemented.
- Consider adding logic to check if users have location access before applying filters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/api/viewsets/encounter.py (1)
101-115: It wouldn't hurt to add some error handling and documentation, you know.Consider adding:
- A docstring explaining the method's purpose and behavior
- Error handling for potential database operation failures
- Logging for debugging purposes
Here's a slightly more robust version:
def perform_update(self, instance): + """ + Updates an encounter instance and manages associated device relationships. + + When an encounter is completed, this method: + 1. Removes the encounter association from linked devices + 2. Updates the device encounter history with end timestamps + """ with transaction.atomic(): + try: if instance.status in COMPLETED_CHOICES: device_ids = list( Device.objects.filter(current_encounter=instance).values_list( "id", flat=True ) ) Device.objects.filter(id__in=device_ids).update(current_encounter=None) DeviceEncounterHistory.objects.filter( device_id__in=device_ids, encounter=instance, end__isnull=True ).update(end=timezone.now()) super().perform_update(instance) + except Exception as e: + logger.error(f"Failed to update encounter {instance.id}: {str(e)}") + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/encounter.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/api/viewsets/encounter.py (2)
23-24: Oh look, nicely organized imports!The new Device and DeviceEncounterHistory imports are properly placed within the existing care.emr.models group. How refreshing to see someone following basic import organization principles.
101-115: Well, at least the transaction handling is solid.The implementation correctly uses atomic transactions and efficient bulk updates to maintain data consistency when updating device-encounter relationships. Not bad at all.
Summary by CodeRabbit
New Features
Documentation
Tests