Add CSV import flow to Categories page#47827
Conversation
melvin-bot
bot
requested review from
rushatgabhane
and removed request for
a team
|
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@rushatgabhane - I think it is mostly ready for review, only on Chrome on Android there is an issue with |
|
Can we check the spacing of this empty state? The illustration feels a bit close to the headline. And the text below the headline should use our textSupporting color. Please refer to the Figma for this one. |
|
The border radius for the spreadsheet rows should be 8px, they look a bit bigger in your screenshots though. |
|
When you open up the dropdown menu, I would think the currently selected menu option would have a selected BG color and a checkmark? From the Figma: |
rlinoz
left a comment
rlinoz
left a comment
There was a problem hiding this comment.
This is working very well overall!
| }, | ||
| }, | ||
|
|
||
| CSV_IMPORT_COLUMNS: { |
There was a problem hiding this comment.
There was the idea in the doc to split into constants for each file, should we update to match that?
There was a problem hiding this comment.
I get the idea, but it is not needed IMO and in the function findColumnName we don't know it e.g. field GL Code is from categories or tags, so what should be set there as an attribute? (there is no difference, because they are the same) Example below:
case 'glcode': case 'gl': attribute = CONST.CSV_IMPORT_COLUMNS.CATEGORIES.GL_CODE; break;
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks, the video looks good
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
I have marked the snyk warnings about the xlms as resolved https://app.snyk.io/org/expensify/project/a75415c0-01a0-4906-abb8-070d86e05d58/pr-check/0bd42126-7fbf-4cea-a9f5-e8870001d371 I do not think we need to worry about those |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.29-0 🚀
|
|
This PR is failing for Desktop because of issue #48540 |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.29-12 🚀
|
| iconRight={item.iconRight} | ||
| shouldPutLeftPaddingWhenNoIcon={item.shouldPutLeftPaddingWhenNoIcon} | ||
| label={item.label} | ||
| style={{backgroundColor: item.isSelected ? theme.activeComponentBG : undefined}} |
There was a problem hiding this comment.
Looks like this addition led to the following issue:
Since given the new addition and the fact that in SearchTypeMenuNarrow.tsx we never pass the isSelected property. The issue was fixed by passing down isSelected which fixed the issue of the selected item highlight. More details in the issue proposal.
| const columnValuesString = column.slice(containsHeader ? 1 : 0).join(', '); | ||
|
|
||
| const colName = findColumnName(column[0]); | ||
| const defaultSelectedIndex = columnRoles.findIndex((item) => item.value === colName); |
There was a problem hiding this comment.
Coming from this issue
In several cases defaultSelectedIndex will return -1 if the index is not found.
As result dropdown button does not match the selection in the dropdown list
And to fix this
A check was added If the index is not found, the default index will be 0
| onConfirm={() => { | ||
| setIsImportingCategories(false); | ||
| closeImportPage(); | ||
| Navigation.navigate(ROUTES.WORKSPACE_CATEGORIES.getRoute(policyID)); |
There was a problem hiding this comment.
Coming from this issue #56936, when we close import page and modal, we're using navigate, which is causing a bug. This adds ImportedCategoriesPage to the navigation stack, so when we go back from Categories, it takes us to ImportedCategoriesPage again. Instead, we could use goBack here. More context on this comment: #56936 (comment)
| const spreadsheetColumns = spreadsheet?.data; | ||
| if (!spreadsheetColumns) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
FYI, This caused #59354 because we're returning undefined when we don't have spreadsheet.
Proposal: #59354 (comment)
| > | ||
| <Button | ||
| text={translate('common.import')} | ||
| onPress={importFunction} |
There was a problem hiding this comment.
missing pressOnEnter prop that caused this bug #64044
| roles.push( | ||
| {text: translate('common.ignore'), value: CONST.CSV_IMPORT_COLUMNS.IGNORE}, | ||
| {text: translate('common.name'), value: CONST.CSV_IMPORT_COLUMNS.NAME, isRequired: true}, | ||
| {text: translate('common.enabled'), value: CONST.CSV_IMPORT_COLUMNS.ENABLED, isRequired: true}, |
| setSpreadsheetData(data as string[][]).then(() => { | ||
| Navigation.navigate(goTo); | ||
| }); | ||
| }) | ||
| .finally(() => { | ||
| setIsReadingFIle(false); |
There was a problem hiding this comment.
We should have handled the error and loading case of setSpreadsheetData like here.
18 participants
Details
This PR add possibility to import Categories from spreadsheet. It uses new library called
xlsxto read the data from files.Fixed Issues
$ #47357
PROPOSAL: N/A
Tests
Import spreadsheetChoose fileOffline tests
Feature not supported offline - info modal should be shown
QA Steps
Same as Test steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android.mov
Android: mWeb Chrome
iOS: Native
ios_failed.mp4
ios_success.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov