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

Expense - Payment option is not selected from the dropdown unless it is reselected #49331

Open
6 tasks done
IuliiaHerets opened this issue Sep 17, 2024 · 21 comments
Open
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

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


Version Number: 9.0.36-0
Reproducible in staging?: Y
Reproducible in production?: N/A - new feature, doesn't exist in prod
Email or phone of affected tester (no customers): applausetester+watashiwastar2@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  • Workspace has set up bank account.
  1. Go to staging.new.expensify.com
  2. Go to workspace chat.
  3. Submit an expense.
  4. On the main chat, click on the payment dropdown button.
  5. Select the other payment option.
  6. Click on the payment dropdown button.
  7. Select the other payment option again.

Expected Result:

The payment option will be selected without issue.

Actual Result:

The payment option is not selected until it is selected for the second time.
On Android and iOS app, the payment option is selected without issue during the first attempt, but it reverts to the previous option when opening dropdown and dismissing the modal.

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6606369_1726563197027.20240917_164453.mp4

View all open jobs on GitHub

@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. labels Sep 17, 2024
Copy link

melvin-bot bot commented Sep 17, 2024

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Sep 17, 2024

Triggered auto assignment to @mountiny (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Sep 17, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Sep 17, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-collect - Release 2

@grgia
Copy link
Contributor

grgia commented Sep 17, 2024

potentially related to #48863 @shubham1206agra

@mountiny
Copy link
Contributor

Reproducible in production?: N/A - new feature, doesn't exist in prod

Interesting, this does not feel like a new feature

@shubham1206agra
Copy link
Contributor

Culprit PR is #48615. @bernhardoj

@trjExpensify
Copy link
Contributor

Thread here, I think Bernhard is out of action, so we need someone to jump on this. @ZhenjaHorbach perhaps as you have the context from the review?

@ZhenjaHorbach
Copy link
Contributor

Thread here, I think Bernhard is out of action, so we need someone to jump on this. @ZhenjaHorbach perhaps as you have the context from the review?

Sure !
I can check in an hour !

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Sep 17, 2024

Hmmmm
Actually this issue is related with these changes

#48615 (review)

Since we updated dependencies we update list sorting it according to this condition every time

// Put the preferred payment method to the front of the array, so it's shown as default. We assume their last payment method is their preferred.
if (lastPaymentMethod) {
return buttonOptions.sort((method) => (method.value === lastPaymentMethod ? -1 : 0));
}

But since we select active option by index

const selectedItem = options[selectedItemIndex] || options[0];

It turns out that after sorting, the order of elements changes, but the index remains

And I'm a little confused 😅
I agree with @roryabraham that we needed to update dependencies

But it looks like this condition is redundant

// Put the preferred payment method to the front of the array, so it's shown as default. We assume their last payment method is their preferred.
if (lastPaymentMethod) {
return buttonOptions.sort((method) => (method.value === lastPaymentMethod ? -1 : 0));
}

@grgia
Copy link
Contributor

grgia commented Sep 17, 2024

@ZhenjaHorbach do you think we'll be able to get a fix up today or consider reverting this PR for the deploy?

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Sep 17, 2024

@ZhenjaHorbach do you think we'll be able to get a fix up today or consider reverting this PR for the deploy?

Actually we have two fixes

  1. We can revert these changes (Animate settlement button when pay and trigger a haptic feedback #48615 (review))
    And since previous code has been around for a long time, I'm not sure that this will cause any regression

  2. Or we can remove this condition (

    // Put the preferred payment method to the front of the array, so it's shown as default. We assume their last payment method is their preferred.
    if (lastPaymentMethod) {
    return buttonOptions.sort((method) => (method.value === lastPaymentMethod ? -1 : 0));
    }
    )
    And I can't come up of any regression that could cause these changes

@ZhenjaHorbach
Copy link
Contributor

@ZhenjaHorbach do you think we'll be able to get a fix up today or consider reverting this PR for the deploy?

So I think I can create a PR today with the second proposal
Since this proposal looks right to me

@grgia
Copy link
Contributor

grgia commented Sep 17, 2024

I think that's fine @ZhenjaHorbach. Do you happen to know why we added the policyId selector here?

const [lastPaymentMethod = '-1'] = useOnyx(ONYXKEYS.NVP_LAST_PAYMENT_METHOD, {selector: (paymentMethod) => paymentMethod?.[policyID]});

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Sep 17, 2024

I think it would just simplify the code

Снимок экрана 2024-09-17 в 17 26 04

@grgia
Copy link
Contributor

grgia commented Sep 17, 2024

@ZhenjaHorbach sounds good to me

@mountiny
Copy link
Contributor

Thanks for looking into this, just for my understanding that PR is already in production right? So that would mean it cannot be the root cause of the deploy blocker, right?

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Sep 17, 2024

Created PR for this issue
#49345

@ZhenjaHorbach
Copy link
Contributor

Thanks for looking into this, just for my understanding that PR is already in production right? So that would mean it cannot be the root cause of the deploy blocker, right?

Maybe we just didn't notice the issue when it was in staging 😅

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Sep 17, 2024
@mountiny mountiny removed the DeployBlockerCash This issue or pull request should block deployment label Sep 17, 2024
@mountiny
Copy link
Contributor

Demoting as this is already in production

@mountiny
Copy link
Contributor

PR is up, @dukenv0307 is assigned for a review, as its not a blocker we do not have to CP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants