[$250] [HOLD for payment 2024-04-05] Workspace - One of the dropdown options configured in OD is missing from ND

[kbecciv]

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: 1.4.56.0
Reproducible in staging?: y
Reproducible in production?: new feature
Issue reported by:

Action Performed:

1. Log into OD with a new expensifail account
2. OD: Create a Collect workspace with chat enabled
3. OD: Navigate to Workspace settings - Reports - Report and Invoice Fields
4. OD: Add a dropdown field with any name and "Expense Report" type
5. OD: Add 3 options under the dropdown "1" "2" and "3"
6. OD: Set "1" as the initial value
7. OD: Invite a new expensifail account to the workspace as member
8. ND: Log in as the member
9. ND: Create a manual IOU in the workspace chat
10. ND: Open the IOU

Expected Result:

All options should be visible.

Actual Result:

One of the dropdown option configured in OD is missing from ND. In this instance, it's "2".

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Add any screenshot/video evidence

bandicam.2024-03-24.00-09-12-512.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @kevinksullivan

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01286e42f8805eb5ba
• Upwork Job ID: 1778519670656045056
• Last Price Increase: 2024-04-11

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @aldo-expensify (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #wave-collect - Release 1

[allgandalf]

Proposal

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

One of the dropdown options configured in OD is missing from ND

What is the root cause of that problem?

We send wrong information to the component in the prop fieldOptions.

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

We must send all the value reportField.values to the component and the EditReportFieldDropdownPage component already has the filtering logic.
We can also apply some filtering if needed in EditReportFieldDropdownPage itself

Note

In my opinion we should disable the search functionality for dropdown if the list is less than 8 in length, this is consistent with categories and tags

Result

[ahmedGaber93]

Proposal

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

Workspace - One of the dropdown options configured in OD is missing from ND

What is the root cause of that problem?

here when we filtering disabledOptions we use this logic !(value in reportField.disabledOptions))

App/src/pages/EditReportFieldPage.tsx

Line 109 in 00e0121

fieldOptions={reportField.values.filter((value) => !(value in reportField.disabledOptions))}

and this the value of values and disabledOptions

values = ["1","2","3"];
disabledOptions = [false, false, false];

this logic is incorrect, because the keyword in search for the index number in the array not for the value(also if it search for the value it incorrect because disabledOptions is bollean[] not string[]).
for example

console.log("1" in ["1"]) // return false because index 1 not found
console.log("0" in ["1"]) // return true because index 0 found

so ["1","2","3"].filter((value) => !(value in [false, false, false])); will return ["3"]

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

we need to change the filter logic to be

reportField.values.filter((_, i) => !reportField.disabledOptions[i])
// or
reportField.values.filter((_, i) => i in reportField.disabledOptions && !reportField.disabledOptions[i])

What alternative solutions did you explore? (Optional)

we can edit disabledOptions in the backend to be string[] and use value and includes to filtering it

[allgandalf]

Note

In my proposal, i mentioned that we should pass the whole dropdown list to the component, and filters like checking for disabled, turning the search bar off if the length is less that 8 is because, there is a case where we might have selected an option which we later disable, if we filter the values at the Component step itself, then we would not be able to display the disabled but selected option in the edit dropdown page, as we never got that value in the component in the first place>

Also this behavior is consistent with the tags and category list as well, we pass all the values to the component and then filter them accordingly

[aldo-expensify]

I think @ahmedGaber93 is right about the problem being the usage of the in operator here. Testing...

[aldo-expensify]

hmm I don't think we necessarily have to be consistent with the CategoryPicker, that seems to be dealing with many specific things related to categories and is more complex. I think it is fine for the design of EditReportFieldDropdownPage to expect that it should show all values passed in fieldOptions

[allgandalf]

As i said the edge case here where we had selected a dropdown option but later we disable it, then in the list we won't be able to show that disabled option as now we don't pass it to the component, this is again a regression as now we will not be able to deselect that disabled option :)

[aldo-expensify]

As i said the edge case here where we had selected a dropdown option but later we disable it, then in the list we won't be able to show that disabled option as now we don't pass it to the component, this is again a regression as now we will not be able to deselect that disabled option :)

hmmmmmmm that is an interesting case, let me see...

[allgandalf]

I'm testing your PR, i'll put up a video there if i found this bug true :)

[aldo-expensify]

I tested and it doesn't seem to cause a problem. If the selected options is not within the list passed to EditReportFieldDropdownPage, the value is shown there anyway:

Screen.Recording.2024-03-25.at.1.09.32.PM.mov

[allgandalf]

Wait no, This is interesting, when i disable options on OD on your branch, i don't see them in all list but rather in recents list, so ig it didn't filter :thinking

2 was disabled here :), i guess we need a more detailed PR

[allgandalf]

I tested and it doesn't seem to cause a problem. If the selected options is not within the list passed to EditReportFieldDropdownPage, the value is shown there anyway:

Oh great then!! we're good to go i guess

[aldo-expensify]

In your screenshot, the recent show 2 and 3, were any of those disabled?

[allgandalf]

I had disabled the 2nd option

[aldo-expensify]

I think you are right about that there is a problem with the "Recent" options. They should be filtered down by the available (enabled) options too, right?

App/src/pages/EditReportFieldDropdownPage.tsx

Lines 92 to 104 in 4caf61a

	const filteredRecentlyUsedOptions = recentlyUsedOptions.filter((option) => option !== selectedValue);
	if (filteredRecentlyUsedOptions.length > 0) {
	newSections.push({
	title: translate('common.recents'),
	shouldShow: true,
	data: filteredRecentlyUsedOptions.map((option) => ({
	text: option,
	keyForList: option,
	searchText: option,
	tooltipText: option,
	})),
	});
	}

[allgandalf]

They should be filtered down by the available (enabled) options too, right?

I guess so, but we both are getting different results on disabling dropdowns so i am assuming there is some different settings for both of us

[aldo-expensify]

I reproduced the "Recent" options bug offering me a disabled option and I fixed it here

@GandalfGwaihir @ahmedGaber93 do you both agree with sharing the reward for fixing this? This would be 250/250? I think both helped sufficiently to split it.

[allgandalf]

Fine with me :), wbu @ahmedGaber93 ?

[ahmedGaber93]

@aldo-expensify fine with me.

[allgandalf]

I think both helped sufficiently to split it.

When are you merging this PR? I am having second thoughts about filtering at the prop level, will do some testing ?!

Or even a Follow up PR would be sound

[aldo-expensify]

I am having second thoughts about filtering at the prop level

Why is that? do you think there may be another bug?

[aldo-expensify]

hmm maybe we should do a useMemo around fieldOptions, otherwise it will be a new array each time we re-render making the this useMemo useless

[aldo-expensify]

Removing Deploy blocker label, the issue is fixed now

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.57-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix filter based on reportField.disabledOptions #38958

If no regressions arise, payment will be issued on 2024-04-05. 🎊

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@aldo-expensify] The PR that introduced the bug has been identified. Link to the PR: It was a new feature feat: add policyReportFields to the policy object directly #36553
• [@aldo-expensify] 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: https://github.com/Expensify/App/pull/36553/files#r1561546435
• [@aldo-expensify] 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: No need
• [@aldo-expensify] Determine if we should create a regression test for this bug: No need, QA caught this bug
• [@aldo-expensify] 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.: No
• [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[allgandalf]

@ahmedGaber93 and i are due payment for this issue according to this comment, c.c. @aldo-expensify @kevinksullivan

[allgandalf]

friendly bump for payment @kevinksullivan , summary over here

[melvin-bot]

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

[melvin-bot]

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

[kevinksullivan]

just using External to get the job created in upwork.

[kevinksullivan]

job created and offer sent to @ahmedGaber93 . Couldn't find @GandalfGwaihir so can you let me know when you apply and I will pay this out? Thanks!

[kevinksullivan]

all set