fix: add timezone key to alert and derived stream #4536
Conversation
|
Warning Rate limit exceeded@omkarK06 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 19 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. WalkthroughThe changes introduce a new optional field, Changes
Possibly related PRs
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
Outside diff range and nitpick comments (1)
web/src/components/alerts/ScheduledAlert.vue (1)
Line range hint
894-1104: The timezone handling logic looks good! Consider adding some code comments.The new reactive properties, computed property, and methods related to timezone handling are implemented correctly. They enable syncing the timezone value with the parent component and provide timezone search functionality.
The update to the
filterFieldsmethod to handle both object and string type columns is a good improvement.Consider adding some code comments to explain the purpose and functionality of the timezone related properties and methods. It will help in understanding the code better.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- web/src/components/alerts/AddAlert.vue (7 hunks)
- web/src/components/alerts/ScheduledAlert.vue (12 hunks)
- web/src/components/reports/CreateReport.vue (2 hunks)
- web/src/utils/date.ts (2 hunks)
- web/src/utils/zincutils.ts (1 hunks)
Additional comments not posted (9)
web/src/utils/zincutils.ts (1)
916-924: LGTM!The
getTimezonesByOffsetfunction is implemented correctly. It properly converts the offset from minutes to hours, retrieves all timezone names using themomentlibrary, and filters them based on their UTC offset. The logic is sound and should return the expected list of timezone names matching the provided offset.web/src/components/alerts/ScheduledAlert.vue (3)
303-305: LGTM!The change to use a separate
filterFieldsmethod for handling the@filterevent looks good. It keeps the template cleaner.
Line range hint
703-764: Looks good! The cron expression and timezone support is a nice addition.The changes to support configuring cron expressions with timezone look great. Defaulting to the browser timezone and UTC, and providing timezone search functionality enhances the user experience.
Line range hint
783-790: The updated frequency validation looks good!The changes to validate both minutes and cron frequency types, and showing an error for empty cron expressions is correct.
web/src/components/alerts/AddAlert.vue (4)
204-204: LGTM!The
v-modelbinding fortimezoneallows theAddAlertcomponent to manage the timezone data and pass it to thescheduled-alertcomponent. This change looks good.
461-461: LGTM!The imports for
getTimezonesByOffsetandconvertDateToTimestamputility functions look good. They are likely used for handling timezone selection and conversion logic in the component.Also applies to: 470-470
Line range hint
1245-1282: LGTM!The changes for setting the
timezonedata in theformDataobject look good. They ensure that thetimezoneis properly set, either from the existingtimezonevalue or by deriving it from thetz_offsetvalue using thegetTimezonesByOffsetfunction.
1356-1381: LGTM!The changes for constructing a timestamp based on the current date, time, and selected timezone when the alert's frequency type is "cron" and
is_real_timeis false look good. The resulting timestamp and offset are properly stored in theformDataobject.web/src/components/reports/CreateReport.vue (1)
425-452: Looks good! Please address the minor suggestions.The addition of the timezone selection functionality using the
q-selectcomponent is implemented correctly. The validation ensures that a timezone value is always provided.A few additional points to consider:
The
convertDateToTimestampfunction has been removed in this change. Please verify if the selected timezone is being correctly used for scheduling the report without this function. If not needed, the removal of the function is fine.The
@blurevent handler logic can be simplified by directly assigning the browser's default timezone instead of using a ternary operator. Apply this diff:- timezone = - timezone == '' - ? Intl.DateTimeFormat().resolvedOptions().timeZone - : timezone + timezone = timezone || Intl.DateTimeFormat().resolvedOptions().timeZone
| export const convertDateToTimestamp = ( | ||
| date: string, | ||
| time: string, | ||
| timezone: string, | ||
| ) => { | ||
| const browserTime = | ||
| "Browser Time (" + Intl.DateTimeFormat().resolvedOptions().timeZone + ")"; | ||
|
|
||
| const [day, month, year] = date.split("-"); | ||
| const [hour, minute] = time.split(":"); | ||
|
|
||
| const _date = { | ||
| year: Number(year), | ||
| month: Number(month), | ||
| day: Number(day), | ||
| hour: Number(hour), | ||
| minute: Number(minute), | ||
| }; | ||
|
|
||
| if (timezone.toLowerCase() == browserTime.toLowerCase()) { | ||
| timezone = Intl.DateTimeFormat().resolvedOptions().timeZone; | ||
| } | ||
|
|
||
| // Create a DateTime instance from date and time, then set the timezone | ||
| const dateTime = _DateTime.fromObject(_date, { zone: timezone }); | ||
|
|
||
| // Convert the DateTime to a Unix timestamp in milliseconds | ||
| const unixTimestampMillis = dateTime.toMillis(); | ||
|
|
||
| return { timestamp: unixTimestampMillis * 1000, offset: dateTime.offset }; // timestamp in microseconds | ||
| }; |
There was a problem hiding this comment.
Improve input validation, timezone handling, and error handling.
The convertDateToTimestamp function could be improved in the following areas:
-
Input Validation:
- Add validation for the date and time string formats to ensure they are in the expected format (DD-MM-YYYY for date, HH:MM for time).
- Handle invalid timezone strings gracefully and provide appropriate error messages.
-
Timezone Handling:
- Clarify the timezone matching logic and consider using strict equality instead of case-insensitive comparison.
- Document the behavior when the provided timezone matches the browser's timezone.
-
Error Handling:
- Implement error handling for invalid inputs and DateTime creation failures.
- Throw appropriate errors or return error values to indicate failures.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
web/src/components/alerts/AddAlert.vue (1)
1353-1378: Useconsole.logfor logging and improve the logged message.The logic for constructing the timestamp based on the current date and time, and converting it using the selected timezone is correct.
However, please consider the following suggestions:
- Use
console.loginstead ofalertfor logging.- Improve the logged message to provide more context. For example:
console.log(`Converted timestamp: ${convertedDateTime.timestamp}, Offset: ${convertedDateTime.offset}`);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/common/meta/alerts/mod.rs (1 hunks)
- web/src/components/alerts/AddAlert.vue (6 hunks)
- web/src/components/alerts/ScheduledAlert.vue (9 hunks)
Additional context used
Path-based instructions (1)
src/common/meta/alerts/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (8)
src/common/meta/alerts/mod.rs (1)
40-41: LGTM!The addition of the optional
timezonefield to theTriggerConditionstruct looks good. It aligns with the PR objective of introducing a timezone key to alert components. The field type and serde annotation are used appropriately.web/src/components/alerts/ScheduledAlert.vue (5)
303-305: Refactoring the filter event handler is a good improvement.Delegating the filtering logic to the
filterFieldsmethod and utilizing theupdatefunction for asynchronous updates aligns with Vue.js best practices. It improves code organization and maintainability.
Line range hint
703-764: The cron frequency configuration is a valuable addition.The code segment introduces the functionality to configure cron-based frequency for alerts, providing more flexibility and customization options. The toggle between "minutes" and "cron" frequency types, along with the cron input field and timezone selection, enhances the user experience and allows for precise scheduling.
The use of the
filteredTimezonecomputed property andtimezoneFilterFnmethod for filtering timezone options based on user input is a nice touch that improves usability.Overall, the code follows Vue.js best practices and effectively implements the cron frequency configuration feature.
Line range hint
783-789: The error message for invalid frequency input is a good addition.The code segment adds validation and error handling for the frequency input fields. It displays an error message when the frequency value is invalid based on the selected frequency type ("minutes" or "cron").
The use of the
v-showdirective is appropriate for conditionally rendering the error message, and the error message itself provides clear feedback to the user about the required field.This addition enhances the user experience by providing immediate feedback and guiding the user to enter valid frequency values.
Line range hint
892-938: The timezone handling enhancements are well-implemented.The code segment significantly improves the timezone handling functionality of the component. By utilizing the
Intl.supportedValuesOf("timeZone")method, it ensures compatibility across different browsers and systems, providing a comprehensive list of supported timezones.The
filteredTimezonecomputed property andtimezoneFilterFnmethod allow for dynamic filtering of the timezone options based on user input, enhancing the user experience when searching for a specific timezone.The inclusion of the user's current timezone (
currentTimezoneandbrowserTimezone) as default selections, along with the addition of the "UTC" and browser timezone options at the beginning of thetimezoneOptionsarray, makes it convenient for users to choose relevant timezones.Overall, these enhancements demonstrate a thoughtful approach to timezone handling, prioritizing user experience and flexibility.
1064-1093: The generic filtering methods improve code reusability and user experience.The introduction of the
filterFieldsandfilterColumnsmethods significantly enhances the filtering functionality of the component. By designing these methods to be more generic and reusable, the code becomes more maintainable and applicable to different scenarios.The
filterColumnsmethod's ability to handle both string and object types within the options array demonstrates its versatility and adaptability. The case-insensitive search functionality further improves the user experience by allowing users to filter options regardless of the casing.The use of the
updatefunction for asynchronous updates to the filtered options ensures a smooth and responsive user interface, providing a seamless filtering experience.Moreover, the code is well-structured and follows good practices, such as using meaningful variable names and providing comments for clarity, which enhances its readability and maintainability.
Overall, these improvements in the filtering functionality contribute to better code reusability, user experience, and code quality.
web/src/components/alerts/AddAlert.vue (2)
Line range hint
460-469: LGTM!The imports are relevant to the PR objective and are imported from the correct locations.
Line range hint
1244-1278: LGTM!The logic for setting the default timezone based on
tz_offsetis implemented correctly and aligns with the PR objective.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
web/src/components/pipeline/ScheduledPipeline.vue (1)
1088-1117: Column filtering logic update looks good!The modification to the
filterFieldsfunction aligns with the description in the summary, ensuring that the filtering mechanism is robust and versatile by handling both string and object types.Consider removing the console log statement if it's not needed for debugging purposes:
- console.log("options", options);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- web/src/components/pipeline/ScheduledPipeline.vue (10 hunks)
- web/src/components/pipeline/StreamRouting.vue (6 hunks)
Additional comments not posted (11)
web/src/components/pipeline/StreamRouting.vue (5)
205-209: LGTM!The new imports are relevant to the timezone handling changes and are used in the subsequent code. Good job!
218-218: LGTM!The new import is relevant to the timezone handling changes and is used in the subsequent code. Good job!
253-253: LGTM!The new
timezoneproperty in thetrigger_conditioninterface is consistent with the PR objective and has an appropriate type.
258-258: LGTM!The new optional
tz_offsetproperty in theStreamRouteinterface is appropriate to store the timezone offset.
350-350: Excellent work on handling the timezone selection and conversion logic!
- Setting the default timezone to "UTC" in
trigger_conditionis a good practice.- The logic in the
onMountedhook to set thetimezonebased ontz_offsetis implemented correctly. Defaulting to "UTC" whentz_offsetis zero and fetching the appropriate timezone usinggetTimezonesByOffsetotherwise is a smart approach.- The additional logic in
getRoutePayloadfor handling the "cron" frequency type ensures that the timezone offset is computed accurately by converting the current date and time into a timestamp format based on the specified timezone.Overall, the changes effectively handle the timezone selection and conversion. Well done!
Also applies to: 377-385, 638-665
web/src/components/pipeline/ScheduledPipeline.vue (6)
262-262: LGTM!The renaming of
filterColumnstofilterFieldsaligns with the shift in focus from columns to fields and does not introduce any issues.
Line range hint
527-631: Timezone selection feature looks good!The implementation of the timezone selection feature aligns with the description provided in the summary. The code segment does not introduce any apparent issues or inconsistencies. The use of
browserTimezoneas the default and the filtering of timezone options enhance the user experience.
Line range hint
684-761: Warning message for period and cron expression match looks good!The addition of the warning message improves user guidance and helps reduce potential errors. The warning message is displayed conditionally based on the appropriate criteria, and the code segment does not introduce any apparent issues or inconsistencies.
Line range hint
784-907: Timezone handling setup looks good!The code segment sets up the necessary variables and functions for timezone handling. The use of the
useLocalTimezoneutility function ensures that the current timezone is correctly determined. The inclusion of the browser's timezone and the UTC option in thetimezoneOptionsarray provides relevant choices for users. ThetimezoneFilterFnfunction enables dynamic filtering of timezone options based on user input. The code segment does not introduce any apparent issues or inconsistencies.
886-889: Timezone initialization looks good!The initialization of
currentTimezoneandbrowserTimezoneis consistent with the timezone handling setup described in the summary. The use of theuseLocalTimezoneutility function ensures that the current timezone is correctly determined. The code segment does not introduce any apparent issues or inconsistencies.
891-903: Timezone options initialization looks good!The initialization of
timezoneOptionswith supported timezone values ensures that the available options are comprehensive and up to date. The addition of the "UTC" and "Browser Time" options provides relevant choices for users. The code segment does not introduce any apparent issues or inconsistencies.
omkarK06
force-pushed
the
alert_timezone
branch
from
7abbcb9 to
92402b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
web/src/components/pipeline/ScheduledPipeline.vue (1)
1101-1101: Remove console logging before merging.The console logging of the
optionsarray can be useful for debugging purposes during development. However, it should be removed before merging the changes to production to avoid unnecessary logging in the console.- console.log("options", options);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/common/meta/alerts/mod.rs (1 hunks)
- web/src/components/alerts/AddAlert.vue (6 hunks)
- web/src/components/alerts/ScheduledAlert.vue (9 hunks)
- web/src/components/pipeline/ScheduledPipeline.vue (10 hunks)
- web/src/components/pipeline/StreamRouting.vue (6 hunks)
- web/src/components/reports/CreateReport.vue (2 hunks)
- web/src/utils/date.ts (2 hunks)
- web/src/utils/zincutils.ts (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- src/common/meta/alerts/mod.rs
- web/src/components/alerts/AddAlert.vue
- web/src/components/alerts/ScheduledAlert.vue
- web/src/components/pipeline/StreamRouting.vue
- web/src/utils/date.ts
- web/src/utils/zincutils.ts
Additional comments not posted (10)
web/src/components/pipeline/ScheduledPipeline.vue (9)
262-262: LGTM!The renaming of the event handler from
filterColumnstofilterFieldsis appropriate as it reflects a broader context of filtering fields rather than just columns.
527-527: LGTM!The addition of the
q-togglecomponent to enable/disable a cron job enhances the functionality and the implementation looks correct.
Line range hint
587-648: LGTM!The changes introduce the ability to configure the frequency using either a numeric input for minutes or a cron expression, along with a timezone select dropdown for the cron job. The implementation looks correct and enhances the scheduling functionality.
684-684: LGTM!The addition of the
q-pb-smclass to the period titledivimproves the spacing and readability. The change is minor but appropriate.
754-761: LGTM!The addition of the warning message to inform users that the period should align with the cron expression is a good enhancement. It helps prevent potential misconfigurations and improves user guidance.
784-784: LGTM!The import of the
useLocalTimezoneutility function from@/utils/zincutilsis correct and suggests that it will be used to handle timezone-related functionality.
Line range hint
848-907: LGTM!The changes introduce timezone-related functionality to the component. The reactive variables
filteredTimezone,currentTimezone, andbrowserTimezonehandle the timezone selection and storage. ThetimezoneOptionsarray is populated with supported timezone values and additional options. ThetimezoneFilterFnfunction provides dynamic filtering of timezone options based on user input. The implementation looks correct and enhances the timezone selection feature.
1088-1090: LGTM!The update in the
filterFieldsfunction to use thefilterColumnsfunction for filtering the fields based on user input is appropriate. It promotes code reuse and maintains consistency in the filtering logic.
1092-1117: LGTM!The changes in the
filterColumnsfunction to handle both object and string types for filtering enhance its functionality and make the filtering mechanism more robust and flexible.web/src/components/reports/CreateReport.vue (1)
425-452: LGTM!The new timezone selection feature looks good:
- Allows users to select a timezone, enhancing the user experience.
- Defaults to the browser's timezone if not provided by the user.
- Properly validates the timezone selection.
- Correctly binds the selected timezone to the
scheduling.timezonemodel.
omkarK06
force-pushed
the
alert_timezone
branch
from
4c13018 to
ad08f6e
Compare
#4541
Summary by CodeRabbit
Summary by CodeRabbit
New Features
timezonefield in alert and trigger condition configurations for enhanced flexibility.q-selectcomponent for dynamic timezone selection, allowing users to filter available timezones.Bug Fixes