[HOLD for payment 2024-10-17] [$250] Report fields - Same list values can be saved without error

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

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace settings > Report fields.
3. Click Add field.
4. Add a list type report field and save it.
5. Click on the added list type report field.
6. Click List values.
7. Click Add value.
8. Add a list value and save it.
9. Add another same list value as Step 9 and save it.

Expected Result:

App will show error when adding same list value in Step 9.

Actual Result:

App does not show error when adding same list value in Step 9.
User can add the same list values many times without issue.
When selecting one of the same list values via checklist, all the same values are selected and the dropdown button shows "1 selected".

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6609078_1726764527892.bandicam_2024-09-20_00-44-20-179.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021838693059270278548
• Upwork Job ID: 1838693059270278548
• Last Price Increase: 2024-09-24
• Automatic offers:
• hoangzinh | Reviewer | 104132595
• etCoderDysto | Contributor | 104132596

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot]

Triggered auto assignment to @johncschuster (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]

@johncschuster 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

[etCoderDysto]

Proposal

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

Report fields - Same list values can be saved without error

What is the root cause of that problem?

We are passing list values from draft (formDraft?.[INPUT_IDS.LIST_VALUES]) to validateReportFieldListValueName. And formDraft?.[INPUT_IDS.LIST_VALUES] is undefined when editing saved report list and empty array will be passed to validateReportFieldListValueName as list of values. And no error will be thrown since the newly added value is not in the empty list

App/src/pages/workspace/reportFields/ReportFieldsAddListValuePage.tsx

Line 42 in 4c9a3fb

WorkspaceReportFieldUtils.validateReportFieldListValueName(values[INPUT_IDS.VALUE_NAME].trim(), '', formDraft?.[INPUT_IDS.LIST_VALUES] ?? [], INPUT_IDS.VALUE_NAME),

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

We should pass existing field values when there is no draft lists (formDraft?.[INPUT_IDS.LIST_VALUES])

• get saved field values

const reportFieldKey = ReportUtils.getReportFieldKey(reportFieldID);
const fieldValues = policy?.fieldList?.reportFieldKey?.values;

• Pass it to validateReportFieldListValueName when there is no draft field values

WorkspaceReportFieldUtils.validateReportFieldListValueName(
                values[INPUT_IDS.VALUE_NAME].trim(),
                '',
                formDraft?.[INPUT_IDS.LIST_VALUES] ?? fieldValues ?? [],

Note: Optionally if reportFieldID is undefined we can pass formDraft?.[INPUT_IDS.LIST_VALUES] to validateReportFieldListValueName and pass existing report fields (fieldValues) when it not undefined.

What alternative solutions did you explore? (Optional)

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-09-20 11:57:13 UTC.

Proposal

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

Report fields - Same list values can be saved without error

What is the root cause of that problem?

When editing a report field, we are passing the form draft data as the value list. This is incorrect. The form draft data should only be used when creating a new field, not when editing, as the draft data is empty during field editing.

App/src/pages/workspace/reportFields/ReportFieldsAddListValuePage.tsx

Line 42 in 4c9a3fb

WorkspaceReportFieldUtils.validateReportFieldListValueName(values[INPUT_IDS.VALUE_NAME].trim(), '', formDraft?.[INPUT_IDS.LIST_VALUES] ?? [], INPUT_IDS.VALUE_NAME),

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

If the report field ID exists, it means the field is not a draft, so we can use the policy field. Otherwise, we should use the draft value:

const reportFieldKey = ReportUtils.getReportFieldKey(reportFieldID);
const reportFieldValues = reportFieldID 
    ? Object.values(policy?.fieldList?.[reportFieldKey]?.values ?? {}) 
    : formDraft?.[INPUT_IDS.LIST_VALUES] ?? [];

Then update the following line:

WorkspaceReportFieldUtils.validateReportFieldListValueName(
    values[INPUT_IDS.VALUE_NAME].trim(), '', reportFieldValues, INPUT_IDS.VALUE_NAME
);

Note that we have to wrap the values returned from the field with Object.values() to get the values array and use it in the validateReportFieldListValueName, otherwise it will not work.

What alternative solutions did you explore? (Optional)

Optionally: i can see that we use the following logic in multiple locations here , here , and here

Object.values(policy?.fieldList?.[reportFieldKey]?.values ?? {}) 

so we can move it to a util function instead of duplicating the code everywhere

[melvin-bot]

@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[melvin-bot]

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

[etCoderDysto]

Note that we have to wrap the values returned from the field with Object.values() to get the values array and use it in the validateReportFieldListValueName, otherwise it will not work.

We don't need Object.values(). values is already an array of existing values ;⁠-⁠).

[abzokhattab]

1. The use of Object.values() is important to ensure a fallback to an empty array when any of the following is undefined: policy?.fieldList?.[reportFieldKey]?.values. This notation is consistently used in multiple locations, including here , here , and here. To maintain consistency, I have retained this approach. However, we could consider creating a utility function to streamline and clean up these locations, as the logic is the same across them.
2. A small note: the following snippet is not accurate: const fieldValues = policy?.fieldList?.reportFieldKey?.values. It should instead be written as policy?.fieldList?.[reportFieldKey]?.values

[abzokhattab]

I am just raising the points since the two proposals were around the same time and I took the time to validate my solution with POC and a complete working solution.

lets see what others think

Thanks!

[etCoderDysto]

@abzokhattab thanks for pointing out those issues. IMO, those improvements seems minor and should be handled during PR phase as they don't have a substantial deviation from my RCA and suggested solution. I welcome any decision by the C+.

And I appreciate that you always raise your concern in a very polite way.

[hoangzinh]

Thanks for proposals, everyone. I've appreciated it.

Hmm, it's hard for me to make a decision in such cases. Agreed with @abzokhattab raised here. But overall, @etCoderDysto's approach is correct (and the same as @abzokhattab's approach), and those mistakes can be addressed in PR. Because @etCoderDysto is first (earlier than 6 mins compared with @abzokhattab), therefore I think we can go with @etCoderDysto's proposal

Link to proposal #49524 (comment)

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

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

📣 @etCoderDysto 🎉 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 📖

[melvin-bot]

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

[melvin-bot]

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

• fix: Display error message when saving the same report list value #49791

If no regressions arise, payment will be issued on 2024-10-17. 🎊

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

• @hoangzinh requires payment automatic offer (Reviewer)
• @etCoderDysto requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
• [@hoangzinh] 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:
• [@hoangzinh] A discussion in #expensify-bugs 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:
• [@hoangzinh] Determine if we should create a regression test for this bug.
• [@hoangzinh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Julesssss]

I don't think we need a regression test.

[johncschuster]

Payment Summary:

Contributor: @etCoderDysto paid $250 via Upwork
Contributor+: @hoangzinh paid $250 via Upwork