Timezone in App #2581
Timezone in App #2581
Conversation
WalkthroughThe pull request introduces significant changes to the handling of date and time within the GraphQL operations of the application. It adds new utility functions for date-time conversions, modifies existing methods to incorporate these conversions, and updates the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if either of these two conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
Other🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (3)
test/widget_tests/after_auth_screens/events/time_conversion_test.dart (2)
17-19: Consider adding more test cases for combineDateTimeWhile the current test case verifies the basic functionality of
combineDateTime, consider adding more test cases to cover edge cases and different input formats. This could include:
- Combining date and time with different formats (e.g., AM/PM time)
- Handling of invalid inputs
- Behavior with extreme date/time values
Example:
test('combineDateTime handles various input formats', () { expect(combineDateTime('2023-05-01', '02:30:00 PM'), '2023-05-01 14:30:00'); expect(combineDateTime('2023-12-31', '23:59:59'), '2023-12-31 23:59:59'); // Add more assertions for edge cases });
21-31: Enhance datetime splitting tests and fix local time format
Consider adding more test cases for
splitDateTimeUTCandsplitDateTimeLocalto cover various scenarios, including edge cases and different input formats.In the
splitDateTimeLocaltest, the expected time format seems inconsistent with the UTC test. The local time is expected to be in "HH:mm" format, while the UTC time includes seconds and milliseconds. Ensure this is the intended behavior.Example of additional test cases:
test('splitDateTimeUTC handles various input formats', () { expect(splitDateTimeUTC('2023-12-31T23:59:59.999Z'), {'date': '2023-12-31', 'time': '23:59:59.999Z'}); // Add more assertions for edge cases }); test('splitDateTimeLocal handles various input formats', () { expect(splitDateTimeLocal('2023-12-31T23:59:59.999'), {'date': '2023-12-31', 'time': '23:59'}); // Add more assertions for edge cases });lib/utils/time_conversion.dart (1)
3-5: Consider validating input incombineDateTimeThe
combineDateTimefunction concatenatesdateandtimewithout validation. If either parameter is empty or improperly formatted, it may lead to issues downstream. Consider adding validation or formatting to ensure the combined string is a valid date-time.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- lib/services/database_mutation_functions.dart (5 hunks)
- lib/services/event_service.dart (1 hunks)
- lib/utils/time_conversion.dart (1 hunks)
- lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart (1 hunks)
- test/widget_tests/after_auth_screens/events/time_conversion_test.dart (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
test/widget_tests/after_auth_screens/events/time_conversion_test.dart (2)
1-15: LGTM: Well-structured test setupThe import statements and the main test structure are well-organized. The use of
setUpandtearDownmethods for service registration is a good practice for maintaining a clean test environment.
1-108: Overall assessment: Good foundation, room for improvementThe test suite provides a solid foundation for verifying the time conversion utilities. It covers the basic functionality of various conversion methods and handles different data structures. However, there are several opportunities to enhance the test coverage and robustness:
- Add more test cases to cover edge cases and different input formats.
- Verify the actual conversion logic, not just the output format.
- Use fixed timezones and known input/output pairs for more precise testing.
- Enhance tests for complex nested structures and lists.
- Include tests for error handling and invalid inputs.
Implementing these suggestions will significantly improve the reliability and comprehensiveness of the test suite, ensuring that the time conversion utilities work correctly across various scenarios.
lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart (1)
Line range hint
1-438: Review overall timezone implementation in CreateEventViewModelThe changes in this file are limited to the
createEventmethod, specifically the formatting ofstartTimeandendTime. While this change aligns with the PR objective of implementing timezone handling, there are a few concerns:
- The
eventStartTime,eventEndTime,eventStartDate, andeventEndDateproperties are not modified to account for timezone differences. Should these be adjusted as well?- The
recurrenceStartDateandrecurrenceEndDatehandling remains unchanged. Do these need to be considered in the timezone implementation?- There's no visible logic for handling user-specific timezone preferences. How is the user's timezone being determined and applied?
To ensure a comprehensive implementation of the timezone feature, consider reviewing these points and adding any necessary logic or comments to clarify the timezone handling approach throughout the
CreateEventViewModelclass.To verify the completeness of the timezone implementation, please run the following script:
#!/bin/bash # Description: Analyze timezone-related code in CreateEventViewModel and related files # Test 1: Search for timezone-related code in CreateEventViewModel echo "Searching for timezone-related code in CreateEventViewModel:" rg --type dart "(?i)timezone|utc|local.*time" lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart # Test 2: Search for user timezone preference handling echo "\nSearching for user timezone preference handling:" rg --type dart "(?i)user.*timezone|timezone.*preference" # Test 3: Search for date and time parsing in related event files echo "\nSearching for date and time parsing in related event files:" rg --type dart "(?i)parse.*time|parse.*date|format.*time|format.*date" lib/view_model/after_auth_view_models/event_view_models/This script will help us identify any missing pieces in the timezone implementation and ensure that all necessary components are properly handled.
lib/utils/time_conversion.dart (1)
24-24: Verify the date-time format stringsThe format strings used in
convertUTCToLocalandconvertLocalToUTCmay not fully align with standard ISO 8601 formats. Ensure that the patterns correctly represent the intended formats, including milliseconds and timezone indicators.Run the following script to review all
DateFormatpatterns used:Also applies to: 29-29
lib/services/database_mutation_functions.dart (3)
9-10: LGTMThe new imports for
connectivity_view_model.dartandtime_conversion.dartare appropriate and necessary for the added date conversion functionality.
102-103: LGTMThe added code correctly converts dates from UTC to local time in the query results using
traverseAndConvertDates. This ensures that all date-time data in queries is consistent with the client's local time.
222-223: LGTMThe added code effectively converts dates from UTC to local time in the query results for non-authenticated queries using
traverseAndConvertDates. This maintains consistency in date-time handling across the application.
test/widget_tests/after_auth_screens/events/time_conversion_test.dart
Outdated
Show resolved
Hide resolved
test/widget_tests/after_auth_screens/events/time_conversion_test.dart
Outdated
Show resolved
Hide resolved
test/widget_tests/after_auth_screens/events/time_conversion_test.dart
Outdated
Show resolved
Hide resolved
lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lib/utils/time_conversion.dart (1)
19-19: Consider standardizing time formats for consistencyIn
splitDateTimeLocal, the time format is'HH:mm', which includes hours and minutes. InsplitDateTimeUTC, the time format is'HH:mm:ss.SSS\'Z\'', including hours, minutes, seconds, and milliseconds. For consistency, consider including seconds and milliseconds in both local and UTC time formats, or document the reason for the difference.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- lib/services/database_mutation_functions.dart (5 hunks)
- lib/services/event_service.dart (1 hunks)
- lib/utils/time_conversion.dart (1 hunks)
- lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/services/database_mutation_functions.dart
- lib/services/event_service.dart
- lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart
🧰 Additional context used
🔇 Additional comments (6)
lib/utils/time_conversion.dart (6)
8-8: Add error handling for date parsing insplitDateTimeUTC
16-16: Add error handling for date parsing insplitDateTimeLocal
24-24: Add error handling for date parsing inconvertUTCToLocal
29-29: Add error handling for date parsing inconvertLocalToUTC
45-46: Ensure safe casting of date and time fields
61-61: Handle non-string values indirectFieldsconversion
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
- lib/services/database_mutation_functions.dart (5 hunks)
- lib/utils/time_conversion.dart (1 hunks)
- lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart (1 hunks)
- pubspec.yaml (1 hunks)
- test/widget_tests/after_auth_screens/events/time_conversion_test.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/services/database_mutation_functions.dart
- lib/utils/time_conversion.dart
- lib/view_model/after_auth_view_models/event_view_models/create_event_view_model.dart
- test/widget_tests/after_auth_screens/events/time_conversion_test.dart
🧰 Additional context used
🔇 Additional comments (2)
pubspec.yaml (2)
24-24: Excellent addition of theclockpackage for timezone handling.The addition of
clock: ^1.1.1is a great choice for implementing timezone features. This package allows for easy mocking of the system clock, which will be invaluable for testing time-dependent code, especially when dealing with different timezones.Consider utilizing this package in your tests to ensure the robustness of your timezone conversion logic across various scenarios.
Line range hint
18-23: Verify the intention behind removing analysis and linting dependencies.The following dependencies have been removed or commented out:
_fe_analyzer_shared: ^60.0.0analyzeranalyzer_plugincustom_lint_builder: ^0.4.0While these removals don't directly impact the timezone feature implementation, they are typically used for static analysis and linting. Removing them might affect code quality checks. Can you confirm if this was intentional and if there are alternative tools in place for maintaining code quality?
Also applies to: 32-34
✅ Verification successful
Linting and analysis dependencies have been appropriately updated.
The removal of older linting tools has been effectively replaced by the addition of
custom_lint,lint, andtalawa_lint. This ensures that code quality checks are maintained with up-to-date tools.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any remaining linting or analysis tools rg --type yaml 'lint|analyzer' pubspec.yamlLength of output: 219
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2581 +/- ##
===========================================
+ Coverage 96.45% 96.46% +0.01%
===========================================
Files 179 180 +1
Lines 9191 9227 +36
===========================================
+ Hits 8865 8901 +36
Misses 326 326 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
test/service_tests/event_service_test.dart (4)
Line range hint
255-292: Add error handling for null or unexpected responsesIn the
Test createVolunteerGroup method, consider adding assertions to handle cases where the response data might be null or missing expected fields. This will make the test more robust against unexpected API responses.
Line range hint
255-338: Refactor duplicate setup code in test methodsMultiple test methods have duplicated setup code for
dataBaseMutationFunctions,query, and instantiation ofEventService. To adhere to the DRY principle and improve maintainability, consider extracting common setup steps into asetUpmethod provided by thetestpackage.
Line range hint
255-338: Enhance test coverage with edge casesThe newly added tests focus on successful executions. To improve test coverage, consider adding tests for edge cases such as:
- API returning error responses
- Unexpected data formats
- Exception handling when inputs are invalid
This ensures the methods behave correctly under all circumstances.
Line range hint
255-338: Simplify unnecessary casting in test assertionsIn your test assertions, you're casting
resulttoQueryResulteven though it is already of that type. This casting is unnecessary and can be removed to simplify the code.Apply this diff to remove unnecessary casts:
expect(result, isNotNull); - expect( - (result as QueryResult).data!['createVolunteerGroup']['_id'], + expect( + result.data!['createVolunteerGroup']['_id'], 'groupId1', );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- test/service_tests/event_service_test.dart (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
test/service_tests/event_service_test.dart (4)
122-122: Corrected typo in 'createdEvent' keyGood job fixing the typo from
'cretedEvent'to'createdEvent', ensuring the test checks the correct response key.
141-141: Variables map now includes 'eventId'Including
'eventId'in the variables map for theregisterForAnEventtest ensures that the mutation receives the necessary parameter, enhancing test accuracy.
147-147: Updated response key to 'registerForEvent'The response data now correctly uses the
'registerForEvent'key, matching the expected mutation response.
Line range hint
255-292: Verify variable structure matches GraphQL mutation requirementsIn the
createVolunteerGrouptest, ensure that the variables structure aligns with what the GraphQL mutation expects. The current structure usesvariables: {'data': variables}. Confirm that'data'is the correct key expected by the backend.Run the following script to check the expected variable structure in the GraphQL schema:
✅ Verification successful
Verified: The
variablesstructure in thecreateVolunteerGrouptest correctly uses the'data'key as expected by the GraphQL mutation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the expected input structure for createVolunteerGroup mutation. # Test: Search for the createVolunteerGroup mutation definition. Expect: The correct variable structure. rg --type graphql 'mutation.*createVolunteerGroup' -A 10Length of output: 90
Script:
#!/bin/bash # Description: Verify the expected input structure for createVolunteerGroup mutation across all relevant files. # Search for the createVolunteerGroup mutation definition in Dart files. rg 'mutation\s+createVolunteerGroup' --type dart -A 10Length of output: 56
Script:
#!/bin/bash # Description: Broadly search for the createVolunteerGroup mutation definition across all files. # Search for the createVolunteerGroup mutation in all files. rg 'createVolunteerGroup' -A 10Length of output: 24329
|
Please try to get the code coverage up so that the tests pass. |
|
The coverage is low mainly due to changes in I will be adding lines ignore for the some lines in
Yes, it's similar to Talawa-Admin. Now it's a middleware to change the time. I've tested this and now whenever I make a new event using IST time then it converts and saves in UTC in database, and at the time of viewing any event - it automatically converts back to IST (my local time) from UTC. So, for a user it's always shown in local time for them, but we store in UTC in database. |
Just dont make these changes in your PR, I will do that in mine and add tests too |
|
also please attach a clip of app working properly after making these changes to ensure proper working |
|
Event creation in local time: Saved in UTC in database: |
|
@noman2002 PTAL. The aim is for the mobile app to interact with the API only using UTC times. This will help with converting calendar entries to the correct local time when the end user could be in a different timezone from the client. Work in Admin and API has already been completed |
palisadoes
merged commit b5d02c4
into
PalisadoesFoundation:develop
|
@palisadoes @pranshugupta54 This PR breaks the app, no server call are successful after this got merged. need to revert it. |
|
@Dante291, did you try making the changes in that event file. This PR won't work without those changes. |
Yup I did that, also those changes were related to event service only so even if anything breaks if some error is occurring due to that, it should be only related to events, but after this got merged neither user data is loading nor the posts, basically nothing. |
|
I tried running it, looks good to me. Let's discuss this on Slack first and if there is some issue then we can fix it. |
|
Looks like some issues due to unhandled catch blocks. Quick Fix: #2592 cc: @palisadoes |
|
It's merged |
What kind of change does this PR introduce?
Feature: timezone
Issue Number:
Fixes #2558
Did you add tests for your changes?
yes
Summary
Solution: Instead of making significant changes to the Mobile/API repositories, I modified how the GraphQL API interacts with Apollo. Now, we're using middleware to handle time conversions: function converts UTC to the local time zone during queries, and function converts local time back to UTC during mutations.
Alternative Approach: I initially spent a lot of time on a different approach that we had discussed and planned earlier according to our issue and project planning. We had planned to change the database by introducing a timezone field and then saving data based on the organization. However, since we don't intend to give users the option to switch timezones like Google Calendar, we can directly modify the API requests and responses to implement this.
Does this PR introduce a breaking change?
NO
Other information
NA
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests