[$250] Report fields - Incorrect list value editor opens after renaming the list 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: 9.0.39-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh010901@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. Click Type and select List.
5. Click List values > Add value.
6. Add the following values - B, C and D.
7. Click on list value C.
8. Click Name field.
9. Change the name to A and save it.

Expected Result:

List value A (previously C) will open after renaming list value C to A.

Actual Result:

List value B opens after renaming list value C to A.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6609930_1726837303619.20240920_205727.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021837218246278248735
• Upwork Job ID: 1837218246278248735
• Last Price Increase: 2024-09-20
• Automatic offers:
• shubham1206agra | Contributor | 104094354

Issue Owner

Current Issue Owner: @rushatgabhane

[melvin-bot]

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

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

[cretadn22]

Proposal

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

Incorrect list value editor opens after renaming the list value

What is the root cause of that problem?

We currently navigate to the report field value page using the index value. However, when we update the report field value, the order of the report field list changes, which also alters the index of the current value.

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

We should indicate the report value page (setting page, edit name page,....) by its name rather than its index value. Here’s what we need to do:

• Update the routes
• Modify the navigate function to accept the name value instead of the index value
• By using the name in the route, we can easily identify the corresponding index and utilize that index value

What alternative solutions did you explore? (Optional)

[nkdengineer]

Proposal

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

List value B opens after renaming list value C to A.

What is the root cause of that problem?

We have an error with method Onyx.merge here, it will merge in alphabetical order

Onyx when merging list like ['c', 'a', 'b'] it saves as ['a', 'b', 'c']

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

We should display based on field name rather than index value by

1. Change 2 routes here and here to use fieldName instead of valueIndex
2. We need to update code in some components ReportFieldsAddListValuePage, ReportFieldsEditValuePage, ReportFieldsValueSettingsPage to use fieldName params
3. We alse need to update navigate here when we edit value of fieldName

What alternative solutions did you explore? (Optional)

Simply, we can recalculate the valueIndex in here base on LIST_VALUES and currentFieldName to navigate to correct route when we edit done

[melvin-bot]

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

[melvin-bot]

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

[rushatgabhane]

@cretadn22 is there some sort of ID that we can use instead of name to navigate?

please see what data we have available for the Onyx key

[cretadn22]

@rushatgabhane When creating the report field list, we store the draft data in workspaceReportFieldForm, and I don't see any additional IDs.

[rushatgabhane]

c+ reviewed @cretadn22's proposal #49546 (comment)

🎀👀🎀

[melvin-bot]

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

[rushatgabhane]

@cretadn22 let's use newName and oldName to set optimistic / success and failure data

[shubham1206agra]

Proposal

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

Report fields - Incorrect list value editor opens after renaming the list value

What is the root cause of that problem?

With introduction of Expensify/react-native-onyx#567, we started to use shared connection value. But in

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

Line 151 in 9160fa5

title={formDraft?.[INPUT_IDS.LIST_VALUES]?.sort(localeCompare)?.join(', ')}

we have used in place sorting which fools the values order when merging here

App/src/libs/actions/Policy/ReportField.ts

Lines 101 to 104 in 513e6b3

	Onyx.merge(ONYXKEYS.FORMS.WORKSPACE_REPORT_FIELDS_FORM_DRAFT, {
	[INPUT_IDS.LIST_VALUES]: [...listValues, valueName],
	[INPUT_IDS.DISABLED_LIST_VALUES]: [...disabledListValues, false],
	});

which make it look like it merged alphabetically due to shared value system in Onyx, it will sort every instance that uses listValues since variable reference never changed.

Onyx when merging list like ['c', 'a', 'b'], it looks like it saved as ['a', 'b', 'c']

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

We will use a copy instead here

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

Line 151 in 9160fa5

title={formDraft?.[INPUT_IDS.LIST_VALUES]?.sort(localeCompare)?.join(', ')}

const listValues = [...(formDraft?.[INPUT_IDS.LIST_VALUES] ?? [])].sort(localeCompare).join(', ');

So it will perform in place sorting on a copy instead

What alternative solutions did you explore? (Optional)

NA

[shubham1206agra]

@rushatgabhane You may need to re-evaluate your decision.

[rushatgabhane]

I think both solutions would work.

Routes based on index is a bit fragile, don't you think? @johnmlee101 @shubham1206agra

which fools the values order when merging here

Because if Onyx changes, and then this hack would not work.

Do you see any cons for the route based on the name of list value? oldName and newName.
We'll have to write more code for Onyx, but other than that?

cc: @cretadn22

[shubham1206agra]

I think both solutions would work.

Routes based on index is a bit fragile, don't you think? @johnmlee101 @shubham1206agra

Do you see any cons for the route based on the name of list value? oldName and newName.

Yeah, there are cons. The first con is to change the route entry in the navigator if someone changes the name. Second con is what if the request fails and the value gets reverted to the old one. It would show not found page.

Plus, why should we rewrite the navigation routes for such a small reason which can be solved easily without any workaround.

[rushatgabhane]

That is a fair point 👍

@johnmlee101 i like @shubham1206agra's proposal. It makes a copy, so even if onyx changes, this solution will still work.

[melvin-bot]

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

[shubham1206agra]

Sorry, I missed the assignment notification. I will raise the PR toady.

[isabelastisser]

Payment summary:

@shubham1206agra $250 for PR, paid in Upwork.
@rushatgabhane $250 for C+ review, payment pending in NewDot.

[isabelastisser]

@rushatgabhane, do we need a regression test?

[rushatgabhane]

yes i think we should add a regression step here because this bug is easy to happen if someone decides to refactor / simplify the logic

[rushatgabhane]

1. The PR that introduced the bug has been identified. Link to the PR: N.A. Original implementation

2. 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: N.A.

3. 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: N.A.

4. Determine if we should create a regression test for this bug. Yes

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

    1. Open App
    2. Go to workspace settings > Report fields.
    3. Click Add field.
    4. Click Type and select List.
    5. Click List values > Add value.
    6. Add the following values - B, C and D.
    7. Click on list value C.
    8. Click Name field.
    9. Change the name to A and save it.
    10. Verify that list value A (previously C) will open after renaming.
   

[isabelastisser]

all set!

[garrettmknight]

$250 approved for @rushatgabhane