Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove timezone offsetting from date range methods #2703

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shubham1g5
Copy link
Contributor

@shubham1g5 shubham1g5 commented Sep 12, 2023

Summary

This is to fix DateRangeUtilsTest which fails in certain timezones.

The date range method parseHumanReadableDate is used in case search date range widget to convert a text date range like 12-07-2023 to 14-07-2023 to time in milliseconds to set the range to the date widget. This method currently offsets the time in milliseconds for the current timezone. That doesn't seem correct to me as that can lead to dates being different on date widget in comparison to what is provided in the String. Instead we should just be returning the dates in current time zone so that they are consistent between their string manipulation and date widget.

This method itself was added in this PR for context and I can't seem to remember why I decided to offset the timezone earlier.

Feature Flag

CASE_SEARCH

Safety Assurance

  • If the PR is high risk, "High Risk" label is set
  • I have confidence that this PR will not introduce a regression for the reasons below
  • Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Automated test coverage

Change itself is to fix a test

Safety story

Change is Limited to date range widget used in case search

QA note: Check for date range widget in case search working correctly across a few different timezones.

@shubham1g5 shubham1g5 added this to the 2.53 milestone Sep 12, 2023
@shubham1g5 shubham1g5 requested a review from avazirna September 12, 2023 22:45
@avazirna
Copy link
Contributor

@shubham1g5 I might not have full context here, but I think this was done because we store dates in UTC in HQ, so the offsetting is to match the format in which the date is stored perhaps. But I agree that we probably don't need this as it's a date based search, so we don't care much about the timestamp, but we might want to check if the way we are querying is inclusive regarding the end date, i.e. we could be leaving cases out if in the comparison the end date is set as 12-07-2023 00:00:00 and not 12-07-2023 23:59:59 (or completely ignoring the timestamp).

@shubham1g5
Copy link
Contributor Author

@avazirna I don't think that's the reasoning there. We are passing these dates to the case search API as an inclusive range 15-03-2023_to_17_03_2023. Today this sometimes gets changed to +- 1 day such as 14-03-2023_to_16_03_2023 depending on the timezone which will remove some required results and add other unrequired results to the search results. So I don't think we want to do that kind of manipulation on results. The bigger issue here is it's not consistent with what user sees on the case search input (use sees a date like 15-03in the text field marked as 14/03 on the date widget)

@damagatchi retest this please

@shubham1g5 shubham1g5 modified the milestones: 2.53, 2.54, 2.55 Sep 18, 2023
@shubham1g5 shubham1g5 removed this from the 2.55 milestone Dec 9, 2024
@shubham1g5 shubham1g5 marked this pull request as draft December 9, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants