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

[Hold for Payment - Sept 6th] Receipts@ shows localtime #4664

Closed
mananjadhav opened this issue Aug 14, 2021 · 17 comments
Closed

[Hold for Payment - Sept 6th] Receipts@ shows localtime #4664

mananjadhav opened this issue Aug 14, 2021 · 17 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mananjadhav
Copy link
Collaborator

mananjadhav commented Aug 14, 2021

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


Action Performed:

  1. Go to Chat Screen
  2. Start a new Chat with receipts@expensify.com or open an existing chat with receipts@expensify.com
  3. You'll see local time for receipts@expensify.com

Expected Result:

It should hide the receipts@ local time similar to the concierge.

Actual Result:

Shows local time for receipts@ in the chat window.

image

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

Job URL: https://www.upwork.com/jobs/~0166e67922e53032e0

@mananjadhav mananjadhav added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 14, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 14, 2021
@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 14, 2021

Proposal

In ReportActionCompose, there's a flag shouldShowReportRecipientLocalTime. We need to update the checks for the same.

import lodashIntersection from 'lodash/intersection';

const hasAutomatedAccounts = lodashIntersection(
							 reportParticipants, [
							  CONST.EMAIL.CHRONOS,
							  CONST.EMAIL.CONCIERGE,
							  CONST.EMAIL.RECEIPTS
						    ]).length > 0;

// Following codes to be removed
// REMOVE CODE: const hasChronosParticipant = _.contains(reportParticipants, CONST.EMAIL.CHRONOS);
// REMOVE CODE: const hasConciergeParticipant = _.contains(reportParticipants, CONST.EMAIL.CONCIERGE);

// Flag check to be updated as below:
const shouldShowReportRecipientLocalTime = !hasAutomatedAccounts
            && !hasMultipleParticipants
            && reportRecipient
            && reportRecipientTimezone
            && currentUserTimezone.selected !== reportRecipientTimezone.selected;

@MelvinBot
Copy link

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

@Dal-Papa Dal-Papa added the External Added to denote the issue can be worked on by a contributor label Aug 16, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 17, 2021
@MelvinBot
Copy link

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

@NicMendonca
Copy link
Contributor

Posted to Upwork: https://www.upwork.com/jobs/~0166e67922e53032e0

@thienlnam
Copy link
Contributor

If possible, can we use https://github.com/Expensify/expensify-common/blob/master/lib/CONST.jsx#L469 this list of expensify emails here and prevent showing local time for them?

@mananjadhav
Copy link
Collaborator Author

I just checked the list sometime back. Yeah, we should update this list in the App too, and disable the localtime for these accounts.

@pecanoro
Copy link
Contributor

Could you update the proposal with @thienlnam suggestion? Besides that, it sounds good to me.

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 18, 2021

@pecanoro

Updated Proposal from #4664 (comment)

In CONST.js, update all the following email addresses:

EXPENSIFY_EMAILS: ['concierge@expensify.com', 'help@expensify.com', 'receipts@expensify.com', 'chronos@expensify.com', 'qa@expensify.com', 'contributors@expensify.com', 'firstresponders@expensify.com', 'qa+travisreceipts@expensify.com'],

EMAIL: {
   HELP: 'help@expensify.com',
   QA: 'qa@expensify.com'
  ....
},

In ReportActionCompose, there's a flag shouldShowReportRecipientLocalTime. We need to update the checks for the same.

import lodashIntersection from 'lodash/intersection';

const hasAutomatedAccounts = lodashIntersection(
							 reportParticipants, [
							  CONST.EMAIL.CHRONOS,
							  CONST.EMAIL.CONCIERGE,
							  CONST.EMAIL.RECEIPTS,
                                                             CONST.EMAIL.HELP,
                                                             CONST.EMAIL.QA,
                                                             ...
						    ]).length > 0;

// Following codes to be removed
// REMOVE CODE: const hasChronosParticipant = _.contains(reportParticipants, CONST.EMAIL.CHRONOS);
// REMOVE CODE: const hasConciergeParticipant = _.contains(reportParticipants, CONST.EMAIL.CONCIERGE);

// Flag check to be updated as below:
const shouldShowReportRecipientLocalTime = !hasAutomatedAccounts
            && !hasMultipleParticipants
            && reportRecipient
            && reportRecipientTimezone
            && currentUserTimezone.selected !== reportRecipientTimezone.selected;

@pecanoro
Copy link
Contributor

Just wondering, why do we need to update CONST and not use the variable EXPENSIFY_EMAILS directly for hasAutomatedAccounts?

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 19, 2021

We can do that. I was trying to maintain the existing structure in App where we store each of the emails as constants and then refer.

image

We would need these constants as we do have inclusion/exclusion based on conditions in different modules, For instance, we allow chats with Concierge and Chronos but not with Receipts. We don't allow split money etc operations with any automated accounts.

@pecanoro
Copy link
Contributor

Got it!! Then all good!

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 19, 2021
@pecanoro pecanoro added Weekly KSv2 and removed Daily KSv2 labels Aug 19, 2021
@mananjadhav
Copy link
Collaborator Author

Thanks @pecanoro I'll start on the PR. Meanwhile I've submitted the updated proposal on Upwork too.

@NicMendonca
Copy link
Contributor

@mananjadhav hired in Upwork!

@NicMendonca NicMendonca changed the title Receipts@ shows localtime [Hold for Payment - Sept 6th] Receipts@ shows localtime Aug 31, 2021
@NicMendonca
Copy link
Contributor

Looks like this was deployed yesterday. Holding for payment until Sept 6th.

@MelvinBot MelvinBot removed the Overdue label Aug 31, 2021
@NicMendonca
Copy link
Contributor

paid & ended contract (sorry for the 1 day delay!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants