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

IuliiaHerets · 2024-08-08T12:38:31Z

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:

Go to staging.new.expensify.com
Go to workspace chat.
Submit two expenses.
Go to workspace settings > Report fields (enable in More features).
Add a list type report field, with some list values and an initial value.
Return to the workspace chat.
Go to expense report.
Click on the list report field.
Select a different list value.
Revisit the report.
Click on the list report field.
Click on the already selected value (so that it will change to the initial value).
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:

Screenshots/Videos

Bug6565437_1723109214472.20240808_172103.mp4

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
etCoderDysto | Contributor | 105612296

Issue Owner

Current Issue Owner: @grgia

melvin-bot · 2024-08-08T12:38:42Z

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 · 2024-08-08T12:38:53Z

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

IuliiaHerets · 2024-08-08T12:39:12Z

@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 · 2024-08-08T13:00:18Z

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

App/src/pages/EditReportFieldDropdown.tsx

Line 111 in 9c7c4a1

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

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

App/src/pages/EditReportFieldPage.tsx

Lines 76 to 82 in 9c7c4a1

    
           const handleReportFieldChange = (form: FormOnyxValues<typeof ONYXKEYS.FORMS.REPORT_FIELDS_EDIT_FORM>) => { 
        
               const value = form[fieldKey]; 
        
               if (isReportFieldTitle) { 
        
                   ReportActions.updateReportName(report.reportID, value, report.reportName ?? ''); 
        
                   Navigation.goBack(); 
        
               } else { 
        
                   ReportActions.updateReportField(report.reportID, {...reportField, value: value === '' ? null : value}, reportField);

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

App/src/pages/EditReportFieldDropdown.tsx

Line 111 in 9c7c4a1

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

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 · 2024-08-08T13:04:06Z

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

App/src/pages/EditReportFieldDropdown.tsx

Line 111 in 4f45424

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

Then here we pass the new value of the report field as null

App/src/pages/EditReportFieldPage.tsx

Line 82 in 4f45424

    
           ReportActions.updateReportField(report.reportID, {...reportField, value: value === '' ? null : value}, reportField);

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

App/src/libs/ReportActionsUtils.ts

Line 1223 in 4f45424

    
           return Localize.translateLocal('report.actions.type.changeField', {oldValue, newValue, fieldName});

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;
}

App/src/libs/ReportActionsUtils.ts

Line 1220 in 4f45424

if (!oldValue) {

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

App/src/pages/EditReportFieldDropdown.tsx

Lines 111 to 112 in 4f45424

    
           onSelectRow={(option) => onSubmit({[fieldKey]: !option?.text || fieldValue === option.text ? '' : option.text})} 
        
           initiallyFocusedOptionKey={selectedOptionKey ?? undefined}

nkdengineer · 2024-08-08T13:11:12Z

Updated proposal

Detail the RCA

etCoderDysto · 2024-08-08T13:17:59Z

~~Updated proposal~~

~~Added an alternative solution~~

melvin-bot · 2024-08-12T10:26:26Z

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

melvin-bot · 2024-08-12T10:26:30Z

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

melvin-bot · 2024-08-12T10:52:42Z

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

etCoderDysto · 2024-08-12T11:16:16Z

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

Screen.Recording.2024-08-12.at.2.12.40.in.the.afternoon.mp4

cc: @grgia @mountiny

mountiny · 2024-08-12T13:42:49Z

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

etCoderDysto · 2024-08-12T13:53:22Z

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

Steps

Go to staging.new.expensify.com
Go to workspace chat.
Submit two expenses.
Go to workspace settings > Report fields (enable in More features).
Add a list type report field, with some list values and an initial value. (in my video the initial value is '1')
Return to the workspace chat.
Go to expense report.
Click on the list report field.
Select a different list value. (in my video the value is 2)
Click on the list report field.
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:

Screen.Recording.2024-08-12.at.2.12.40.in.the.afternoon.mp4

mountiny · 2024-08-12T13:55:29Z

@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 · 2024-08-12T14:01:45Z

@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 · 2024-08-12T14:04:15Z

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

App/src/pages/EditReportFieldPage.tsx

Lines 76 to 82 in 9c7c4a1

    
           const handleReportFieldChange = (form: FormOnyxValues<typeof ONYXKEYS.FORMS.REPORT_FIELDS_EDIT_FORM>) => { 
        
               const value = form[fieldKey]; 
        
               if (isReportFieldTitle) { 
        
                   ReportActions.updateReportName(report.reportID, value, report.reportName ?? ''); 
        
                   Navigation.goBack(); 
        
               } else { 
        
                   ReportActions.updateReportField(report.reportID, {...reportField, value: value === '' ? null : value}, reportField);

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

nkdengineer · 2024-08-12T14:05:25Z

@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 · 2024-08-12T14:14:04Z

@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

grgia · 2024-12-04T16:53:16Z

@dukenv0307

Could you take a look at Role on a member in a workspace as a reference for behavior.

We'd like to mimic the behavior there where clicking the currently selected value just does nothing and navigates back. Basically like “reselecting” it.

nkdengineer · 2024-12-05T05:53:06Z

@grgia Based on the discussion above here. We want to use deselecting behavior like we do for category/tag and will display a violation after deselecting the selected report field list value.

So we need to make some changes:

We will not display the default value as a fallback here

App/src/components/ReportActionItem/MoneyReportView.tsx

Line 115 in e25e3fa

const fieldValue = reportField.value ?? reportField.defaultValue;

When we create a new expense report, the value in reportField of the expense report should be the defaultValue of the policy reportField if it exists. With this when we create a new expense report, the default value can still display as reportField.value not reportField.defaultValue
When we edit the report field, If we select the selected report field, we will send the value as null to the backend. In this case, the backend doesn't return a report violation now and we need to return a report violation in the case value is changed to null because we will not display reportField.defaultValue anymore.

trjExpensify · 2024-12-05T22:38:21Z

Hm, but given that there always has to be a value for the report field (consistent with OldDot behaviour and what the backend expects) why don't we treat it like the Role select list where selecting the value already selected, doesn't deselect. That would avoid all of the downstream complications with "undefined", silently still setting the default value, or weird system messages that look broken.

2024-12-05_22-34-46.mp4

grgia · 2024-12-09T13:51:19Z

@nkdengineer any thoughts on the above?

etCoderDysto · 2024-12-09T14:05:07Z

@grgia When we select an already selected report field, we can avoid making an API request, and the already selected report field will remain selected. If we do that, we don't have to make BE changes to return a violation since we will not be submitting a report field with a null value. @dukenv0307 Can you please take a look at my proposal? In alternative solution, It suggests that we should close the modal without making an API request when the user selects an already selected report field. You can take a look at the result of my proposal in this video

Thanks!

grgia · 2024-12-09T19:22:43Z

Bump @dukenv0307

nkdengineer · 2024-12-09T19:35:00Z

@nkdengineer any thoughts on the above?

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.

@grgia I suggest this behavior here because the backend still accepts changing the dropdown report field to empty. The context is here: #47067 (comment)

dukenv0307 · 2024-12-10T03:08:28Z

@grgia @etCoderDysto I think we already discussed this issue before here then decided to go with @nkdengineer's idea

dukenv0307 · 2024-12-10T03:27:56Z

I think we should ask design team for the final decision

shawnborton · 2024-12-10T13:46:08Z

I like @trjExpensify 's suggestion above.

dannymcclain · 2024-12-10T14:30:58Z

I also like @trjExpensify's suggestion.

grgia · 2024-12-11T17:07:30Z

Yep let's stick with that behavior @dukenv0307

dukenv0307 · 2024-12-20T08:45:53Z

Seem like we want to change the expectation based on this comment. So @etCoderDysto's proposal is valid now. But should we re-assign to @etCoderDysto or assign both of them since @nkdengineer's proposal was selected before and he spent time on it? cc @grgia

grgia · 2024-12-20T17:37:40Z

@dukenv0307 given the scope changes, we should look at the proposals again holistically. Given the convo and participation, what would you suggest?

dukenv0307 · 2024-12-21T04:54:03Z

@grgia We should go with @etCoderDysto's alternative solution

etCoderDysto · 2025-01-03T11:18:17Z

@grgia A kind reminder to take a loo at this comment ↑

dukenv0307 · 2025-01-07T03:25:05Z

@grgia friendly bump

grgia · 2025-01-08T16:56:59Z

Great, thank you!

melvin-bot · 2025-01-08T16:57:22Z

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

etCoderDysto · 2025-01-11T12:49:20Z

@dukenv0307 PR is ready for review

IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 8, 2024

melvin-bot bot assigned bfitzexpensify Aug 8, 2024

melvin-bot bot added the Overdue label Aug 12, 2024

bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 12, 2024

melvin-bot bot changed the title ~~Report fields -System message shows undefined when list value is changed back to initial value~~ [$250] Report fields -System message shows undefined when list value is changed back to initial value Aug 12, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 12, 2024

melvin-bot bot assigned dukenv0307 Aug 12, 2024

melvin-bot bot removed the Overdue label Aug 12, 2024

This comment was marked as outdated.

Sign in to view

melvin-bot bot assigned grgia Aug 12, 2024

bfitzexpensify added this to [#whatsnext] #expense Aug 12, 2024

cead22 self-assigned this Dec 4, 2024

melvin-bot bot added the Overdue label Dec 20, 2024

melvin-bot bot removed the Overdue label Dec 20, 2024

melvin-bot bot added the Overdue label Dec 30, 2024

melvin-bot bot removed the Overdue label Jan 8, 2025

grgia assigned etCoderDysto Jan 8, 2025

etCoderDysto mentioned this issue Jan 11, 2025

fix: System message shows undefined when selecting an already selected list value #55118

Open

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Jan 11, 2025

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

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

Comments

IuliiaHerets commented Aug 8, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Aug 8, 2024

IuliiaHerets commented Aug 8, 2024

IuliiaHerets commented Aug 8, 2024

etCoderDysto commented Aug 8, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

nkdengineer commented Aug 8, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

nkdengineer commented Aug 8, 2024

etCoderDysto commented Aug 8, 2024 • edited Loading

melvin-bot bot commented Aug 12, 2024

melvin-bot bot commented Aug 12, 2024

This comment was marked as outdated.

melvin-bot bot commented Aug 12, 2024

etCoderDysto commented Aug 12, 2024 • edited Loading

mountiny commented Aug 12, 2024

etCoderDysto commented Aug 12, 2024

mountiny commented Aug 12, 2024

nkdengineer commented Aug 12, 2024

etCoderDysto commented Aug 12, 2024 • edited Loading

nkdengineer commented Aug 12, 2024

etCoderDysto commented Aug 12, 2024 • edited Loading

grgia commented Dec 4, 2024

nkdengineer commented Dec 5, 2024

trjExpensify commented Dec 5, 2024

grgia commented Dec 9, 2024

etCoderDysto commented Dec 9, 2024 • edited Loading

grgia commented Dec 9, 2024

nkdengineer commented Dec 9, 2024

dukenv0307 commented Dec 10, 2024 • edited Loading

dukenv0307 commented Dec 10, 2024

shawnborton commented Dec 10, 2024

dannymcclain commented Dec 10, 2024

grgia commented Dec 11, 2024

dukenv0307 commented Dec 20, 2024

grgia commented Dec 20, 2024

dukenv0307 commented Dec 21, 2024

etCoderDysto commented Jan 3, 2025

dukenv0307 commented Jan 7, 2025

grgia commented Jan 8, 2025

melvin-bot bot commented Jan 8, 2025

etCoderDysto commented Jan 11, 2025

IuliiaHerets commented Aug 8, 2024 •

edited by melvin-bot bot

Loading

etCoderDysto commented Aug 8, 2024 •

edited

Loading

nkdengineer commented Aug 8, 2024 •

edited

Loading

etCoderDysto commented Aug 8, 2024 •

edited

Loading

etCoderDysto commented Aug 12, 2024 •

edited

Loading

etCoderDysto commented Aug 12, 2024 •

edited

Loading

etCoderDysto commented Aug 12, 2024 •

edited

Loading

etCoderDysto commented Dec 9, 2024 •

edited

Loading

dukenv0307 commented Dec 10, 2024 •

edited

Loading