Open IuliiaHerets opened 1 month ago
melvin-bot[bot]
commented
1 month ago Triggered auto assignment to @kadiealexander (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
commented
1 month ago We think that this bug might be related to #wave-control
IuliiaHerets
commented
1 month ago @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
Nodebrute
commented
1 month ago Edited by proposal-police: This proposal was edited at 2024-10-06 20:13:28 UTC.
Dropdown button does not match the selection in the dropdown list
This issue occurs not only with categories but also with tags. In this case, the defaultSelectedIndex will return -1 if the index is not found.
https://github.com/Expensify/App/blob/6d8e739f7b1c286ae65565a6689b27b5983fe48a/src/components/ImportColumn.tsx#L165
And then we pass this defaultSelectedIndex here, which is incorrect we should pass index 0 which is index of ignore
https://github.com/Expensify/App/blob/6d8e739f7b1c286ae65565a6689b27b5983fe48a/src/components/ImportColumn.tsx#L204
In cases where the defaultSelectedIndex is -1 we should pass 0 here
https://github.com/Expensify/App/blob/6d8e739f7b1c286ae65565a6689b27b5983fe48a/src/components/ImportColumn.tsx#L204
We can do something like this (pseudo-code)
const defaultSelectedIndex = columnRoles.findIndex((item) => item.value === colName);
const finalIndex = defaultSelectedIndex !== -1 ? defaultSelectedIndex : 0;and then we can pass this finalIndex here https://github.com/Expensify/App/blob/6d8e739f7b1c286ae65565a6689b27b5983fe48a/src/components/ImportColumn.tsx#L204
Some cleanup may be required, which we can address in the PR. Result
https://github.com/user-attachments/assets/19886efd-dad6-4280-aaf7-56a0888522a5
[!NOTE]
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.
const defaultSelectedIndex = columnRoles.findIndex((item) => item.value === colName); const finalIndex = defaultSelectedIndex !== -1 ? defaultSelectedIndex : columnRoles.findIndex(item => item.value === 'ignore');
Proposal Updated Added POC
twilight2294
commented
1 month ago Dropdown button does not match the selection in the dropdown list
The dropdown button highlight is selected based on the isSelected prop:
But when we get the defaultSelectedIndex, we are essentially comparing against the colName which is derived from column hence we always get -1 as the return value. This will display the last option in the list as the header value of the dropdown.
the reason this check fails is that the values of colName are:
Hence they will never match with the values of columnRoles:
We should instead get the value from options/ columnRoles itself by checking if isSelected is set to true and if yes them we should show that option as the header:
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
https://github.com/user-attachments/assets/d44301c9-3c50-44f9-84d6-859610b5a587
mkzie2
commented
1 month ago Import categories - Dropdown button does not match the selection in the dropdown list
This bug happens with the column that returns the columnName as empty in the default case here
It leads the defaultSelectedIndex to be -1 which causes the display text in the dropdown button to be different from the selected option in the drop-down menu
https://github.com/Expensify/App/blob/7981265dc14bc9cb99b512baddbb027446568fcf/src/components/ImportColumn.tsx#L165
For the default case, we should return CONST.CSV_IMPORT_COLUMNS.IGNORE here instead of an empty string.
default:
attribute = CONST.CSV_IMPORT_COLUMNS.IGNORE;
break;Job added to Upwork: https://www.upwork.com/jobs/~021843456236485457533
melvin-bot[bot]
commented
1 month ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @ZhenjaHorbach (External)
trjExpensify
commented
1 month ago This is a wave-control project, moving.
ZhenjaHorbach
commented
1 month ago I will check proposals today or tomorrow !
ZhenjaHorbach
commented
1 month ago @Nodebrute @twilight294 @mkzie2 Thanks for proposals !
@Nodebrute @mkzie2
Your proposals look similar (Have the same result)
But I'm not sure that we should stick to a specific value
Because if this value will be missing in a list, issue will be reproduced
I know that now we have Ignore on every Options list at the moment (But I won't be surprised if something changes in the future)
So I would prefer some more flexible solution
@twilight294 Your proposal looks promising But unfortunately it doesn't work for me Could you please provide more code?
Nodebrute
commented
1 month ago @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. Categories-2024-09-10 17_10_20.413.csv
Nodebrute
commented
1 month ago @mkzie2's solution will not work because, in this case, the colName will return glcode. https://github.com/Expensify/App/blob/d6d300798d9430b89d9ac11dea01b10f7c1ff779/src/components/ImportColumn.tsx#L34 And in this case, when we don't pass 'glcode' in the tags, it will return -1, thus reproducing the error. https://github.com/Expensify/App/blob/d6d300798d9430b89d9ac11dea01b10f7c1ff779/src/components/ImportColumn.tsx#L165
@ZhenjaHorbach I have proposed two solutions. The first is to fallback to the 0 index. Let’s assume we don’t have ignore in the future, which I believe will never happen😅, but at the very least, we will have a default value at the 0 index. So, instead of falling back to ignore, we can always fallback to the 0 index.
My second solution, where we can find the index of ignore instead of falling back to 0, is quite reliable. When we set the spreadsheet data, we use ignore, so ignore will remain our default for a long time.
https://github.com/Expensify/App/blob/d6d300798d9430b89d9ac11dea01b10f7c1ff779/src/libs/actions/ImportSpreadsheet.ts#L13
Nodebrute
commented
1 month ago @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.
mkzie2
commented
1 month ago @ZhenjaHorbach Here is why I propose adding the ignore as a fallback in the findColumnName function.
When we import a CSV file, we save the spreadsheet data, and all column names are ignore by default. Then the column name will be updated based on the value in findColumnName function.
It will not update in the case empty string is returned because we already have the check here. But the defaultSelectedIndex is also used to focus and display the correct column value then it makes sense to return ignore by default
https://github.com/Expensify/App/blob/f0be8a731dc0832827dbd14d23fd49b332ff275e/src/components/ImportColumn.tsx#L167-L174
twilight2294
commented
1 month ago @twilight294 Your proposal looks promising But unfortunately it doesn't work for me Could you please provide more code?
oh my bad, actually we have to add extra type prop to DropdownOption here, Below is the test branch and a test csv with categories, do let me know if you have any more doubts:
CSV: Categories-2024-10-10 05_30_44.389.csv
Test PR: - https://github.com/Expensify/App/pull/50542
Nodebrute
commented
1 month ago @twilight294 in your test branch you are using 0 as fallback, similar to my proposal.
Cc: @ZhenjaHorbach
ZhenjaHorbach
commented
1 month ago @mkzie2's solution will not work because, in this case, the colName will return glcode.
And in this case, when we don't pass 'glcode' in the tags, it will return -1, thus reproducing the error. https://github.com/Expensify/App/blob/d6d300798d9430b89d9ac11dea01b10f7c1ff779/src/components/ImportColumn.tsx#L165
@ZhenjaHorbach I have proposed two solutions. The first is to fallback to the
0index. Let’s assume we don’t have ignore in the future, which I believe will never happen😅, but at the very least, we will have a default value at the 0 index. So, instead of falling back toignore, we can always fallback to the0index.My second solution, where we can find the index of
ignoreinstead of falling back to0, is quite reliable. When we set the spreadsheet data, we useignore, soignorewill remain our default for a long time.
Hmmmm Yes Your alternative solution looks interesting And I think it makes sense to use ignore as the default value in case of something
ZhenjaHorbach
commented
1 month ago I think this proposal make sense in which many details are taken into account
So I'm happy to go with this proposal 🎀👀🎀 C+ reviewed
melvin-bot[bot]
commented
1 month ago Triggered auto assignment to @mjasikowski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
twilight2294
commented
1 month ago @ZhenjaHorbach we should always compare with options and not columnRoles, as outlined in my proposal here.
c.c. @mjasikowski
mjasikowski
commented
4 weeks ago @ZhenjaHorbach let's go ahead with @Nodebrute's proposal
melvin-bot[bot]
commented
4 weeks ago 📣 @ZhenjaHorbach 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
melvin-bot[bot]
commented
4 weeks ago 📣 @Nodebrute 🎉 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 📖
mkzie2
commented
4 weeks ago @ZhenjaHorbach Can you please check again? We should update findColumnName function by returning the default value correctly instead of having a fallback like the selected proposal, it doesn't fix the RCA. I also have an explanation here
const defaultSelectedIndex = columnRoles.findIndex((item) => item.value === colName);
const finalIndex = defaultSelectedIndex !== -1 ? defaultSelectedIndex : columnRoles.findIndex(item => item.value === 'ignore');
@mkzie2, your proposal didn't work.
@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. Categories-2024-09-10 17_10_20.413.csv
Here are the steps to test your proposal. You'll see that it doesn't fix the problem.
Nodebrute
commented
4 weeks ago Pr will be ready in few hours
melvin-bot[bot]
commented
3 weeks ago Reviewing label has been removed, please complete the "BugZero Checklist".
melvin-bot[bot]
commented
3 weeks ago The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.51-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-10-29. :confetti_ball:
For reference, here are some details about the assignees on this issue:
melvin-bot[bot]
commented
3 weeks ago 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:
kadiealexander
commented
2 weeks ago Reassigning for someone to handle payment as I'm OOO for the next two weeks.
melvin-bot[bot]
commented
2 weeks ago Triggered auto assignment to @alexpensify (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.
ZhenjaHorbach
commented
2 weeks ago 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:
- [x] [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:
https://github.com/Expensify/App/pull/47827
- [x] [@ZhenjaHorbach] 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/47827/files#r1820299780
- [x] [@ZhenjaHorbach] 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:
NA
- [x] [@ZhenjaHorbach] Determine if we should create a regression test for this bug.
- [x] [@ZhenjaHorbach] 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.
Regression Test Proposal
Do we agree 👍 or 👎
alexpensify
commented
1 week ago Payouts due: 2024-10-29
Upwork job is here. Everyone has been paid in Upwork. I'm keeping this open to complete the regression test.
alexpensify
commented
1 week ago Not overdue
mjasikowski
commented
1 week ago @alexpensify shall we add the Retest Weekly label?
alexpensify
commented
1 week ago Other GHs have been a priority; I need to circle back here and create the regression test GH. This action wouldn't classify for a relabel.
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
https://github.com/user-attachments/assets/48118e97-396a-46ef-a88e-e1e98b26c949
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @kadiealexander