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

[IuliiaHerets]

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:

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

Screenshots/Videos

Bug6570023_1723474162914.Screen_Recording_2024-08-12_at_7.20.21_AM.mp4

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 Owner

Current Issue Owner: @dukenv0307

[melvin-bot]

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

[melvin-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.

[github-actions]

👋 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]

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

[VictoriaExpensify]

Agree this is an issue and we should fix it.

[melvin-bot]

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

[melvin-bot]

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

[VictoriaExpensify]

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

[Beamanator]

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

[nkdengineer]

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.

App/src/libs/ReportUtils.ts

Line 4285 in 420c2a1

expenseReport.fieldList = policy?.fieldList;

Then because the field has default value is empty, a report violation is added here.

App/src/libs/actions/IOU.ts

Lines 480 to 486 in 420c2a1

	Object.values(iouReport.fieldList ?? {}).forEach((field) => {
	if (excludedFields.includes(field.fieldID) || !!field.value || !!field.defaultValue) {
	return;
	}
	// in case of missing field violation the empty object is indicator.
	missingFields[field.fieldID] = {};
	});

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;

App/src/libs/ReportUtils.ts

Line 4285 in 420c2a1

expenseReport.fieldList = policy?.fieldList;

Then the violation will not appear here since the value already exists

App/src/libs/actions/IOU.ts

Lines 480 to 486 in 420c2a1

	Object.values(iouReport.fieldList ?? {}).forEach((field) => {
	if (excludedFields.includes(field.fieldID) || !!field.value || !!field.defaultValue) {
	return;
	}
	// in case of missing field violation the empty object is indicator.
	missingFields[field.fieldID] = {};
	});

What alternative solutions did you explore? (Optional)

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

App/src/libs/ReportUtils.ts

Line 7471 in 420c2a1

return Localize.translateLocal('reportViolations.fieldRequired', reportField.name);

and should not display a red dot here

App/src/components/ReportActionItem/MoneyReportView.tsx

Line 122 in 420c2a1

brickRoadIndicator={violation ? 'error' : undefined}

[dukenv0307]

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

[puneetlath]

Ahh ok, sorry I didn't correctly understand. Would it be possible to create them optimistically if we have the data, but don't create them optimistically if we don't?

Otherwise, what you suggest makes sense to me.

[VictoriaExpensify]

Not overdue

[nkdengineer]

detect we have the fieldList data we need to potentially show the optimistic violation and show it

@mountiny I think it's hard to detect, for example, we cannot know if the paid expense report is the latest or not or cannot know if we have the paid or not because in OpenApp API, the data may not contain all REPORTPREVIEW actions or expense reports of the policy expense chat

Ahh ok, sorry I didn't correctly understand. Would it be possible to create them optimistically if we have the data, but don't create them optimistically if we don't?

@puneetlath We discussed this above, and then we go with the final solution.

[puneetlath]

Ok got it! Sounds good to me.

[dukenv0307]

To sum it up, we won't create optimistic data for violation when creating the new expense report

We can go with @nkdengineer's proposal

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @dukenv0307 🎉 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]

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[thienlnam]

Catching up in this thread, but it looks like we just decided to remove optimistic report field violations.

When report fields get updated, there should be a onyx update sent out to update the field list. So the field list should be up to date if you're online. So why can't we just not show the violation when the field is auto-filled?

[mountiny]

From what I understood, if you sign out and sign in, we have no way to know if the field should be auto-filled or what it should be auto-filled with. Instead of adding more data (in this case, it might be quite complex) to the OpenApp response, we thought it would be cleaner to just not show this optimistically if we cannot guarantee we know the outcome always and wait for the server response.

Please let us know if there is something we missed, but I think that on fresh sign in, we cannot know what field to autofill or what to autofill it with

[thienlnam]

On a fresh sign in, the policies should be fetched right? And the fieldList is part of the policy onyx key so it should also be loaded unless it gets loaded with like a condensed amount of data

[thienlnam]

But also, that edge case feels like like it is not as problematic as not showing report violations at all optimistically, do you agree with that?

[mountiny]

When we load all the policies, they are formed here and there is no fieldList there, I think we omit the large lists like tags or categories for policies that are not primary.

I am not sure, I think we are comparing a false positive error rarely and then no feedback rarely. That is once we load the policy and report with the reportFields we are good. I prefer the no feedback rather than false positive error

[thienlnam]

In that case, we can just return the fieldList there as well. That field isn't as large as tags or categories which is why we merged it as part of the policy key, and that should resolve this problem - does that sound good?

[mountiny]

We can try it, I did not want to add more load on the OpenApp call, but we can evaluate later

[melvin-bot]

This issue has not been updated in over 15 days. @puneetlath, @mountiny, @VictoriaExpensify, @dukenv0307, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[nkdengineer]

@mountiny Is it ready for payment?

[mountiny]

I would still first try to handle the backend change, I will try to look into it this week

[nkdengineer]

@mountiny friendly bump

[mountiny]

Still on my list