1715544 - Timezone converter's 12h correction logic is incorrect for times between 12pm and 1pm, and 12am and 1am

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

The logic that adds 12 hours to e.g. "1pm" to make it "13:00" in 24h format incorrectly adds this 12h offset to "12pm" too - thus if you try to convert "12pm pst in cet" you get "9am" as the result, instead of "9pm".

I'm making a patch.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Actually - Silke, I know you've been interested in working on some JS bugs, and this would be a great one to pick up if you have time!

The incorrect logic is here.

Basically, we are setting the "meridien shift" to 12h, if the time format was "xxPM" - but we don't exclude the "12pm" case. times that are in the 12:xx range are the same in 12h and 24h format. We need to exclude this case.

After changing the logic, we should add some cases to the list of testcases here to include this scenario.

If you want to work on this, feel free to assign yourself. :)

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Daisuke, looks like this bug is in beta too, but is suppressed by the fact that unit conversion is not enabled on beta yet. Is there any plan to enable this on beta any time soon, or have some kind of rollout or experiment or something? Just curious if this issue needs any extra tracking.

Comment 3

[Silke Hofmann]

(In reply to Nihanth Subramanya [:nhnt11] from comment #1)

Actually - Silke, I know you've been interested in working on some JS bugs, and this would be a great one to pick up if you have time!

The incorrect logic is here.

Basically, we are setting the "meridien shift" to 12h, if the time format was "xxPM" - but we don't exclude the "12pm" case. times that are in the 12:xx range are the same in 12h and 24h format. We need to exclude this case.

After changing the logic, we should add some cases to the list of testcases here to include this scenario.

If you want to work on this, feel free to assign yourself. :)

Thanks Nihanth, I'm happy to work on this! :) Could you please assign it to me? I don't seem to have the rights to do so myself.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

This bug seems to affect times between 12am and 1am as well, updating the title to reflect this.

(In reply to Silke Hofmann from comment #3)

(In reply to Nihanth Subramanya [:nhnt11] from comment #1)
Thanks Nihanth, I'm happy to work on this! :) Could you please assign it to me? I don't seem to have the rights to do so myself.

Done.

Comment 5

[Daisuke Akatsuka (:daisuke)]

Hi Nihanth, thank you very much for finding the issue!

(In reply to Nihanth Subramanya [:nhnt11] from comment #2)

Daisuke, looks like this bug is in beta too, but is suppressed by the fact that unit conversion is not enabled on beta yet. Is there any plan to enable this on beta any time soon, or have some kind of rollout or experiment or something? Just curious if this issue needs any extra tracking.

We have one issue to enable unit conversion on the beta channel.
That is internationalization.
For unit conversion, we need to internationalize for not only output, but input.
For now, as we have not found a good way to internationalize especially for input, are supporting only "en-US". (Perhaps, need NLP module?)
When resolving this issue, we can enable it on the beta, I think.

Comment 6

[Silke Hofmann]

Attachment: Bug 1715544 - Extend timezone converter's 12h correction logic for times between 12pm and 1pm, and 12am and 1am. Add testcases. r=nhnt11

Comment 7

[Silke Hofmann]

Hey Nihanth, I'll provide some more info about the patch I've just submitted tomorrow. :)

Comment 8

[Silke Hofmann]

Thank you for reviewing the patch Nihanth! I’ve just submitted an update that includes the changes you suggested. Here is everything I’ve done so far:

• For times between 12am and 1am, inputHours and meridian shift are now set to zero
• For times between 12pm and 1pm, meridian shift is now set to zero
• For pm times with hours smaller than 12, meridian shift is now set to 12

I’ve added six test cases that cover the three cases above.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95cb20a0eee5
 - Extend timezone converter's 12h correction logic for times between 12pm and 1pm, and 12am and 1am. Add testcases. r=nhnt11

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/95cb20a0eee5

Comment 11

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I followed the details provided in the description on Fx 91.0a1(2021-06-29) and I can confirm this issue is fixed. Thank you!