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.34k stars 2.77k forks source link

Report fields - Same list values can be saved without error #49524

Open IuliiaHerets opened 2 hours ago

IuliiaHerets commented 2 hours 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: 9.0.38-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+sdsddh2@applause.expensifail.com 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:

Screenshots/Videos

https://github.com/user-attachments/assets/08ffa841-0178-40eb-9aa7-e44f9195298d

View all open jobs on GitHub

melvin-bot[bot] commented 2 hours ago

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 commented 2 hours ago

@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 commented 2 hours ago

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 https://github.com/Expensify/App/blob/4c9a3fbac779689ec6abade55df8234b1b859e51/src/pages/workspace/reportFields/ReportFieldsAddListValuePage.tsx#L42

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

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 commented 2 hours ago

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. imagehttps://github.com/Expensify/App/blob/4c9a3fbac779689ec6abade55df8234b1b859e51/src/pages/workspace/reportFields/ReportFieldsAddListValuePage.tsx#L42

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)