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.

Sign up for GitHub

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

Jump to bottom

Remove timezone restriction to show time in chat #7895

Merged
merged 2 commits into from
Mar 1, 2022
Merged

Remove timezone restriction to show time in chat #7895

merged 2 commits into from
Mar 1, 2022

Conversation

Santhosh-Sellavel
Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel commented Feb 24, 2022 •
edited
Loading

Details

Fixed Issues

$ #7832

Tests

  1. Open Any User Chat with the same timezone as yours. OR (Create two test accounts User A & B with the same timezone settings.
  2. Open User B's chat logged in as User A
  3. Verify the current time of User B's shown
  4. Now change the timezone in User A's settings to a different timezone from User B's
  5. Repeat 2 & 3
  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I made sure to pull main before submitting my PR for review
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I ran the tests & they passed on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
  • I followed the guidelines as stated in the Review Guidelines

PR Reviewer Checklist

  • I verified the Author pulled main before submitting the PR
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • [] I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I verified tests pass on all platforms & I tested again on all platforms
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • [] I verified style guidelines were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines

QA Steps

  1. Open Any User Chat with the same timezone as yours. OR (Create two test accounts User A & B with the same timezone settings.
  2. Open User B's chat logged in as User A
  3. Verify the current time of User B's shown
  4. Now change the timezone in User A's settings to a different timezone from User B's
  5. Repeat 2 & 3
  • Verify that no errors appear in the JS console

Tested On

  • Web

  • Mobile Web

  • iOS

  • Android

  • Desktop - I am facing some errors while running the desktop app in development. So I kindly verify on this alone, I am sure the issue is handled on all platforms.

Screenshots

Web

01_WEB.mov

Mobile Web

Web.mov

iOS

iOS.mov

Android

Android.mov

Desktop

@Santhosh-Sellavel Santhosh-Sellavel marked this pull request as ready for review February 24, 2022 21:46
@Santhosh-Sellavel Santhosh-Sellavel requested a review from a team as a code owner February 24, 2022 21:46
@MelvinBot MelvinBot removed the request for review from a team February 24, 2022 21:46
parasharrajat
parasharrajat suggested changes Feb 24, 2022
View reviewed changes
src/libs/reportUtils.js Outdated
@@ -226,8 +225,7 @@ function canShowReportRecipientLocalTime(personalDetails, myPersonalDetails, rep
&& reportRecipient
&& reportRecipientTimezone
&& currentUserTimezone.selected
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this anymore?

Copy link
Member

Choose a reason for hiding this comment

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

We can also remove this ONYXKEYS.MY_PERSONAL_DETAILS is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need those to ensure reportRecipient && reportRecipientTimezone are available to show the time. The result of this also used to change styles.

Screenshot 2022-02-25 at 12 56 19 PM

currentUserTimezone.selected can be removed.

What do you say?

Copy link
Member

Choose a reason for hiding this comment

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

So you should remove it. Please clean up extra things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to remove currentUserTimezone.selected this alone right? @parasharrajat

Copy link
Member

Choose a reason for hiding this comment

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

You said this can be removed so please remove it. And try to see what can be cleaned up in the code related to this. I think this ONYXKEYS.MY_PERSONAL_DETAILS is also unused now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes will remove the unused code related to those!

parasharrajat
parasharrajat approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM

cc: @chiragsalian

🎀 👀 🎀 C+ reviewed

chiragsalian
chiragsalian approved these changes Mar 1, 2022
View reviewed changes
Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

Code, LGTM.

@chiragsalian chiragsalian merged commit 4f4b158 into Expensify:main Mar 1, 2022
@chiragsalian
Copy link
Contributor

Saw this section right after - PR Reviewer Checklist
@parasharrajat, do you know if i should be filling that or you?

@parasharrajat
Copy link
Member

Oops missed it. I will update my review comment to include this.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 2022

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Mar 10, 2022
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @chiragsalian in version: 1.1.42-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parasharrajat parasharrajat parasharrajat approved these changes

@chiragsalian chiragsalian chiragsalian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Santhosh-Sellavel @chiragsalian @parasharrajat @OSBotify