[$250] Report fields -System message shows undefined when list value is changed back to initial value

[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.18-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 chat.
3. Submit two expenses.
4. Go to workspace settings > Report fields (enable in More features).
5. Add a list type report field, with some list values and an initial value.
6. Return to the workspace chat.
7. Go to expense report.
8. Click on the list report field.
9. Select a different list value.
10. Revisit the report.
11. Click on the list report field.
12. Click on the already selected value (so that it will change to the initial value).
13. Revisit the report.

Expected Result:

The system message will not contain "undefined".

Actual Result:

The system message contains "undefined" when the list value is changed back to the initial value.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/1b16a1c3-01a3-4094-95e5-921f7ee73fe6

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014a67065c97e4bdba
• Upwork Job ID: 1822942704704377446
• Last Price Increase: 2024-08-19
• Automatic offers:
• dukenv0307 | Reviewer | 103641179
• nkdengineer | Contributor | 103641182

Issue Owner

Current Issue Owner: @grgia

[melvin-bot[bot]]

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

We think that this bug might be related to #wave-control

[IuliiaHerets]

@bfitzexpensify 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]

Edited by proposal-police: This proposal was edited at 2024-08-08 13:17:27 UTC.

Proposal

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

System message shows undefined when list value is changed back to initial value

What is the root cause of that problem?

When selecting the same value (fieldValue === option.text), we assign empty string to fieldKey and pass it to the submit function https://github.com/Expensify/App/blob/9c7c4a1cb9405f64c31c03c4f1fa33c0c041dac0/src/pages/EditReportFieldDropdown.tsx#L111 Then when value is an empty string we assign null to value prop, update the BE with with a new reportField that has null as a value https://github.com/Expensify/App/blob/9c7c4a1cb9405f64c31c03c4f1fa33c0c041dac0/src/pages/EditReportFieldPage.tsx#L76-L82

Then BE returns a report action object that have null on originalMessage.newValue prop

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

We should remove

|| fieldValue === option.text

from https://github.com/Expensify/App/blob/9c7c4a1cb9405f64c31c03c4f1fa33c0c041dac0/src/pages/EditReportFieldDropdown.tsx#L111 Or we should change it to

 onSelectRow={(option) => onSubmit({[fieldKey]: option?.text  ??  '' })}

What alternative solutions did you explore? (Optional)

We can dismiss the modal when value is an empty string

if (value) {
ReportActions.updateReportField(report.reportID, {...reportField, value}, reportField);
            }
Navigation.dismissModal(report?.reportID);

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-08-08 13:11:00 UTC.

Proposal

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

The system message contains "undefined" when the list value is changed back to the initial value.

What is the root cause of that problem?

If the selected value is the current value of the report field, we pass it as empty string here

https://github.com/Expensify/App/blob/4f454243f3673aaa68adf6ce08ecc14dd0752353/src/pages/EditReportFieldDropdown.tsx#L111

Then here we pass the new value of the report field as null https://github.com/Expensify/App/blob/4f454243f3673aaa68adf6ce08ecc14dd0752353/src/pages/EditReportFieldPage.tsx#L82

So BE returns the new value in report action as null. Then when merging data to Onyx, the newValue field is deleted since it has null value.

It leads newValue being undefined when we get the message here https://github.com/Expensify/App/blob/4f454243f3673aaa68adf6ce08ecc14dd0752353/src/libs/ReportActionsUtils.ts#L1223

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

We should add another case when the new value doesn't exist to display a message like remove the ${fieldName} (previously "${oldValue}"). We also need to create a new translation key for this message

if (!newValue) {
    return // translated message;
}

https://github.com/Expensify/App/blob/4f454243f3673aaa68adf6ce08ecc14dd0752353/src/libs/ReportActionsUtils.ts#L1220

What alternative solutions did you explore? (Optional)

We can do nothing if the user select the selected value

onSelectRow={(option) => {
    if (fieldValue === option.text || !option?.text) {
        return;
    }
    onSubmit({[fieldKey]: option.text})
}} 

https://github.com/Expensify/App/blob/4f454243f3673aaa68adf6ce08ecc14dd0752353/src/pages/EditReportFieldDropdown.tsx#L111-L112

[nkdengineer]

Updated proposal

• Detail the RCA

[etCoderDysto]

~Updated proposal~

• ~Added an alternative solution~

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[dukenv0307]

@nkdengineer's proposal LGTM. We should fallback to the default value as mentioned in step 12.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[etCoderDysto]

@dukenv0307 Can we please ask someone from internal to confirm the expected behaviour? From UX perspective, It doesn't make sense to select the default value when the user is selecting another report field twice. Especially when the user can select the default value if they want.

Report field values are 1, 2, 3. And the default value is 1 in the video. When selecting '2' twice, the value reverts to '1' which is the default value. If the user want to change the value to '1', they can select it form the list.

