[WEB-3044] fix: Correct handling of equal start and end dates in cycle create and update #6328
Conversation
|
Warning Rate limit exceeded@gurusainath has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 54 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces enhancements to the cycle-related serializers, views, and utility functions across multiple files. The changes primarily focus on improving date conversion logic, adding new read-only fields to the Changes
Sequence DiagramsequenceDiagram
participant User
participant CycleSerializer
participant TimezoneConverter
participant Database
User->>CycleSerializer: Create/Update Cycle
CycleSerializer->>TimezoneConverter: Convert Dates
TimezoneConverter-->>CycleSerializer: Converted Dates
CycleSerializer->>Database: Save Cycle
Database-->>CycleSerializer: Return Cycle with Metadata
Possibly related PRs
Suggested labels
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: 1
🧹 Nitpick comments (4)
apiserver/plane/api/serializers/cycle.py (2)
34-36: Avoid unnecessary conditional expression.
You can directly use boolean assignment without theTrue if ... else Falsepattern.- is_start_date_end_date_equal = ( - True if data.get("start_date") == data.get("end_date") else False - ) + is_start_date_end_date_equal = data.get("start_date") == data.get("end_date")
43-45: Check clarity ofis_end_dateusage.
Passingis_end_date=is_start_date_end_date_equalis correct for your range logic, but the naming might be more explicit if you are indicating “same-day end date.”apiserver/plane/app/serializers/cycle.py (1)
24-26: Same comment on boolean assignment.
You can remove the redundant ternary expression here, too.- is_start_date_end_date_equal = ( - True if data.get("start_date") == data.get("end_date") else False - ) + is_start_date_end_date_equal = data.get("start_date") == data.get("end_date")apiserver/plane/utils/timezone_converter.py (1)
64-65: Revisit the 1-second offset.
Confirm if shifting by one second is intentional and documented. Minor offsets can confuse downstream calculations if not clearly specified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apiserver/plane/api/serializers/cycle.py(2 hunks)apiserver/plane/app/serializers/cycle.py(1 hunks)apiserver/plane/app/views/cycle/base.py(4 hunks)apiserver/plane/utils/timezone_converter.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-apiserver
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
apiserver/plane/api/serializers/cycle.py (2)
9-9: New serializer class looks good.
This addition provides helpful read-only fields for cycle metrics.
38-40: Ensure date field safety.
Although guarded by prior checks, ifstart_dateis unexpectedlyNone, calling.date()will raise an error. Consider raising a more descriptive exception or skipping conversion if that’s a possibility.apiserver/plane/app/serializers/cycle.py (2)
28-30: Validate potential for null.
Ifstart_dateis somehow not passed or malformed,.date()might fail. Consider adding a short-circuit or raising an exception if the date is invalid.
33-35: Maintain consistent naming.
Ensuring thatis_end_date=is_start_date_end_date_equaldoesn't conflict in meaning with other boolean flags can improve readability. Otherwise, it seems consistent with your date handling approach.apiserver/plane/utils/timezone_converter.py (3)
6-6: Helpful function for time zone conversions.
Ensure that any potential invalid or missinguser_timezonescenario is handled upstream, as callingpytz.timezonewith an invalid argument will raise an error.
32-32: Addition ofis_end_dateparameter broadens the function’s utility.
This extended signature fits well with your new date-handling logic.
66-68: Inclusive end date logic.
Adding 23:59:59 is a common approach to treat end dates as inclusive. Make sure this is clearly communicated to teams and documented, as it can affect scheduling.apiserver/plane/app/views/cycle/base.py (3)
146-146: Annotation logic is concise.
This clarifies status transitions well. No issues noted.
259-261: Time zone conversion usage is clean.
The multi-line call is more readable, and passingrequest.user.user_timezoneis consistent with the new utility function.
459-461: Repetition is consistent.
Same usage pattern ensures consistent time zone conversion in different endpoints.
| is_start_date_end_date_equal = ( | ||
| True if str("start_date") == str("end_date") else False | ||
| ) | ||
| start_date = convert_to_utc( | ||
| date=str(start_date), project_id=project_id, is_start_date=True | ||
| ) | ||
| end_date = convert_to_utc( | ||
| date=str(end_date), | ||
| project_id=project_id, | ||
| is_end_date=is_start_date_end_date_equal, | ||
| ) |
There was a problem hiding this comment.
Critical bug in equality check.
Currently, it compares literal strings "start_date" and "end_date", which will never match. Fix this to compare the actual variables:
- is_start_date_end_date_equal = (
- True if str("start_date") == str("end_date") else False
- )
+ is_start_date_end_date_equal = (
+ True if str(start_date) == str(end_date) else False
+ )📝 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.
| is_start_date_end_date_equal = ( | |
| True if str("start_date") == str("end_date") else False | |
| ) | |
| start_date = convert_to_utc( | |
| date=str(start_date), project_id=project_id, is_start_date=True | |
| ) | |
| end_date = convert_to_utc( | |
| date=str(end_date), | |
| project_id=project_id, | |
| is_end_date=is_start_date_end_date_equal, | |
| ) | |
| is_start_date_end_date_equal = ( | |
| True if str(start_date) == str(end_date) else False | |
| ) | |
| start_date = convert_to_utc( | |
| date=str(start_date), project_id=project_id, is_start_date=True | |
| ) | |
| end_date = convert_to_utc( | |
| date=str(end_date), | |
| project_id=project_id, | |
| is_end_date=is_start_date_end_date_equal, | |
| ) |
Description
start_dateandend_dateare equal during cycle creation and updates.Type of Change
References
[WEB-3044]
Summary by CodeRabbit
New Features
Bug Fixes
Refactor