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 2024-11-29] [$375] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount #52029

Closed
2 of 8 tasks
IuliiaHerets opened this issue Nov 5, 2024 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Design External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 5, 2024

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


Version Number: 9.0.57-3
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

  1. Launch app
  2. Login with account with expensify card enabled with bank account added, and few members invited in workspace
  3. Go to workspace settings
  4. Tap expensify card
  5. Tap issue card
  6. Select a user - physical card -monthly
  7. Go to confirmation page
  8. Navigate tapping app's back button
  9. Note cursor shown at beginning at set limit page

Expected Result:

We should always add the cursor to the end of the amount input field

Actual Result:

When going back to the amount field, we put the cursor to the start of the amount which is bad UX

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6655220_1730794710561.Screenrecorder-2024-11-05-13-32-37-9_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021854516214587143362
  • Upwork Job ID: 1854516214587143362
  • Last Price Increase: 2024-11-14
  • Automatic offers:
    • ikevin127 | Contributor | 104952317
Issue OwnerCurrent Issue Owner: @muttmuure
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 5, 2024
Copy link

melvin-bot bot commented Nov 5, 2024

Triggered auto assignment to @muttmuure (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.

@koko57
Copy link
Contributor

koko57 commented Nov 5, 2024

Should the value be cleared when we select another assignee? It was not something that was required in the docs.
I think the form should be cleared only when we exit the flow or we submit the form to issue the card.
And I don't think it's something we would like to change - let's say we went to confirmation step - all the data is ok, but we want to change the assignee only - when we confirm that and we're redirected back to confirmation page the value would be empty. (I wonder if the error would appear, not sure about that).
The only value we want to change in this case is the default card name (for physical card) to be updated to the newly selected user's name i.e. User's Card to NewUser's Card. It has been already implemented.

cc @mountiny

@NJ-2020
Copy link
Contributor

NJ-2020 commented Nov 5, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Card - Android - "Set a limit" amount selected for previous user is displayed

What is the root cause of that problem?

We're not clearing the the limit when selecting assignee

const data: Partial<IssueNewCardData> = {
assigneeEmail: assignee?.login ?? '',
};
if (isEditing && issueNewCard?.data?.cardTitle === Card.getCardDefaultName(PersonalDetailsUtils.getUserNameByEmail(issueNewCard?.data?.assigneeEmail, 'firstName'))) {
// If the card title is the default card title, update it with the new assignee's name
data.cardTitle = Card.getCardDefaultName(PersonalDetailsUtils.getUserNameByEmail(assignee?.login ?? '', 'firstName'));
}
Card.setIssueNewCardStepAndData({
step: isEditing ? CONST.EXPENSIFY_CARD.STEP.CONFIRMATION : CONST.EXPENSIFY_CARD.STEP.CARD_TYPE,
data,
isEditing: false,
});

What changes do you think we should make in order to solve the problem?

We need to reset the limit value by adding limit: 0 to this variable if the previous assignee and the current assignee email login is not same

const data: Partial<IssueNewCardData> = {
assigneeEmail: assignee?.login ?? '',
};

We can't pass limit: undefined because Onyx will not apply the undefined value to the limit property
And for null value the type of limit is number or undefined because it's optional, so when pass null value it will throw an error unless we add the null type to the limit property

const submit = (assignee: ListItem) => {
    const data: Partial<IssueNewCardData> = issueNewCard?.data.assigneeEmail === assignee.login ? {
        assigneeEmail: assignee?.login ?? '',
    } : {
        assigneeEmail: assignee?.login ?? '',
        limit: 0
    };

    if (isEditing && issueNewCard?.data?.cardTitle === Card.getCardDefaultName(PersonalDetailsUtils.getUserNameByEmail(issueNewCard?.data?.assigneeEmail, 'firstName'))) {
        // If the card title is the default card title, update it with the new assignee's name
        data.cardTitle = Card.getCardDefaultName(PersonalDetailsUtils.getUserNameByEmail(assignee?.login ?? '', 'firstName'));
    }

    Card.setIssueNewCardStepAndData({
        step: isEditing ? CONST.EXPENSIFY_CARD.STEP.CONFIRMATION : CONST.EXPENSIFY_CARD.STEP.CARD_TYPE,
        data,
        isEditing: false,
    });
};

And inside the limit input we should fallback the default value to undefined if the limit value is 0 so it start with empty value

defaultValue={CurrencyUtils.convertToFrontendAmountAsString(issueNewCard?.data?.limit, CONST.CURRENCY.USD, false)}

defaultValue={CurrencyUtils.convertToFrontendAmountAsString(issueNewCard?.data?.limit || undefined, CONST.CURRENCY.USD, false)}

What alternative solutions did you explore? (Optional)

@mountiny mountiny self-assigned this Nov 6, 2024
@mountiny mountiny changed the title Card - Android - "Set a limit" amount selected for previous user is displayed [Workspace Feeds] Card - Android - "Set a limit" amount selected for previous user is displayed Nov 6, 2024
@mountiny mountiny added the Design label Nov 6, 2024
Copy link

melvin-bot bot commented Nov 6, 2024

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

@mountiny
Copy link
Contributor

mountiny commented Nov 6, 2024

Good questions, I feel like we do not want to reset it, think that you want to go over the step back and to select different value in the previous step and then you continue to the amount again - I would want to keep the amount I already inserted.

@shawnborton do you agree with that expected results, hence close this issue?

@shawnborton
Copy link
Contributor

Yeah, I agree with your analysis Vit - I don't think we need to reset the limit in this stepped flow personally, I think it makes sense to keep it here. cc @Expensify/design just for visibility but I think they might agree?

@dannymcclain
Copy link
Contributor

I'm fine with not resetting the value, but isn't the bug that the cursor appears at the front of the amount instead of at the end? I guess it's really not that big of deal. Happy to go with whatever you all think is best!

@dubielzyk-expensify
Copy link
Contributor

Agree with not reseting it, but would also love to fix what Danny's mentioning to put the cursor at the end of the number input.

@mountiny mountiny changed the title [Workspace Feeds] Card - Android - "Set a limit" amount selected for previous user is displayed [Workspace Feeds] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount Nov 7, 2024
@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Nov 7, 2024
@melvin-bot melvin-bot bot changed the title [Workspace Feeds] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount [$250] [Workspace Feeds] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount Nov 7, 2024
Copy link

melvin-bot bot commented Nov 7, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021854516214587143362

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 7, 2024
Copy link

melvin-bot bot commented Nov 7, 2024

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

@mountiny
Copy link
Contributor

mountiny commented Nov 7, 2024

Agreed, updated the title and steps and we can await proposals. Thanks everyone for chiming in!

@mananjadhav
Copy link
Collaborator

Still going through the issue and proposal.

@mountiny mountiny changed the title [$250] [Workspace Feeds] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount [$250] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount Nov 12, 2024
@melvin-bot melvin-bot bot added the Overdue label Nov 12, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

@shawnborton, @mananjadhav, @mountiny, @muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

@greg-schroeder
Copy link
Contributor

Looks ready to merge except for potentially a review by @shawnborton

@shawnborton
Copy link
Contributor

All good with me!

@ikevin127
Copy link
Contributor

♻️ Status update: PR was merged and deployed to staging yesterday and is now awaiting deploy to production!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 22, 2024
@melvin-bot melvin-bot bot changed the title [$375] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount [HOLD for payment 2024-11-29] [$375] Card - Android - when user goes back to amount step in the flow the cursor is at the beginning instead of the end of the amount Nov 22, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 22, 2024
Copy link

melvin-bot bot commented Nov 22, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Nov 22, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.65-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-29. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Nov 22, 2024

@mananjadhav / @ikevin127 @muttmuure @mananjadhav / @ikevin127 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 29, 2024
@ikevin127
Copy link
Contributor

cc @muttmuure for visibility

@ikevin127
Copy link
Contributor

ikevin127 commented Nov 29, 2024

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other: react-native external package issue.

Where bug was reported:

  • 2a. Reported on production
  • 2b. Reported on staging (deploy blocker)
  • 2c. Reported on both staging and production
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment: N/A as this was a react-native external package issue.

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion: N/A as this was a react-native external package issue.

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

  1. Launch Android: Native app.
  2. Login with account with expensify card enabled with bank account added, and few members invited in workspace.
  3. Go to workspace settings.
  4. Tap expensify card.
  5. Tap issue card.
  6. Select a user -> physical card -> monthly.
  7. Input an amount in the Set a limit amount page.
  8. Go to confirmation page.
  9. Navigate back to the Set a limit amount page.
  10. Verify that the cursor caret is at the end of amount, not at the begginning right after the currency symbol.

Do we agree 👍 or 👎.

@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2024
Copy link

melvin-bot bot commented Dec 2, 2024

@shawnborton, @mananjadhav, @mountiny, @muttmuure, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@muttmuure
Copy link
Contributor

@ikevin127 - $375 C (Paid)
@mananjadhav - $375 C+

@melvin-bot melvin-bot bot removed the Overdue label Dec 2, 2024
@mananjadhav
Copy link
Collaborator

mananjadhav commented Dec 2, 2024

@muttmuure I am going to hold raising a request on NewDot as all my payouts for November have some issue.

Raised my request on NewDot.

@melvin-bot melvin-bot bot added the Overdue label Dec 4, 2024
@ikevin127
Copy link
Contributor

Not overdue, this can be set to Weekly until the remaining payment is handled.

@muttmuure muttmuure added Weekly KSv2 and removed Daily KSv2 labels Dec 5, 2024
@melvin-bot melvin-bot bot removed the Overdue label Dec 5, 2024
@garrettmknight
Copy link
Contributor

$375 approved for @mananjadhav

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Design External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Development

No branches or pull requests