-
Notifications
You must be signed in to change notification settings - Fork 14
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
Suppress TimeZone info on TimeData questions #942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! This would change for mobile as well right ? Also wondering if time values can be used in any calculations that might break because of the format change. If yes, may be it's worth announcing this change on commcare forums.
Interesting, would this fix this issue https://dimagi-dev.atlassian.net/browse/SAAS-11477 |
@knguyen142 Oh, almost certainly, I forgot this was something that was being actively used there. I'll wrap up the formplayer side of this now so it's ready to go. One silly thing is that this will really only be important for 2 more days, since we're about to flip back off of DST, but no reason to not get it resolved ASAP. |
Yes it'd be nice to see if this fixes it so we can prepare for next year. Thanks @ctsims and my apologies for not realizing that this would affect the time question. |
@knguyen142 It's not your fault, the regression tests didn't catch the existing case since we don't have a strong way to test regressions currently in drifting locales . The original code shouldn't have been TZ dependent and was an existing bug that it was that just wasn't surfaced. This PR and dimagi/formplayer#778 should be good to go for rollout as soon as tests pass there. |
@shubham1g5 I will double check this. I think the tz formatting might have only been showing up on the web because of the explicitly set TZ provider, but I could be wrong (or it could only affect half of the problem but not the other). You are right that if the field output changes we should let people know. |
The ticket Kevin linked does show that forms submitted via mobile are including time zone info when saving a time question to a case property. |
A forum user identified that they were having "off by an hour" questions which end up blocking web apps from working correctly, because the server doesn't send back the same value for questions as the browser provided.
This is because the TimeData construct has been improperly carrying forward TZ info internally with the time value, despite it being a "WallClock" representation. Since the internal calendar date for the TimeDate Date is the Epoch, if the TimeZone offset in the current locale differs from the offset on the Epoch, dates would drift. Kevin's recent change to improve Timezone accuracy on formplayer had the unintended side effect of instructing the browser about the current Locale instead of the current offset, triggering the behavior there.
This change simply strips the TZ info when the question converts the internal value back out to a serialized representation, since the TZ (like the Date itself) isn't actually a part of the TimeData object.
Formplayer commit here: dimagi/formplayer#778