Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[$100] Expensify Card - improvements to the Set a limit screen #50788

Open IuliiaHerets opened 3 days ago

IuliiaHerets commented 3 days ago

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: v9.0.49-0 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

Pre-condition: Enable expensify card and add a VBA

  1. Go to https://staging.new.expensify.com/home
  2. Tap profile icon-- workspaces- workspace
  3. Tap expensify cards
  4. Tap +issue card - select a user -- physical card
  5. Note smart limit checked
  6. Tap next
  7. Enter a decimal number and tap next

Expected Result:

In expensify card, issue a card section if we set limit in decimals, must not show error enter valid amount.

Actual Result:

In expensify card, issue a card section if we set limit in decimals, shows error enter valid amount.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/e3ea6c34-11ee-495f-aa66-b796969fc7e4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021846213635698721920
  • Upwork Job ID: 1846213635698721920
  • Last Price Increase: 2024-10-16
  • Automatic offers:
    • rayane-djouah | Reviewer | 104449939
Issue OwnerCurrent Issue Owner: @rayane-djouah
melvin-bot[bot] commented 3 days ago

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

IuliiaHerets commented 3 days ago

@slafortune FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

twilight2294 commented 3 days ago

Edited by proposal-police: This proposal was edited at 2024-10-15 09:59:48 UTC.

Proposal

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

In expensify card, issue a card section if we set limit in decimals, shows error

What is the root cause of that problem?

We have check to give error on decimals: https://github.com/Expensify/App/blob/b4b59651d38deb3d91b1fd561458fe0d4e23752c/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx#L54

We need to make thus error more specific

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

We should add a new valudation check will will throw an error if !Number.isInteger(Number(values.limit)), the error would be similar to Please enter a whole dollar amount before continuing and the spanish translation too

In en.ts and both es.ts we need to create a new copy as :


    const validate = useCallback(
        (values: FormOnyxValues<typeof ONYXKEYS.FORMS.ISSUE_NEW_EXPENSIFY_CARD_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.ISSUE_NEW_EXPENSIFY_CARD_FORM> => {
            const errors = ValidationUtils.getFieldRequiredErrors(values, [INPUT_IDS.LIMIT]);

            // We only want integers to be sent as the limit
            if (!Number(values.limit)) {
                errors.limit = translate('iou.error.invalidAmount');
            }

            if(!Number.isInteger(Number(values.limit))){
            errors.limit = translate('iou.error.invalidDecimalAmount');
            }
            return errors;
        },
        [translate],
    );

Same should be done to the edit limit screen as well

slafortune commented 3 days ago

Yeah - I don't think that we would/should expect a decimal to be entered. I wonder if simply updating the error message to indicate that they need to enter a whole number would be good.

asking in design - https://expensify.slack.com/archives/C03TME82F/p1729001172189579

slafortune commented 3 days ago

Let's use this to make two updates to th Expensify Card Set Limit Screen

@Expensify/design can you chime in on what would be preferred language?

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 3 days ago

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

shawnborton commented 3 days ago

Let's see if the @Expensify/marketing team has any ideas, but I think I like the first bullet of your suggestions.

NickTooker commented 3 days ago

Please enter a whole dollar amount before continuing.

I think this is fine.

jamesdeanexpensify commented 3 days ago

Double checking - the currency can only be dollars for Expensify Card limits right now, is that correct? Because otherwise "dollar amount" would be incorrect. I just want to make sure, thanks!

slafortune commented 3 days ago

@twilight294 would you like to update your proposal?

rayane-djouah commented 3 days ago
  • Align the $ with the amount box

The alignment of the dollar sign with the amount box is being addressed in https://github.com/Expensify/App/issues/50778

nyomanjyotisa commented 3 days ago

Proposal

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

Expensify Card - improvements to the Set a limit screen

What is the root cause of that problem?

We currently use invalidAmount error message here https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/workspace/expensifyCard/WorkspaceEditCardLimitPage.tsx#L92 https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx#L55

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

Add new translations, need to confirm the right copy

invalidWholeDollarAmount: 'Please enter a whole dollar amount before continuing.',
invalidWholeDollarAmount: 'Por favor, introduce una cantidad en dólares enteros antes de continuar.',

And change the following code to use the new translation https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/workspace/expensifyCard/WorkspaceEditCardLimitPage.tsx#L92 https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx#L55

errors.limit = translate('iou.error.invalidWholeDollarAmount');

For the alignment, is being addressed on another issue

What alternative solutions did you explore? (Optional)

twilight2294 commented 3 days ago

@twilight294 would you like to update your proposal?

Yes i did update my proposal @slafortune @rayane-djouah here

Note: In my previous proposal I already proposed the steps we have to take where to make the changes and both english and spanish copies as well, even mentioned the edit limit page as well

shawnborton commented 3 days ago

Since we're addressing the currency symbol elsewhere and this is just a copy update, we should probably bump this down to something small like $62.50

twilight2294 commented 3 days ago

$125 is fair i guess 😞

shawnborton commented 3 days ago

$125 is too much for a simple copy change.

rayane-djouah commented 3 days ago

The proposal by @twilight294 looks good to me

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 3 days ago

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

rayane-djouah commented 3 days ago

The proposal involves adding a new copy and adjusting the validation logic on both the "Issue New Card Limit" and "Edit Card Limit" screens. Let's see what @thienlnam thinks of a fair payment amount.

thienlnam commented 3 days ago

Proposal looks good, it's a pretty straightforward change. Perhaps we can just meet in between with $100. @slafortune is handling payment for this, so feel free to make a final decision here

melvin-bot[bot] commented 2 days ago

Upwork job price has been updated to $100

twilight2294 commented 2 days ago

@thienlnam can you hire me here :)

melvin-bot[bot] commented 2 days ago

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 days ago

📣 @twilight294 You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

twilight2294 commented 1 day ago

PR ready for review @rayane-djouah