[HOLD for payment 2024-09-06] [$250] Report fields - No dropdown button after selecting the list value via checkbox

[lanitochka17]

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: 9.0.17-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
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 settings > Report fields
3. Click Add field
4. Click Type > Select List
5. Click List values
6. Add some list values
7. Select the list values via checkbox

Expected Result:

There will be a dropdown button for enable, disable or delete the value after selecting the list value via checkbox

Actual Result:

There is no dropdown button after selecting the list value via checkbox

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

Bug6563660_1722966650965.20240807_014736.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019f8aa64e5e051072
• Upwork Job ID: 1823390208605053379
• Last Price Increase: 2024-08-13
• Automatic offers:
• ishpaul777 | Reviewer | 103608281
• cretadn22 | Contributor | 103608284

Issue Owner

Current Issue Owner: @ishpaul777

[melvin-bot]

Triggered auto assignment to @greg-schroeder (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.

[lanitochka17]

@greg-schroeder 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

[lanitochka17]

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

[Krishna2323]

Proposal

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

Report fields - No dropdown button after selecting the list value via checkbox

What is the root cause of that problem?

?? instead of ||

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

Line 180 in 7975157

if ((isSmallScreenWidth && selectionMode?.isEnabled) ?? selectedValuesArray.length > 0) {

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

Use || instead of ?? for if block.

 if (!!(isSmallScreenWidth && selectionMode?.isEnabled) || selectedValuesArray.length > 0) {

What alternative solutions did you explore? (Optional)

The toggleAllValues is also incorrect.

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

Lines 119 to 123 in 7975157

	const toggleAllValues = () => {
	const isAllSelected = listValues.length === Object.keys(selectedValues).length;
	setSelectedValues(isAllSelected ? {} : Object.fromEntries(listValues.map((value) => [value, true])));
	};

It should be

    const toggleAllValues = () => {
        const isAllSelected = listValues.every((key) => !!selectedValues[key]);
        setSelectedValues(isAllSelected ? {} : Object.fromEntries(listValues.map((value) => [value, true])));
    };

[cretadn22]

Proposal

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

No dropdown button after selecting the list value via checkbox

What is the root cause of that problem?

Since isSmallScreenWidth and selectionMode?.isEnabled are both false in this case, the condition will also be false.

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

Line 180 in ed6f5c1

if ((isSmallScreenWidth && selectionMode?.isEnabled) ?? selectedValuesArray.length > 0) {

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

We expect that

• when isSmallScreenWidth is false, we need to use selectedValuesArray.length > 0 condition
• when isSmallScreenWidth is true, we need to use selectionMode?.isEnabled condition

Therefore, we need to update it to be consistent with other pages

if (isSmallScreenWidth ? selectionMode?.isEnabled : selectedValuesArray.length > 0) 

---

There is another issue with the toggle all button

43.mov

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

Line 120 in ed6f5c1

const isAllSelected = listValues.length === Object.keys(selectedValues).length;

This issue is due to an incorrect condition. The selectedValues will include all values, regardless of whether they are selected or not.

We need to use selectedValuesArray.length instead of Object.keys(selectedValues).length.

[melvin-bot]

@greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[melvin-bot]

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

[ishpaul777]

~Reviewing 👀 ~

Edit: Sorry I had to head out for a while

[Krishna2323]

@ishpaul777, friendly bump for a review.

[ishpaul777]

The Orignal issue here is fixed by #46698, but the other issue issue mention in @cretadn22 proposal is still reproducable. We can fix that here. @cretadn22's solution for the other issue looks good to me!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[Krishna2323]

@ishpaul777, what do you think about my alternative solution? My proposal identified the other issue before the other proposal.

[ishpaul777]

Your solution is correct but I chose the solution from @cretadn22 because time complexity is O(1) while your solution will have O(n).

[Krishna2323]

@ishpaul777, I don't think that matters much in this case, the same logic is also used in WorkspaceViewTagsPage.

App/src/pages/workspace/tags/WorkspaceViewTagsPage.tsx

Line 121 in 9940e99

const isAllSelected = tagList.every((tag) => !!selectedTags[tag.value]);

[ishpaul777]

I'll let assigned CME make the final call on assignment

[Krishna2323]

@ishpaul777, even if we consider performance, my solution is generally better in performance. Just wanted to note it down because I had a doubt about that. Also the time complexity is O(m) not O(1) because of Object.keys(selectedValues), where m is the length of selected value.

cc: @puneetlath

[cretadn22]

selectedValuesArray is already defined, so we don't need to recalculate it.

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

Line 110 in ed6f5c1

const selectedValuesArray = Object.keys(selectedValues).filter((key) => selectedValues[key]);

[Krishna2323]

Still the performance difference will be very minor because of early termination, so I don't think the proposal should be selected based on that, I had identified the bug first and provided a valid solution first and the solution is also used in other place. Therefore, I see no reason to select a proposal solely based on time complexity, especially since the list will never be excessively large.

EDIT: I also believe that performance, code refactoring, etc., are meant to be discussed during the PR phase.

[melvin-bot]

@puneetlath, @greg-schroeder, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ishpaul777]

pending final proposal review from @puneetlath

[melvin-bot]

@puneetlath @greg-schroeder @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[puneetlath]

Sorry, @ishpaul777 would you mind summarizing the considerations for me?

[ishpaul777]

The original issue in issue description is fixed already, but @Krishna2323 found another bug (Video in @cretadn22's proposal alternative #46911 (comment))

i found @cretadn22 proposal solution a bit better performance wise since it just compare length of both array while @Krishna2323 traverse through array until it founds odd one out.

Krishna proposed -> const isAllSelected = listValues.every((key) => !!selectedValues[key]);
Creta proposed -> const isAllSelected = listValues.length === selectedValuesArray.length;

Please choose : )

[puneetlath]

I think both are valid solutions and would work fine, so this is ultimately a subjective opinion. And I know someone will end up disappointed, so I'm sorry about that, but there are always new issues and plenty of opportunities.

I prefer this solution as I think it's just easier to read/follow: const isAllSelected = listValues.length === selectedValuesArray.length;

Also, small note for when you make the PR, I think proper grammar would be areAllSelected

[melvin-bot]

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

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

[greg-schroeder]

PR deployed to staging. Awaiting deploy to prod

[ishpaul777]

Deployed on Aug 29 with this #48035 checklist, Pay date - Sept 6

[greg-schroeder]

Thanks, will prep for 9/6

[greg-schroeder]

Processing

[cretadn22]

@greg-schroeder What’s the status of the payment?

[greg-schroeder]

Payments were completed.