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

add user locale field to mobile app user settings table + change going on call push notification text #2131

Merged

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Jun 8, 2023

What this PR does

  • add user locale field to mobile app user settings table + add a test that sends PATCH requests to this endpoint
  • change "you're going on call" push notification text to include localized shift time. The general format is now:
    f"You're going on call in {time_until_going_oncall} for schedule {schedule.name}, {formatted_shift}"
    • time_until_going_oncall is a "human-readable" format of the time until the start)
    • schedule.name is self-explanatory
    • formatted_shift this depends on the shift. If the shift starts and ends on the same day, the format will be "HH:mm - HH:mm". Otherwise, if the shift starts and ends on different days, the format will be "YYYY-MM-DD HH:mm - YYYY-MM-DD HH:mm". Note that all datetime related formatting will use the new locale field that we are now storing in the mobile app user settings table. If no locale is yet present we will fallback to "en"

Which issue(s) this PR fixes

closes #2024
https://github.com/grafana/oncall-mobile-app/issues/187

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@joeyorlando joeyorlando added the pr:no public docs Added to a PR that does not require public documentation updates label Jun 8, 2023
@joeyorlando joeyorlando requested a review from a team June 8, 2023 08:49
@joeyorlando joeyorlando changed the title add user locale field to mobile app user settings table add user locale field to mobile app user settings table + change going on call push notification text Jun 8, 2023
@joeyorlando joeyorlando marked this pull request as ready for review June 8, 2023 13:53
@joeyorlando joeyorlando requested a review from a team as a code owner June 8, 2023 13:53

assert (
multiple_day_shift_title
== f"You're going on call in {humanized_time_until_going_oncall} for schedule {schedule_name}, 2023-07-08 09 h 00 - 2023-07-12 17 h 00"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

09 h 00 looks a bit weird IMO, maybe it should be 09:00 or at least 09h00?
I'm not an expert in locales, but I can't see anything similar to this format in the reference for fr-CA.

)
assert (
multiple_day_shift_no_locale_title
== f"You're going on call in {humanized_time_until_going_oncall} for schedule {schedule_name}, 7/8/23, 9:00\u202fAM - 7/12/23, 5:00\u202fPM"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use code \u20 instead of regular " " spaces?

@joeyorlando
Copy link
Contributor Author

@vadimkerr good questions 👍 I agree with your points. I'll need to see if I can configure the babel package to modify these behaviours.

table + change going on call push notification text
@joeyorlando joeyorlando force-pushed the jorlando/add-user-locale-field-to-mobile-app-settings branch from 4bc09d6 to ea0ac2c Compare June 13, 2023 11:03
@joeyorlando
Copy link
Contributor Author

@vadimkerr these two points you raised seem to be coming from the babel library itself. I've raised two questions over in that repo to clarify this behaviour.

\u202f character
python-babel/babel#1012
We could do a string replacement on this character, but that seems flaky imo.

Odd spacing in time formatting w/ fr_CA locale
python-babel/babel#1013

It appears like this is the default formatting behaviour for fr_CA?

>>> babel.dates.format_time(now, format="short", locale="fr_CA")
'15 h 27'
>>> babel.dates.format_time(now, format="short", locale="fr")
'15:27'
>>> babel.dates.format_time(now, format="short", locale="en")
'3:27\u202fPM'

I don't think overriding the datetime formatting pattern syntax would be useful here, because that would default the purpose of localizing the datetimes in the first place 😕

@joeyorlando
Copy link
Contributor Author

@vadimkerr these formatting decisions seem to be the default coming from Unicode CLDR. I suggest for now we stick with these and refine them over time if need be. Overriding them to handle different formats/locales, to suit our liking, seems like a lot of work.

Regarding the \u202f unicode characters that babel returns. I verified that Android renders these properly, I presume iOS does as well. For example, when 6/13/23, 10:00\u202fPM - 6/14/23, 10:00\u202fPM is sent to FCM as the notification title:
Screenshot 2023-06-14 at 18 00 22

@joeyorlando joeyorlando merged commit 572131b into dev Jun 14, 2023
24 of 25 checks passed
@joeyorlando joeyorlando deleted the jorlando/add-user-locale-field-to-mobile-app-settings branch June 14, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shift info push notification tweaks
3 participants