App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Expense report -"Required" Error Not Triggered for Auto-Filled Report Field on Second Expense Submission

Open IuliiaHerets opened this issue 1 year ago • 56 comments

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: v9.0.19-0 Reproducible in staging?: Y Reproducible in production?: N Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Go to FAB > Workspace.
  2. Navigate to Workspace Settings > More Features > Enable Report Field.
  3. In Report Field, add a field, name it, choose a type, and save.
  4. Go to Workspace Chat > Click the plus sign > Submit Expense > Add amount, merchant, and submit.
  5. In Expense report, fill in the Report Field > Notice that the error triggers > Go back to the workspace chat, click Submit, and select Pay Elsewhere.
  6. Submit a second expense by repeating Step 4 > Go to Expense report > Notice that the Report Field is auto-filled, but the error does trigger.

Expected Result:

The "Required" error message should not appear if the Report Field in the second expense is auto-filled with information from the first expense.

Actual Result:

The "Required" error message does not trigger for the Report Field on the second submitted expense when auto-filled.

Workaround:

Unknown

Platforms:

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [ ] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/69cc30dc-7e05-47f5-a160-6bc73c415c73

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0185bccf1e3226865f
  • Upwork Job ID: 1823114245300473767
  • Last Price Increase: 2024-09-02
  • Automatic offers:
    • dukenv0307 | Reviewer | 103834642
    • nkdengineer | Contributor | 103834644
Issue OwnerCurrent Issue Owner: @dukenv0307

IuliiaHerets avatar Aug 12 '24 17:08 IuliiaHerets

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

melvin-bot[bot] avatar Aug 12 '24 17:08 melvin-bot[bot]

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

melvin-bot[bot] avatar Aug 12 '24 17:08 melvin-bot[bot]

:wave: 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.

github-actions[bot] avatar Aug 12 '24 17:08 github-actions[bot]

We think that this bug might be related to #vip-vsb

IuliiaHerets avatar Aug 12 '24 17:08 IuliiaHerets

Agree this is an issue and we should fix it.

VictoriaExpensify avatar Aug 12 '24 21:08 VictoriaExpensify

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

melvin-bot[bot] avatar Aug 12 '24 21:08 melvin-bot[bot]

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

melvin-bot[bot] avatar Aug 12 '24 21:08 melvin-bot[bot]

Report Fields are part of Wave_Control, so I'm adding this issue to that project

VictoriaExpensify avatar Aug 12 '24 22:08 VictoriaExpensify

Report Fields are behind a beta, so i'm demoting this one

Beamanator avatar Aug 13 '24 03:08 Beamanator

Edited by proposal-police: This proposal was edited at 2024-08-13 08:42:12 UTC.

Proposal

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

The "Required" error message does not trigger for the Report Field on the second submitted expense when auto-filled.

What is the root cause of that problem?

When we create an expense report, the fieldList is assigned as the fieldList of the policy.

https://github.com/Expensify/App/blob/420c2a13cc176dca5adf8dcc8132ef5f8ff678f7/src/libs/ReportUtils.ts#L4285

Then because the field has default value is empty, a report violation is added here. https://github.com/Expensify/App/blob/420c2a13cc176dca5adf8dcc8132ef5f8ff678f7/src/libs/actions/IOU.ts#L480-L486

After BE returns the data, this field is auto-filled but the report violation isn't cleared then the error still appears

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

When we create an expense report here, we should auto-fill the report field from the last paid expense report of this policy expense chat (we can confirm the logic from BE).

const reportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReportID}`] ?? {};
const lastSettledReportPreviewAction = Object.values(reportActions)
    .filter((action) => ReportActionsUtils.isReportPreviewAction(action) && isSettled(action.childReportID))
    .sort((a, b) => {
        if (a.created < b.created) {
            return -1;
        }
        return 1;
    })
    ?.at(-1);

const lastSettedExpenseReport = getReportOrDraftReport(lastSettledReportPreviewAction?.childReportID);

expenseReport.fieldList = lastSettedExpenseReport?.fieldList ?? policy?.fieldList;

return expenseReport;

https://github.com/Expensify/App/blob/420c2a13cc176dca5adf8dcc8132ef5f8ff678f7/src/libs/ReportUtils.ts#L4285

Then the violation will not appear here since the value already exists https://github.com/Expensify/App/blob/420c2a13cc176dca5adf8dcc8132ef5f8ff678f7/src/libs/actions/IOU.ts#L480-L486

What alternative solutions did you explore? (Optional)

If the violation is fieldRequired and the value exists we should return an empty string here

https://github.com/Expensify/App/blob/420c2a13cc176dca5adf8dcc8132ef5f8ff678f7/src/libs/ReportUtils.ts#L7471

and should not display a red dot here https://github.com/Expensify/App/blob/420c2a13cc176dca5adf8dcc8132ef5f8ff678f7/src/components/ReportActionItem/MoneyReportView.tsx#L122

nkdengineer avatar Aug 13 '24 04:08 nkdengineer

@nkdengineer Your solution makes sense to me. Can you pls share the detail implementation? Thanks

dukenv0307 avatar Aug 13 '24 08:08 dukenv0307

@dukenv0307 Update my proposal with implementation detail. We can confirm with the internal team about this logic and the BE logic.

nkdengineer avatar Aug 13 '24 08:08 nkdengineer

We can confirm with the internal team about this logic and the BE logic.

@puneetlath can you help check the concern above? Thanks

dukenv0307 avatar Aug 13 '24 09:08 dukenv0307

It feels to me like this is working as expected. But confirming here: https://expensify.slack.com/archives/C06ML6X0W9L/p1723746405468429

puneetlath avatar Aug 15 '24 18:08 puneetlath

@puneetlath The problem here is I want to know how BE auto-fill the report field for an expense report when we create an expense. So I can do this in optimistic data to prevent the incorrect report violation in optimistic data.

nkdengineer avatar Aug 16 '24 07:08 nkdengineer

@puneetlath, @VictoriaExpensify, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Aug 16 '24 18:08 melvin-bot[bot]

@nkdengineer we are still discussing this one. It seems like the report fields should be sticky between the reports but when I tried on OldDot, it was not sticky. Can you try as well what is the different behaviour between NewDot and OldDot in this flow? Thanks!

mountiny avatar Aug 19 '24 06:08 mountiny

@mountiny I tested in OldDot, it was sticky

https://github.com/user-attachments/assets/e2d9e0a2-6415-476f-85b2-cfba197181f2

nkdengineer avatar Aug 19 '24 07:08 nkdengineer

@nkdengineer what if you create a new workspace and report and set up the report fields in oldDot solely (sorry mobile today so cant test myself) - i think that was the flow where i got no stickiness

mountiny avatar Aug 19 '24 13:08 mountiny

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Aug 19 '24 16:08 melvin-bot[bot]

@puneetlath, @VictoriaExpensify, @dukenv0307 Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] avatar Aug 20 '24 18:08 melvin-bot[bot]

what if you create a new workspace and report and set up the report fields in oldDot solely (sorry mobile today so cant test myself) - i think that was the flow where i got no stickiness

@mountiny I tested this case and the new report is also auto-filled by previous paid expense report after we paid an expense report.

nkdengineer avatar Aug 21 '24 04:08 nkdengineer

It sounds like everything is working correctly then, is that right?

puneetlath avatar Aug 21 '24 19:08 puneetlath

@puneetlath Yes BE is working correctly but we need to update the optimistic data in front-end to prevent incorrect violation is displayed.

The problem here is I want to know how BE auto-fill the report field for an expense report when we create an expense. So I can do this in optimistic data to prevent the incorrect report violation in optimistic data.

Can you check my concern above?

cc @mountiny

nkdengineer avatar Aug 22 '24 03:08 nkdengineer

@puneetlath, @VictoriaExpensify, @dukenv0307 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] avatar Aug 22 '24 18:08 melvin-bot[bot]

@nkdengineer Yes you are right, we need to update the optimistic case, althouhg I wonder if we have all the data for this available when user signs out and signs back in, can you check that and make a proposal for that case?

mountiny avatar Aug 23 '24 19:08 mountiny

Not overdue

mountiny avatar Aug 23 '24 19:08 mountiny

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Aug 26 '24 16:08 melvin-bot[bot]

@nkdengineer are you able to look into this one further after my latest comment?

mountiny avatar Aug 26 '24 16:08 mountiny

I'm checking this by testing some cases with many report previews. Will give an update tomorrow.

nkdengineer avatar Aug 26 '24 16:08 nkdengineer