https://github.com/user-attachments/assets/8dc5f09e-b631-41ad-afee-999c3249c3e3

cc: @grgia @mountiny

[mountiny]

@etCoderDysto can you please write this bug in numbered steps? That will be easier to understand

[etCoderDysto]

@mountiny If we implement The selected solution, the result will be following

Steps

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Submit two expenses.
4. Go to workspace settings > Report fields (enable in More features).
5. Add a list type report field, with some list values and an initial value. (in my video the initial value is '1')
6. Return to the workspace chat.
7. Go to expense report.
8. Click on the list report field.
9. Select a different list value. (in my video the value is 2)
10. Click on the list report field.
11. Click on the already selected value (in my video the value is 2)

Expected Result: The selected report field shouldn't fallback to default report field, and it should be the selected one which is ('2') Actual result: The selected report field falls back to the initial report field('1') when user selects a report field that is already selected

Attachment:

https://github.com/user-attachments/assets/8dc5f09e-b631-41ad-afee-999c3249c3e3

[mountiny]

@etCoderDysto got it, I agree with that, we should not change to the default.

Can we just close the panel and do nothing when user clicks on the already selected report field?

[nkdengineer]

@mountiny

If we want to do nothing, I believe the proposal from @etCoderDysto doesn't get this expected behavior. This proposal makes the API called then a report action is created with the message displayed like change report field from value1 to value1

I updated my proposal with alternative solution to do that

[etCoderDysto]

Can we just close the panel and do nothing when user clicks on the already selected report field?

@mountiny When the user selects existing report field, handleReportFieldChange is called with value prop assigned to an empty string. We can check if 'value' is empty string and dismiss the modal when it is. But I have to check if this change won't introduce a regression since handleReportFieldChange is called when editing Date and Text report fields

if (value) {
  ReportActions.updateReportField(report.reportID, {...reportField, value}, reportField);reportField);
            }
Navigation.dismissModal(report?.reportID);

https://github.com/Expensify/App/blob/9c7c4a1cb9405f64c31c03c4f1fa33c0c041dac0/src/pages/EditReportFieldPage.tsx#L76-L82

Alternatively, my proposal states that we should call updateReportField, with the existing report field value when user selects existing report field.

[nkdengineer]

@mountiny Additional I think we can do this because for tag or category, we can de-select the select tag/category when we click on the selected option. So I think can do the same for report field.

[etCoderDysto]

@mountiny

If we want to do nothing, I believe the proposal from @etCoderDysto doesn't get this expected behavior. This proposal makes the API called then a report action is created with the message displayed like change report field from value1 to value1

I updated my proposal with alternative solution to do that

It will make the API call, but there will be no action messages. The action message is only rendered when there is a change in value. And the alternative solution you just proposed won't close the modal, since dismmisModal is called only in handleReportFieldChange

[melvin-bot[bot]]

Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.

[mountiny]

@dubielzyk-expensify curious for your input on this expected behaviour. You can see the steps in the op. The question is whether we should change the value back to the initial value when user clicks on the already selected value. This does not feel right to me

[etCoderDysto]

Updated my proposal with an alternative solution to close the modal when report field is not changed

[dubielzyk-expensify]

@dubielzyk-expensify curious for your input on this expected behaviour. You can see the steps in the op. The question is whether we should change the value back to the initial value when user clicks on the already selected value. This does not feel right to me

Yeah, I'm with you. I don't think we should do that. Any click on an item in a select list should result in that item being selected. We do use "deselecting" in that sorta way.

[dukenv0307]

will give the final review in a hour

[nkdengineer]

We do use "deselecting" in that sorta way.

@dubielzyk-expensify If we deselecting in this case what do you think if we display a message like remove the ${fieldName} (previously "${oldValue}") as the image below?

cc @mountiny And we're displaying the report field with fallback as the default value. That causes some confusion when reportField.value is undefined but it's displayed as the default value https://github.com/Expensify/App/blob/420c2a13cc176dca5adf8dcc8132ef5f8ff678f7/src/components/ReportActionItem/MoneyReportView.tsx#L93

[dubielzyk-expensify]

Sorry that was meant to say "we don't use deselecting in that sorta way". For select lists there is no deselecting (cc @Expensify/design in case I've missed something). Tapping the same value should fill the value that's selected.

[nkdengineer]

Thanks.

[dukenv0307]

To sum it up:

Our expectation changes to keep the selected value (The previous expectation is to use the initial value)

So I would like to choose @etCoderDysto's proposal

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[shawnborton]

Ah interesting, I think my understanding was the opposite @dubielzyk-expensify in that you would select the currently selected value to unselect it.

So I think what's happening in the original bug report is okay, we just need to figure out the right copy for the case where you remove the current value of a report field.

I tend to like the idea about using a pattern like remove the ${fieldName} (previously "${oldValue}") - that seems to work well here.