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

When bank account is pending a "Test transactions" heading displayed incorrectly #6900

Closed
mvtglobally opened this issue Dec 24, 2021 · 39 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Dec 24, 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. Open app
  2. Go through the VBA flow to trigger pending account

Expected Result:

"Test transactions" should display only for the page with the transactions CTA

Actual Result:

"Test transactions" displayed when user still has steps to complete verification
Potentially related to #6477

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.1.23-0
Reproducible in staging?: Y
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

2021-12-23_10-30-38

Expensify/Expensify Issue URL:
Issue reported by: @marcaaron
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1640291627075700

View all open jobs on GitHub

@MelvinBot
Copy link

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

@PrashantMangukiya

This comment has been minimized.

@marcaaron
Copy link
Contributor

@PrashantMangukiya Please wait for the Help Wanted label to be applied before proposing a solution. Thanks!

@alex-mechler
Copy link
Contributor

This can be dne by a contributor! Adding the label

@alex-mechler alex-mechler added the External Added to denote the issue can be worked on by a contributor label Dec 24, 2021
@MelvinBot
Copy link

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

@PrashantMangukiya

This comment has been minimized.

@MelvinBot
Copy link

@laurenreidexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@laurenreidexpensify
Copy link
Contributor

Adding this to upwork now

@MelvinBot MelvinBot removed the Overdue label Dec 28, 2021
@laurenreidexpensify
Copy link
Contributor

@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

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

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

@PrashantMangukiya
Copy link
Contributor

Proposed Solution:

At present title is static so it shows same title all time. So we have to make title dynamic. We can set title (line 166) based on the isVerifying (line 161) variable, because isVerifying is controlling ui content that we concern. i.e. when isVerifying true it shows Let's Finish in Chat section, otherwise showing test transaction input form.

const maxAttemptsReached = lodashGet(this.props, 'reimbursementAccount.maxAttemptsReached');
const isVerifying = !maxAttemptsReached && state === BankAccount.STATE.VERIFYING;
return (
<View style={[styles.flex1, styles.justifyContentBetween]}>
<HeaderWithCloseButton
title={this.props.translate('workspace.common.testTransactions')}
stepCounter={{step: 5, total: 5}}
onCloseButtonPress={Navigation.dismissModal}
onBackButtonPress={() => Navigation.goBack()}
shouldShowBackButton
shouldShowStepCounter={!isVerifying}
/>

We have add/update code as shown below, It will make title dynamic as per need.

  // **** Add below code ***
  let titleText = this.props.translate('workspace.common.testTransactions');
  if (isVerifying) {
      titleText = this.props.translate('workspace.common.bankAccount');
  }

 <HeaderWithCloseButton
      title={titleText}      // *** Update this line 
      stepCounter={{step: 5, total: 5}}
      onCloseButtonPress={Navigation.dismissModal}
      onBackButtonPress={() => Navigation.goBack()}
      shouldShowBackButton
      shouldShowStepCounter={!isVerifying}
  />

Below is the screenshot once code updated:

ConnectBankAccount

TestTransactions

@parasharrajat
Copy link
Member

@PrashantMangukiya proposal looks good but I am not sure what should be the expected header or it is supposed to be hidden? cc: @marcaaron

@marcaaron
Copy link
Contributor

I think definitely not hidden. @Expensify/marketing maybe can help.

@laurenreidexpensify laurenreidexpensify removed the External Added to denote the issue can be worked on by a contributor label Dec 31, 2021
@laurenreidexpensify laurenreidexpensify removed their assignment Dec 31, 2021
@laurenreidexpensify laurenreidexpensify added the External Added to denote the issue can be worked on by a contributor label Dec 31, 2021
@MelvinBot
Copy link

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

@laurenreidexpensify
Copy link
Contributor

@jliexpensify am OOO next week, back 11 Jan so reassigning

@jliexpensify
Copy link
Contributor

Hello, taking over from Lauren here! Just wondering if you wanted me to hire anyone? Cheers! cc @marcaaron @parasharrajat

@parasharrajat
Copy link
Member

I am still not sure what is decided for the header text. it seems we are still discussing but as we only need to change the text for it.

I think @PrashantMangukiya 's proposal is good #6900 (comment).

cc: @marcaaron
🎀 👀 🎀 C+ reviewed

@marcaaron
Copy link
Contributor

Thanks, we're still waiting for copy so there's nothing to do for now.

@deetergp
Copy link
Contributor

Are we still waiting on copy?

@MelvinBot MelvinBot removed the Overdue label Jan 15, 2022
@parasharrajat
Copy link
Member

Yeah.

@parasharrajat
Copy link
Member

cc: @Expensify/marketing Could you please confirm the Header Text to be used for the screen?

@jamesdeanexpensify
Copy link
Contributor

Do you just need review and approval of the two headers shown in this comment?

@parasharrajat
Copy link
Member

No, we are looking to get the correct header name for the screen attached on the issue description.

@jamesdeanexpensify
Copy link
Contributor

Maybe something like "Almost there..." ?

@parasharrajat
Copy link
Member

cc: @marcaaron , He is involved in VBA.

@deetergp
Copy link
Contributor

A penny for your thoughts, @marcaaron?

@MelvinBot MelvinBot removed the Overdue label Jan 28, 2022
@marcaaron
Copy link
Contributor

@jamesdeanexpensify suggestion to use "Almost there..." makes sense to me.

@parasharrajat
Copy link
Member

Sounds good. I liked @PrashantMangukiya' proposal #6900 (comment).

cc: @deetergp

🎀 👀 🎀 C+ reviewed

@PrashantMangukiya
Copy link
Contributor

There is another issue related to this and they implemented similar solution using isVerifying etc. (while we waiting for the grammar here). Here is the issue #7322 Now I am wandering what we have do in this issue. Someone from C+ or Expensify team please refer it and decide strategy what to do in this issue.

@parasharrajat
Copy link
Member

Thanks for linking that @PrashantMangukiya .

Ok, This issue is fixed now via #7322. Let's just close it @jliexpensify

@jliexpensify
Copy link
Contributor

Job closed, cheers Rajat!

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 Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests