[Hold for Payment 6th August] Timezone information is not visible when app is in Spanish

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Navigate to a conversation with a user that has a different timezone than you
2. Change the app language to Spanish

Expected Result:

Timezone comment should be visible at any time.

Actual Result:

Timezone information is not visible when app is in Spanish. The message can be briefly seen just as the user changes the app to Spanish but it disappears after ~1sec

Workaround:

User can check the time zone in the user profile.

Platform:

Where is this issue occurring?

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

Version Number: 1.0.80-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Screenshot from video showing the translated text that's visible for an instant:

Recording.201.mp4

Upwork job URL:* https://www.upwork.com/jobs/~01c1138bff53cf206e

View all open jobs on Upwork

---

From @JmillsExpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1627011603184100

Someone's timezone is only visible on mobile, not the electron/web app, which creates a weird inconsistency depending on which platform you're on. 1.0.79-3

[MelvinBot]

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

[helaoutar]

This happens because inside src/pages/home/report/ParticipantLocalTime.js in getParticipantLocalTime(), moment.tz seems to be returning different results depending on the language:

• Engligh: AM/PM format
• Spanish: 24 hour format

This is causing ParticipantLocalTime component to not render, because it's conditioned by this variable:
const isReportRecipientLocalTimeReady = this.state.localTime.toString().match(/(A|P)M/ig); (this.state.localTime being the return value of getParticipantLocalTime).

Which is null when the language is Spanish (because of the format).

Proposal:

Use hh:mm A to format the date within getParticipantLocalTime:

getParticipantLocalTime() {
        const reportRecipientTimezone = lodashGet(this.props.participant, 'timezone', {});
        return moment().tz(reportRecipientTimezone.selected).format('hh:mm A');
    }

This will make sure that the date is in AM/PM format.

[parasharrajat]

oh nice catch @helaoutar .

[aman-atg]

Hi there, Let me know if we want a solution without changing the formatting ( from 'LT' to 'hh:mm A' ) since we're using LT on all other places

[iwiznia]

@helaoutar I don't think that solution is good. US uses AM/PM by default, but Spain uses 24h system as default. We should not be changing the format to be the same in every country, because that defeats the purpose of localizing the app. The fix should be to not rely on finding am/pm in the timestamp here.

So, yes, @aman-atg, we want a solution that does not change the format.

[parasharrajat]

I added that check as localized time was available with a delay. But I didn't notice the Other languages. We can simply remove this check and Print the whole message when complete time string is available.

[aman-atg]

So, yes, @aman-atg, we want a solution that does not change the format.

Proposal:

Explanation

• So, what I've observed here is that, this isn't even required here and causing the issue
  

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

  Lines 50 to 52 in c009187

  	// Moment.format does not return AM or PM values immediately.
  	// So we have to wait until we are ready before showing the time to the user
  	const isReportRecipientLocalTimeReady = this.state.localTime.toString().match(/(A|P)M/ig);

  • So, first of all it's not required
  • Secondly, AM / PM aren't being displayed immediately because we're setting it after an interval of 1s (you can verify that by changing 1000 to 10 or something lower
    

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

    Line 35 in c009187

    }, 1000));

Solution :

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

Lines 43 to 47 in c009187

	getParticipantLocalTime() {
	const reportRecipientTimezone = lodashGet(this.props.participant, 'timezone', {});
	return moment().tz(reportRecipientTimezone.selected).format('LT');
	}

• Issue while refreshing will be fixed if we add this line of code inside getParticipantLocalTime function

        moment.locale(this.props.preferredLocale);

• Below code should be added before componentWillUnmount , so that we don't render time when it's not formatted according to the language

   shouldComponentUpdate(nextProps, nextState) {
        return nextState.localTime !== this.state.localTime;
    }

testing.mp4

[parasharrajat]

Secondly, AM / PM aren't being displayed immediately because we're setting it after an interval of 1s (you can verify that by changing 1000 to 10 or something lower

This is not true. In the constructor, State is initialized with a value. There must be some other reason. If I am missing something, could you please explain?

const isReportRecipientLocalTimeReady = this.state.localTime.toString().match(/(A|P)M/ig);

This was used as a moment was taking some time to convert the time. This prevents the printing of incomplete messages on the screen.

   shouldComponentUpdate(nextProps, nextState) {
        return nextState.localTime !== this.state.localTime;
    }

seems helpful in reducing rendering.

[aman-atg]

Secondly, AM / PM aren't being displayed immediately because we're setting it after an interval of 1s (you can verify that by changing 1000 to 10 or something lower.

Here, I meant changing the state after the language has been changed

@parasharrajat

• Also, Here comments are suggesting that moment is returning the whole time string except AM / PM (that too with a delay ), which doesn't make any sense.
  

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

  Lines 50 to 52 in c009187

  	// Moment.format does not return AM or PM values immediately.
  	// So we have to wait until we are ready before showing the time to the user
  	const isReportRecipientLocalTimeReady = this.state.localTime.toString().match(/(A|P)M/ig);

[deetergp]

This is definitely something that can be handled externally.

[MelvinBot]

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

Triggered auto assignment to @timszot (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[trjExpensify]

Posted the job to Upwork here: https://www.upwork.com/jobs/~01c1138bff53cf206e

[rdjuric]

I don't think your solution works @aman-atg

This is what we're trying to avoid with the isReportRecipientLocalTimeReady check

web.mov

Proposal

This happens because we're looking for a PM or AM string in the participant time during our isReportRecipientLocalTimeReady check. This check will never pass when we change our language to Spanish, since it has a 24hr time format.

We mistakenly added this check to avoid the flash shown in the video above. The flash happens because we're not localizing the language in our getParticipantLocalTime method, so it first renders without the localization (PM/AM when in English), and then renders with localization (with PM/AM) when we call our getLocalMomentFromTimestamp (used to show when a message was sent), which adds a locale to our moment instance.

App/src/libs/DateUtils.js

Lines 37 to 40 in ad77386

	function getLocalMomentFromTimestamp(locale, timestamp) {
	moment.locale(locale);
	return moment.unix(timestamp).tz(timezone);
	}

To solve this, we can:

1. Remove the isReportRecipientLocalTimeReady conditional
2. Change the locale in the initialization of our moment instance in DateUtils instead of doing it inside getLocalMomentFromTimestamp - the localization is applied globally so shouldn't be initialized inside a function.
3. Put the logic to get the recipient local time in a new function timezoneToLocalTime in our DateUtils. I think it's better to keep our logic/utils to manipulate our dates in the DateUtils, this will help avoid problems like these in the future.

[aman-atg]

@rdjuric
Sorry, I'm not really sure what you're trying to convey here. But here's the video with some testing.
You can share a proper video with steps so that I can try to reproduce it.
There was one issue when refreshing the browser which will be solved by the updated Proposal

testing.mp4

[timszot]

@trjExpensify we can go ahead and hire @aman-atg for this one!

[rdjuric]

No @aman-atg, moment is sync and returns no promises.

I think you've misread my Proposal earlier.. I only explained that the check wasn't required because it's checking for a behaviour which simply doesn't happen.

The behavior happens, that's why @parasharrajat added the check. The AM/PM flashes when you first open or when you refresh the page. Isn't this why you added the moment.locale(this.props.preferredLocale) after I mentioned it?

[aman-atg]

Isn't this why you added the moment.locale(this.props.preferredLocale) after I mentioned it?

• No

The behavior happens, that's why @parasharrajat added the check. The AM/PM flashes when you first open or when you refresh the page.

• Well, you can refer to this comment here to understand what's happening.. I can't explain any further

[rdjuric]

I don't think there's much else I can say, your proposal was wrong and you still don't understand the cause of the issue. I also really dislike that your updated proposal only includes more boilerplate code instead of fixing the root cause.

But this is not my call so that's okay, good luck on the PR 😃

[aman-atg]

And please try the correct solution and show how it will solve the issue.

• Still waiting for your correct solution's demo

you still don't understand the cause of the issue.

• 🤦

[parasharrajat]

moment.locale(locale);

Please be careful with updating the locale at the moment globally. this will cause another issue where we want to run moment() to get the time/date. Please correct me if I am wrong about moment.locale.

[trjExpensify]

Hey guys, catching up here. As a reminder, please keep the conversation constructive. This isn't a community that welcomes petty exchanges and insults. We're here to focus on getting shit done and building something that we all are truly proud of as a collective.

Our open source community is an extension of Expensify and one of our core values is Humility.

It's totally healthy to disagree on a problem or a proposed solution, but we expect members of the community to approach these conversations constructively, with humility and the persistence to find common ground and agreement. "Agreeing to disagree" isn't helpful either, it means you either feel like it's impossible to come to an agreement or you aren't interested in coming to agreement, neither of which is reflective of a humble quality and signals a disregard for the opinions and ideas of others.

So let's work together, and get along. If this behaviour persists from here on, it will result in the immediate termination of your participation in the Expensify Contributor Community as per our Code of Conduct.

Thanks for your cooperation, let's move on. 🚀

[trjExpensify]

PR merged and on staging.

[parasharrajat]

I see a regression that I fixed when I initially added this feature.
on first reload time string switch from It's 15:51 martes for Weird to It's 3:51 PM Tuesday for Weird. instead It's 3:51 PM Tuesday for Weird should be shown from start.

@trjExpensify @isagoico Could you please test this and confirm? Thanks

The steps are as follows:

1. Open any chat where you can see the time.
2. Reload the page and pay attention to the time string. (WEB)

[isagoico]

Oh yeah I see it. Want me open a separate issue for this? Or should this be treated as a deploy blocker.

Note: it's only happening in English. Spanish shows 24 hour format

[parasharrajat]

No. As this is a regression it should be fixed as part of it. just Mark the issue on the linked PR. Thanks. @isagoico

[aman-atg]

@isagoico , this is a regression but it's not caused by this PR. Otherwise it would have been easily caught.
Just like #4352.

( Can't reproduce this regression on the original branch )

[parasharrajat]

this is a regression but it's not caused by this PR. Otherwise, it would have been easily caught.

Ah. It's hard to see at first sight. I know about this as I faced this earlier while creating that time string. I pointed this out #4218 (comment) here

const isReportRecipientLocalTimeReady = this.state.localTime.toString().match(/(A|P)M/ig);

This was used as a moment was taking some time to convert the time. This prevents the printing of incomplete messages on the screen.

So I think it is. But yeah it could be not a regression from this PR.

[aman-atg]

Found the cause of regression here

[MelvinBot]

@timszot, @trjExpensify, @aman-atg it looks like no one is assigned to work on this job.
Please double the price or add a comment stating why the job isn't being doubled.

[timszot]

@vitHoracek I don't think this message is correct. We shouldn't be showing these on issues held for payment, right?

[aman-atg]

@trjExpensify, Any update for this one on Upwork? Thanks.

[trjExpensify]

Yep, sorry for the delay @aman-atg. Paid!