Scheduling Validations #2701
Scheduling Validations #2701
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring across multiple files, shifting the focus from resource-based to user-based filtering in scheduling and availability components. The changes primarily involve renaming Changes
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Nitpick comments (3)
care/emr/migrations/0061_rename_resource_schedulableuserresource_user_and_more.py (1)
19-21: Confirm the default oflistfordosage_instruction.Switching to
JSONFieldwith a list default is logical. However, please verify your domain requirements to ensure a list suits all usage scenarios—some might prefer a dict structure for key-value entries.care/emr/models/scheduling/schedule.py (1)
10-10: Update the outdated TODO comment.The comment still mentions indexing with “resource,” but now it's a user reference. A quick rename here would keep things consistent and less confusing for future maintainers.
care/emr/api/viewsets/scheduling/availability.py (1)
218-218: Rename variable for clarity
Usingresourceto store aSchedulableUserResourceobject can mislead. Consider renaming it to something likeschedulable_user_resourcefor consistency and readability.-resource = SchedulableUserResource.objects.filter(user=user).first() +schedulable_user_resource = SchedulableUserResource.objects.filter(user=user).first()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
care/emr/api/viewsets/facility.py(1 hunks)care/emr/api/viewsets/scheduling/availability.py(3 hunks)care/emr/api/viewsets/scheduling/availability_exceptions.py(1 hunks)care/emr/api/viewsets/scheduling/booking.py(2 hunks)care/emr/api/viewsets/scheduling/schedule.py(1 hunks)care/emr/migrations/0061_rename_resource_schedulableuserresource_user_and_more.py(1 hunks)care/emr/models/scheduling/schedule.py(1 hunks)care/emr/resources/scheduling/availability_exception/spec.py(1 hunks)care/emr/resources/scheduling/schedule/spec.py(1 hunks)
🔇 Additional comments (10)
care/emr/migrations/0061_rename_resource_schedulableuserresource_user_and_more.py (1)
13-17: Smooth renaming ofresourcetouser.Kudos for systematically updating the field name to something more descriptive. Double-check that any queries or references to the old field name aren’t lurking around elsewhere, though I’m sure you’ve combed it all carefully.
care/emr/api/viewsets/scheduling/availability_exceptions.py (1)
17-17: Filter shift fromresource__external_idtoresource__user__external_idlooks good.Just be mindful you’ve got “resource” in the chain while also referencing the user. Everything aligns with the new approach, but as with other files, watch for stray references that might trip up newcomers.
care/emr/resources/scheduling/schedule/spec.py (1)
74-74: Praise for the user-based refactor.Updating the
resourceparameter touseris consistent with the broader shift away from resource-based scheduling. This change aligns theSchedulableUserResourceinstantiation directly to theuser, which clarifies intent and improves maintainability. Great step forward, albeit do keep a close eye on any leftover references toresourcecalls in the rest of the codebase.care/emr/api/viewsets/scheduling/schedule.py (1)
21-21: Filter naming clarity is appreciated.Renaming the filter to
userclarifies its purpose and makes it more discoverable for future maintenance. This is a cohesive move that syncs well with the revised data-modeling approach. Nicely done.care/emr/api/viewsets/scheduling/booking.py (3)
29-29: Filter rename touser.Replacing the
resourcefilter withuseris consistent with the rest of the changes. This enforces a uniform approach across the scheduling module.
32-32: Descriptive function name improvement.Renaming
filter_by_resourcetofilter_by_userclarifies its functionality and reduces confusion. This small shift makes it more coherent for future contributors.
36-36: Effective user-based query.Using
user__external_id=valuefurther cements the transition to user-centric lookups. The logic is concise and avoids any leftover references toresource_id. Keep in mind to verify all other related references, just to avoid any sneaky mismatches.care/emr/api/viewsets/facility.py (1)
107-107: Great alignment with user-based architecture.Changing
.values("resource_id")to.values("user_id")eliminates confusion about the entity in focus. This is consistent with the rest of the refactor, which we trust is thoroughly tested.care/emr/api/viewsets/scheduling/availability.py (2)
11-11: Nice addition for robust lookups
It’s more reliable than manually filtering the user. Though one might wonder why it took so long, this is definitely an improvement.
114-114: Consistent with user-based filtering
Switching fromresourcetouseraligns with your new approach, so just ensure other references remain consistent to avoid confusion down the line.
| user = User.objects.get(external_id=self.resource) | ||
| resource = SchedulableUserResource.objects.get( | ||
| resource=user_resource, | ||
| user=user, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify naming to avoid mixing "resource" and "user".
The parameter self.resource is now used to fetch a User, and then assigned to obj.resource. It works, but maybe rename the incoming data field to user_id or something less confusing. It’ll save you from an extra squint in the future.
| @@ -105,12 +106,12 @@ def get_slots_for_day(self, request, *args, **kwargs): | |||
| @classmethod | |||
| def get_slots_for_day_handler(cls, facility_external_id, request_data): | |||
| request_data = SlotsForDayRequestSpec(**request_data) | |||
| user = User.objects.filter(external_id=request_data.resource).first() | |||
| user = get_object_or_404(User, external_id=request_data.user) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant check after get_object_or_404
get_object_or_404 already handles the not-found scenario, so the manual if not user check right below is never reached.
user = get_object_or_404(User, external_id=request_data.user)
-if not user:
- raise ValidationError("Resource does not exist")Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
care/security/authorization/user_schedule.py (1)
58-59: Soft reminder to ensure consistent usage
The logic here is duplicated from thecan_write_user_schedulemethod above. If the underlying data requirement differs, consider consolidating or extracting into a helper.care/emr/api/viewsets/scheduling/availability.py (1)
218-218: Clarify the variable naming
Renaming local variables fromresourceto something more descriptive likeschedulable_useroruser_resourcemay help new developers quickly understand the function’s purpose.-resource = SchedulableUserResource.objects.filter(user=user).first() +user_resource = SchedulableUserResource.objects.filter(user=user).first()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
care/emr/api/viewsets/scheduling/availability.py(4 hunks)care/emr/api/viewsets/scheduling/booking.py(2 hunks)care/emr/api/viewsets/scheduling/schedule.py(2 hunks)care/emr/resources/scheduling/slot/spec.py(1 hunks)care/security/authorization/user_schedule.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/api/viewsets/scheduling/schedule.py
🔇 Additional comments (8)
care/security/authorization/user_schedule.py (1)
43-44: Excellent approach to retrieving organization details
These lines correctly utilizevalues_listwithflat=Trueto fetch the organization'sparent_cachefor further validation. This is clear and straightforward.care/emr/resources/scheduling/slot/spec.py (1)
92-92: Verify the updated attribute
Changing fromresource_idtouser_idis consistent with the shift to user-centric logic. Just ensure all references toresource.resource_idhave been replaced accordingly in the broader codebase.✅ Verification successful
No lingering
resource_idreferences found, but I suppose you knew that alreadyAfter a thorough analysis of the codebase (and I mean thorough), the change from
resource_idtouser_idappears to be consistent. The only references toresource_idfound are in:
- A migration file (which is expected and shouldn't be modified)
- Questionnaire-related files using it in a different context
The file under review correctly uses
user_idand integrates well with the rest of the codebase, which seems to have already embraced the user-centric approach. The extensive presence ofUserSpecimports and user-related patterns throughout the scheduling system suggests this was a well-planned transition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Double-check for references to the old `resource_id` attribute across the codebase rg "resource\.resource_id"Length of output: 28
Script:
#!/bin/bash # Let's try a more comprehensive search for resource_id patterns rg -i "resource_?id" # Also search for specific resource ID access patterns ast-grep --pattern 'resource.$_id'Length of output: 450
Script:
#!/bin/bash # Let's check the specific file and its imports to understand the context better cat "care/emr/resources/scheduling/slot/spec.py"Length of output: 2618
Script:
#!/bin/bash # Let's also check for any User model references with resource relationships rg "resource.*user" -A 2Length of output: 9090
care/emr/api/viewsets/scheduling/booking.py (3)
29-29: Filter field rename is consistent
Renamingresourcetouseraligns with the new user-based paradigm. Minimal changes needed here.
32-36: Consider fallback behavior
If you’re transitioning from a resource to a user, ensure the fallback logic (e.g., if no matching user is found) is well-tested for unexpected scenarios or data migrations not yet completed.
93-99: Renaming toavailable_usersis well done
This is straightforward and consistent with the rest of the refactoring. The fix ensures a more intuitive naming, though subtlety might be lost on external collaborators who still think in terms of “resources.”care/emr/api/viewsets/scheduling/availability.py (3)
30-30: Field rename touser
The rename touseris consistent with the user-based approach. This will reduce confusion about typical roles vs. “resources.”
109-109: Remove unreachable check
get_object_or_404(User, external_id=request_data.user)already raises an error if the user is missing, making theif not user:check unreachable.user = get_object_or_404(User, external_id=request_data.user) -if not user: - raise ValidationError("Resource does not exist")
114-114: Consistency in referencinguser
This line is fully consistent with the revised user-based approach. Nicely done.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
resourcereferences touserin multiple viewsets, models, and resources.userinstead ofresourcein related models.Migrations
resourcefield touserin schedulable user resources.medicationrequestmodel to use JSONField for dosage instructions.