[$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 contributors@expensify.com 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): applausetester+kh050806@applause.expensifail.com 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.

[nkdengineer]

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

Updated my main solution with this expected.

[etCoderDysto]

@shawnborton If i am not wrong the report field is a required field. If we unselect a report field, it will fallback to a default initial report field - it won't be empty. If we deselect a value what would happen is show in this video. For context, report field '1', '2', and '3' are report fields, and are displayed on the list. '1' is a default value. If the user unselects a report field of value '2', the report field value defaults back to '1'. I think that would be weird since the user can select '1' from the list.

[nkdengineer]

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

@mountiny What do you think about this idea?

[nkdengineer]

@shawnborton If i am not wrong the report field is a required field. If we unselect a report field, it will fallback to a default initial report field - it won't be empty.

As I mentioned above, it's displayed as the default value because we are falling back on it here. Current reportField.value is undefined. It's also undefined when we create an expense report and log in again.

[nkdengineer]

Another option here is to display a helpful message since we display the default value when the value is empty: reset the ${fieldName} to the default value (previously "${oldValue}").

[shawnborton]

If i am not wrong the report field is a required field. If we unselect a report field, it will fallback to a default initial report field - it won't be empty.

Can't we throw an RBR error for the user so they know they need to select a value? That would feel most consistent with our general form styles I think.

[nkdengineer]

If i am not wrong the report field is a required field. If we unselect a report field, it will fallback to a default initial report field - it won't be empty.

Currently, BE will not return the report violation if this report field has the default value.

[etCoderDysto]

Can't we throw an RBR error for the user so they know they need to select a value? That would feel most consistent with our general form styles I think.

For text report fields, we display RBR when the user didn't enter a value. The reason we display RBR in transaction details page is that user can create a text report field without an initial value in workspace settings page. But for a list report field, which we are working on here, the user can't create a list report field without an initial value in workspace settings page. Then that initial value will be used as a default value on transaction details page or combined report. When the user tries to change the list value, a list of options are provided and we update the list report field by the option the user selects.

https://github.com/user-attachments/assets/5506badc-ae55-4aca-975e-07ed9bb3ad3e

[shawnborton]

Got it, thanks for the additional context. So it sounds like for list selection report fields, we just shouldn't allow the user to ever deselect the currently selected item? They can only change the currently selected item to something else, but tapping on the currently selected item wouldn't deselect it. Let me know if that sounds right.

[etCoderDysto]

Got it, thanks for the additional context. So it sounds like for list selection report fields, we just shouldn't allow the user to ever deselect the currently selected item? They can only change the currently selected item to something else, but tapping on the currently selected item wouldn't deselect it. Let me know if that sounds right.

Yes, that sounds to be right.

[nkdengineer]

Currently, BE will not return the report violation if this report field has the default value.

@shawnborton For the list report field when we change the value to empty BE doesn't return any error because it has the default value. For other fields like text, the default value can be empty, and when we create a new expense report we already have a violation like this.

https://github.com/user-attachments/assets/a190e63b-cac5-4492-82c6-ccc469fc2fc7

[shawnborton]

For the list report field when we change the value to empty BE doesn't return any error because it has the default value.

Can you show me what this looks like though? Are you saying we can change the value to nothing in the front end UI?

[nkdengineer]

Currently, when we select on the selected item, the data from BE returns with newValue as null.

From the UI the default value is displayed because if it's empty we will display the default value. In this case A is the default value.

https://github.com/user-attachments/assets/32af8737-4965-4540-aa05-17807ce2dfbb

[shawnborton]

Got it. So yeah, maybe we just keep the behavior you are showing in that latest video but we just need to fix the system message then.

[etCoderDysto]

@shawnborton when the user selects the same report field, we can do nothing - no BE request, no system message, no reseting the value to default value. That should fix the system message issue.

https://github.com/user-attachments/assets/b8e9b410-e7e2-430a-b14a-e203a42c0554

[shawnborton]

That also works I suppose and feels more predictable.

[nkdengineer]

Since BE allows changing the value of the list report field to empty, I think we should keep this behavior as we do for tag/category (For tag/category if we setting required, we can still change this to empty and display a violation about the requirement) and just need to fix the system message for more useful.

We're finding the best behavior and UI, I'm happy with any final decision.

https://github.com/user-attachments/assets/f4ac2ed8-1847-481d-ba55-9937ff7fc8df

[dukenv0307]

@shawnborton What is the final expectation? Thanks

[shawnborton]

Since BE allows changing the value of the list report field to empty, I think we should keep this behavior as we do for tag/category (For tag/category if we setting required, we can still change this to empty and display a violation about the requirement) and just need to fix the system message for more useful.

This is exactly what I was thinking too because it's the most consistent with the rest of the app but I asked for some clarification and @nkdengineer had mentioned that "from the UI the default value is displayed because if it's empty we will display the default value." So it seems like if you deselect the value, we will always replace it with the default value?

Can you clarify what we can and can't do here? My recommendation (as usual!) is to be as consistent with the rest of the app as possible. In the rest of the app, we do allow users to unselect a value for various list selections and we throw an error if you try to submit or save when we actually do require a value to be set.

[dannymcclain]

My recommendation (as usual!) is to be as consistent with the rest of the app as possible. In the rest of the app, we do allow users to unselect a value for various list selections and we throw an error if you try to submit or save when we actually do require a value to be set.

Totally agree with Shawn. If we can keep this behavior consistent with the rest of the app that would be ideal.