Timestring is not being properly set on Refreshing the tab when the Language is English

[aman-atg]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Details

#4218 (comment)

Action Performed:

1. Go to newDot
2. Open any chat
3. Refresh the page
4. timestring will update from Spanish to English even when the Language is set to English

Expected Result:

It should show the timestring in the language that is set.

Actual Result:

bug.mp4

Workaround:

Visual Issue.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @lschurr (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[aman-atg]

Proposal

Explaination

• This is a regression which is most likely caused by this 72800fc
• Earlier we were getting preferredLocale inside props when we used withLocalize but after the above commit this.props.preferredLocale became undefined
• And then locale was passed inside props for child comps. instead of preferredLocale after this 69fd992
• This is causing the moment.locale to take undefined as an argument here which was defined earlier to these commits
  

  App/src/pages/home/report/ParticipantLocalTime.js

  Line 44 in f772837

  moment.locale(this.props.preferredLocale);

Solution

• We just need to rename locale to preferredLocale on this line
  

  App/src/components/withLocalize.js

  Line 59 in f772837

  locale: this.props.preferredLocale,

[parasharrajat]

@aman-atg would you mind commenting of the PR that caused the regression? It will help everyone.

Thanks for taking the time to find the issue and creating a ticket for it

[aman-atg]

@parasharrajat It's the same PR that caused #4352. The issue it solved is #4268.
The last PR also couldn't fix it.
I have mentioned commits here #4404 (comment) for a better understanding

[parasharrajat]

Yeah, that's why to fix this regression another PR should be made by the original author. Don't you think so?

Could you please mention/tag the respective persons who found the original issue?

[arpitdeveloper]

In this case we update "locale: this.props.preferredLocale"
to "preferredLocale: this.props.preferredLocale"
in "App/src/components/withLocalize.js"

[aman-atg]

Yeah, that's why to fix this regression another PR should be made by the original author. Don't you think so?

There're many instances when regressions are fixed as Separate issues when found out later after the PR is merged.
But I don't think that I'm eligible to answer that question. Someone will surely clarify later.

Could you please mention/tag the respective persons who found the original issue?

I've already provided context in the ticket itself.

Details

#4218 (comment)

[lschurr]

Is this a duplicate of this one then? #4218

[aman-atg]

No. In that issue we were not showing Spanish version at all.
And currently the English version is being updated with a delay in this Issue.

[parasharrajat]

@lschurr I see that Changes here #4366 caused this issue.

As @aman-atg mentioned #4404 (comment).
That prop name was renamed in that PR to locale.

[MelvinBot]

Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[lschurr]

Tagging Eng for another set of eyes.

[Luke9389]

@parasharrajat is correct in saying this should be fixed by the original author. We always wait 7 days to pay our contributors so that we can find regressions like this one. The PR we've been linking to is actually another regression fix from this original one, which was only merged 5 days ago.

Thus I think @kidroca should make this change (it's a tiny one-liner change but that's our process).

[kidroca]

@aman-atg
Thanks for the info, I'll submit another PR
I've missed that previously preferredLocale was along the ...props spread
I've only passed locale to trigger re-render when it changes, I had no idea it was actually used in the child components...

[aman-atg]

@kidroca
No problem mate!

[MelvinBot]

This issue has not been updated in over 15 days. @Luke9389 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!