-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2024-10-29] [$250] Import categories - Dropdown button does not match the selection in the dropdown list #50312
Comments
Triggered auto assignment to @kadiealexander ( |
We think that this bug might be related to #wave-control |
@kadiealexander 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 |
Edited by proposal-police: This proposal was edited at 2024-10-06 20:13:28 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Dropdown button does not match the selection in the dropdown list What is the root cause of that problem?This issue occurs not only with categories but also with tags. In this case, the App/src/components/ImportColumn.tsx Line 165 in 6d8e739
And then we pass this defaultSelectedIndex here, which is incorrect we should pass index 0 which is index of ignore App/src/components/ImportColumn.tsx Line 204 in 6d8e739
What changes do you think we should make in order to solve the problem?In cases where the App/src/components/ImportColumn.tsx Line 204 in 6d8e739
We can do something like this (pseudo-code)
and then we can pass this finalIndex here App/src/components/ImportColumn.tsx Line 204 in 6d8e739
Some cleanup may be required, which we can address in the PR. Screen.Recording.2024-10-07.at.1.13.48.AM.movNote There are multiple ways to achieve the same result, but I'm keeping the proposal simple by avoiding excessive options. What alternative solutions did you explore? (Optional)Even though the index of 'ignore' is consistently 0 across all pages, we can implement additional checks to make this more error-proof.
|
Proposal Updated |
ProposalPlease re-state the problem that we are trying to solve in this issue.Dropdown button does not match the selection in the dropdown list What is the root cause of that problem?The dropdown button highlight is selected based on the App/src/components/ImportColumn.tsx Lines 155 to 160 in 134bbd0
But when we get the App/src/components/ImportColumn.tsx Line 165 in 6d8e739
the reason this check fails is that the values of ![]() Hence they will never match with the values of ![]() What changes do you think we should make in order to solve the problem?We should instead get the value from const defaultSelectedIndex = options.findIndex((item) => item.isSelected === true) We should fix this in other places where csv import is used, for cases where none is selected we shouldn't display any value for the header, we should consult with design team for the exact behaviour for this case Screen.Recording.2024-10-07.at.3.07.18.AM.movWhat alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.Import categories - Dropdown button does not match the selection in the dropdown list What is the root cause of that problem?This bug happens with the column that returns the columnName as It leads the App/src/components/ImportColumn.tsx Line 165 in 7981265
What changes do you think we should make in order to solve the problem?For the default case, we should return
App/src/components/ImportColumn.tsx Lines 114 to 116 in 7981265
What alternative solutions did you explore? (Optional) |
Job added to Upwork: https://www.upwork.com/jobs/~021843456236485457533 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ZhenjaHorbach ( |
This is a wave-control project, moving. |
I will check proposals today or tomorrow ! |
@Nodebrute @twilight294 @mkzie2 @Nodebrute @mkzie2 @twilight294 |
@ZhenjaHorbach, @mkzie2's solution will not work. You can try importing this file in the tags import section, and you'll see that the error will still be reproducible with the GL code value. |
@mkzie2's solution will not work because, in this case, the colName will return glcode. App/src/components/ImportColumn.tsx Line 34 in d6d3007
And in this case, when we don't pass 'glcode' in the tags, it will return -1, thus reproducing the error. App/src/components/ImportColumn.tsx Line 165 in d6d3007
@ZhenjaHorbach I have proposed two solutions. The first is to fallback to the My second solution, where we can find the index of App/src/libs/actions/ImportSpreadsheet.ts Line 13 in d6d3007
|
@ZhenjaHorbach Sorry for the message overload! I just checked OD as well, and it seems ignore is the default for everything there too. Let me know what you think about the points above. |
@ZhenjaHorbach Here is why I propose adding the ignore as a fallback in the When we import a CSV file, we save the spreadsheet data, and all column names are App/src/libs/actions/ImportSpreadsheet.ts Lines 12 to 15 in f0be8a7
It will not update in the case empty string is returned because we already have the check here. But the App/src/components/ImportColumn.tsx Lines 167 to 174 in f0be8a7
|
oh my bad, actually we have to add extra type prop to CSV: Categories-2024-10-10 05_30_44.389.csv Test PR: - #50542 |
@twilight294 in your test branch you are using 0 as fallback, similar to my proposal. Cc: @ZhenjaHorbach |
Hmmmm |
I think this proposal make sense in which many details are taken into account So I'm happy to go with this proposal |
Triggered auto assignment to @mjasikowski, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@ZhenjaHorbach we should always compare with c.c. @mjasikowski |
📣 @ZhenjaHorbach 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @Nodebrute 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@ZhenjaHorbach Can you please check again? We should update
|
@mkzie2, your proposal didn't work.
Here are the steps to test your proposal. You'll see that it doesn't fix the problem. |
Pr will be ready in few hours |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.51-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-10-29. 🎊 For reference, here are some details about the assignees on this issue:
|
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:
|
Reassigning for someone to handle payment as I'm OOO for the next two weeks. |
Triggered auto assignment to @alexpensify ( |
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.45-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh0410009@applause.expensifail.com
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
The selection in the dropdown should match what is displayed on the dropdown button.
Actual Result:
The dropdown button shows "Enabled", but the selection in the dropdown is "Ignore".
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6626273_1728232032481.20241007_002106.1.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @kadiealexanderThe text was updated successfully, but these errors were encountered: