Disallow creating availabilities of duration not multiple of slot size duration; Prevent overlapping availability creation #2795
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhanced validation and management for scheduling and availability creation in the EMR system. The changes focus on preventing overlapping availability sessions, ensuring proper schedule linkage, and adding robust validation checks during availability and schedule creation. The modifications span across the viewset, specification, and test files to implement comprehensive validation logic. Changes
Assessment against linked issues
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: 2
🧹 Nitpick comments (5)
care/emr/api/viewsets/scheduling/schedule.py (1)
169-171: Good data consistency enforcement.The method ensures schedule is properly set from URL parameters. Though, it might be worth adding a docstring to explain its purpose, you know, for those who come after us.
def clean_create_data(self, request_data): + """Ensure schedule is set from URL parameters before creating availability.""" request_data["schedule"] = self.kwargs["schedule_external_id"] return request_datacare/emr/resources/scheduling/schedule/spec.py (2)
71-82: Slot duration validation could be more efficient.While the validation logic is correct, we're doing datetime operations that could be simplified to time-based calculations.
Consider this more efficient approach:
- start_time = datetime.datetime.combine(datetime.date.today(), availability.start_time, tzinfo=None) - end_time = datetime.datetime.combine(datetime.date.today(), availability.end_time, tzinfo=None) - slot_size_in_seconds = self.slot_size_in_minutes * 60 - if (end_time - start_time).total_seconds() % slot_size_in_seconds != 0: + minutes_diff = ( + availability.end_time.hour * 60 + availability.end_time.minute + - (availability.start_time.hour * 60 + availability.start_time.minute) + ) + if minutes_diff % self.slot_size_in_minutes != 0:🧰 Tools
🪛 Ruff (0.8.2)
73-73:
datetime.date.today()used(DTZ011)
76-76:
datetime.date.today()used(DTZ011)
🪛 GitHub Actions: Lint Code Base
[error] 73-73: DTZ011: Use datetime.datetime.now(tz=...).date() instead of datetime.date.today()
[error] 76-76: DTZ011: Use datetime.datetime.now(tz=...).date() instead of datetime.date.today()
221-233: Document the overlap checking algorithm.While the implementation is efficient by skipping different days, it would be nice to have some documentation explaining the algorithm.
def has_overlapping_availability(availabilities: list[AvailabilityDateTimeSpec]): + """ + Check if any two availabilities overlap in time on the same day. + + Args: + availabilities: List of availability specs to check + + Returns: + bool: True if any overlaps found, False otherwise + + Note: + Two time ranges overlap if start of one is before end of other + and vice versa. We optimize by skipping checks for different days. + """ for i in range(len(availabilities)):care/emr/tests/test_schedule_api.py (2)
193-236: Add more edge cases to the overlapping availability test.While the current test is good, it would be even better to test more edge cases.
Consider adding these test cases:
# Test exact boundary overlap { "day_of_week": 1, "start_time": "13:00:00", "end_time": "17:00:00", }, { "day_of_week": 1, "start_time": "17:00:00", # Boundary case "end_time": "20:00:00", } # Test complete containment { "day_of_week": 1, "start_time": "09:00:00", "end_time": "17:00:00", }, { "day_of_week": 1, "start_time": "11:00:00", # Contained within first slot "end_time": "15:00:00", }
932-956: Make the slot size validation test more explicit.The test could be clearer about why it fails. The magic number 13:13:00 doesn't immediately show why it's invalid.
Consider making the test more explicit:
- "end_time": "13:13:00", + # 4 hours and 13 minutes = 253 minutes + # Not divisible by slot_size_in_minutes (30) + "end_time": "13:13:00", # Results in 253 minutes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/scheduling/schedule.py(3 hunks)care/emr/resources/scheduling/schedule/spec.py(4 hunks)care/emr/tests/test_schedule_api.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/resources/scheduling/schedule/spec.py
73-73: datetime.date.today() used
(DTZ011)
76-76: datetime.date.today() used
(DTZ011)
🪛 GitHub Actions: Lint Code Base
care/emr/resources/scheduling/schedule/spec.py
[error] 73-73: DTZ011: Use datetime.datetime.now(tz=...).date() instead of datetime.date.today()
[error] 76-76: DTZ011: Use datetime.datetime.now(tz=...).date() instead of datetime.date.today()
⏰ 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/scheduling/schedule.py (1)
22-22: Nice separation of create and retrieve models!The change from
AvailabilityForScheduleSpectoAvailabilityCreateSpecfor creation while keepingAvailabilityForScheduleSpecfor retrieval shows good attention to separation of concerns.Also applies to: 136-137
care/emr/resources/scheduling/schedule/spec.py (1)
54-58:⚠️ Potential issueGood validation logic, but let's fix those timezone warnings.
The validation checks for overlaps and time order are solid. However, there's a timezone-related issue that needs attention in the implementation.
The use of
datetime.date.today()in the validation logic could lead to timezone-related issues. Consider using timezone-aware datetime objects consistently:- start_time = datetime.datetime.combine(datetime.date.today(), availability.start_time, tzinfo=None) + start_time = datetime.datetime.now(UTC).replace( + hour=availability.start_time.hour, + minute=availability.start_time.minute, + second=availability.start_time.second, + microsecond=0 + )Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…uration-multiple-of-slot-size-in-minutes' into rithviknishad/fix/availability-duration-multiple-of-slot-size-in-minutes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
care/emr/resources/scheduling/schedule/spec.py (4)
55-59: Perhaps we could make these error messages a tad more helpful?The validation logic is correct, but the error messages could be more descriptive to help users understand exactly what went wrong.
- raise ValueError("Availability time ranges are overlapping") + raise ValueError("Cannot create overlapping availability time ranges. Please ensure all time slots are distinct.") - raise ValueError("Start time must be earlier than end time") + raise ValueError(f"Invalid time range: start time ({availability.start_time}) must be earlier than end time ({availability.end_time})")
71-83: A tiny optimization opportunity presents itself...The current implementation creates datetime objects just to calculate duration. We could simplify this by working directly with time objects.
- start_time = datetime.datetime.combine( - datetime.datetime.now(tz=UTC).date(), availability.start_time - ) - end_time = datetime.datetime.combine( - datetime.datetime.now(tz=UTC).date(), availability.end_time - ) - slot_size_in_seconds = self.slot_size_in_minutes * 60 - if (end_time - start_time).total_seconds() % slot_size_in_seconds != 0: + seconds_in_day = 24 * 60 * 60 + start_seconds = availability.start_time.hour * 3600 + availability.start_time.minute * 60 + end_seconds = availability.end_time.hour * 3600 + availability.end_time.minute * 60 + if end_seconds < start_seconds: + end_seconds += seconds_in_day + duration_seconds = end_seconds - start_seconds + if duration_seconds % (self.slot_size_in_minutes * 60) != 0:
136-146: Type hints would make this even better...The implementation is clean, but adding return type hints would improve code maintainability.
@classmethod - def validate_availabilities_not_overlapping( - cls, availabilities: list[AvailabilityForScheduleSpec] - ): + def validate_availabilities_not_overlapping( + cls, availabilities: list[AvailabilityForScheduleSpec] + ) -> list[AvailabilityForScheduleSpec]:
222-234: O(n²) might not be our best friend here...The current implementation uses nested loops which could be inefficient for large datasets. Consider optimizing by sorting availabilities first.
def has_overlapping_availability(availabilities: list[AvailabilityDateTimeSpec]): + # Sort availabilities by day_of_week and start_time + sorted_avail = sorted(availabilities, key=lambda x: (x.day_of_week, x.start_time)) + + # Check adjacent pairs for overlap (O(n)) + for i in range(len(sorted_avail) - 1): + if sorted_avail[i].day_of_week == sorted_avail[i + 1].day_of_week: + if sorted_avail[i].end_time > sorted_avail[i + 1].start_time: + return True + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/scheduling/schedule.py(3 hunks)care/emr/resources/scheduling/schedule/spec.py(5 hunks)care/emr/tests/test_schedule_api.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- care/emr/api/viewsets/scheduling/schedule.py
- care/emr/tests/test_schedule_api.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
care/emr/resources/scheduling/schedule/spec.py (1)
90-112: That nested list comprehension issue is still lurking around...The bug in extending availabilities still needs to be addressed as mentioned in the previous review.
|
@rithviknishad Fix Tests |
Proposed Changes
Associated Issue
Merge Checklist
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests