[Search] Add Export CSV and handle offline for all actions#45468
[Search] Add Export CSV and handle offline for all actions#45468luacmartins merged 10 commits intoExpensify:mainfrom
Conversation
|
@ikevin127 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] |
Kicu
left a comment
Kicu
left a comment
There was a problem hiding this comment.
Looks moreless ok, left some comments
src/libs/actions/Search.ts
Outdated
| export {search, createTransactionThread, deleteMoneyRequestOnSearch, holdMoneyRequestOnSearch, unholdMoneyRequestOnSearch}; | ||
| type Params = Record<string, string | string[]>; | ||
|
|
||
| function exportSearchItemsToCSV(query: string, reportIDList: string[] | undefined, transactionIDList: string[], policyIDs: string[]) { |
There was a problem hiding this comment.
we will very soon remove query, do you how this will work then? Could you please ping here whoever is responsible for backend for CSV?
in Search v2.1 we're moving to a parser and AST object for search
There was a problem hiding this comment.
This will need to keep parity with how search works, so we will need to update it to use the same AST object when it is available. I am not sure if this is part of Search v2.1 as well, but I think it should be since export is tied to search.
There was a problem hiding this comment.
my understanding is that switching Search to use AST is within v2.1 and some of us in SWM are currently working on this "migration" already. This is one of the issues you can monitor: #45027
Will keep this in mind and communicate when we will be doing the switch.
There was a problem hiding this comment.
Cool, sounds good. Glad it's on everyone's radar for when that is!
There was a problem hiding this comment.
We're moving to the jsonQuery AST in Search v2.1 and have already implemented parts of the changes, currently working on the final switch to the new syntax, so we'll need to update this code to work with the syntax.
trjExpensify
requested review from
rushatgabhane
and removed request for
ikevin127
|
@rushatgabhane has been C+'ing this project, so I'm switching him in for a review now as we're trying to get this deployed through to prod by the end of the week before the conference on Monday. 👍 |
|
Adding you for a review as well, @rlinoz! |
src/libs/actions/Search.ts
Outdated
| const queryString = Object.entries(finalParameters) | ||
| .map(([key, value]) => `${key}=${Array.isArray(value) ? value.join(',') : String(value)}`) | ||
| .join('&'); |
There was a problem hiding this comment.
Hmm couldn't this hit the url limit of 2048 chars? Each transactionID is ~19 chars long, so if the user selects ~100 transactions we'll hit that limit already, no?
There was a problem hiding this comment.
That is a good point, @filip-solecki I think we will have to go back to the previous idea of finding a way to send a post in the native apps for downloads.
There was a problem hiding this comment.
I had this issue at prev full time work. I remember writing a hack to create an a tag, setting the post response as the target for the a tag and clicking it using javascript
const response = postRequest(url, contentType: 'text/csv', body);
const blob = await response.blob();
const a = document.createElement('a');
a.href = URL.createObjectURL(blob);
a.download = 'fileName.csv';
document.body.appendChild(a);
a.click();
For native we could do something similar
RNFetchBlob.config({}).postRequest(url, contentType: 'text/csv', body)
There was a problem hiding this comment.
Are we sure that the lib can't handle post requests? It seems to define other types and handle a body param in fetch
There was a problem hiding this comment.
We are using the downloadManager which according to the docs doesn't support a POST.
I think we should try to download without the downloadManager set to true and check what happens, but let's do it in the other issue so we can get this done by EOW.
There was a problem hiding this comment.
I think it's worth a shot with useDownloadManager: false the downside is that it won't show progress, notifications, etc.
Is the issue only on android?
There was a problem hiding this comment.
cc @Julesssss maybe you might have some insights on using android's Download Manager and passing params to the request (not in the URL)
There was a problem hiding this comment.
both Android and iOS. Desktop also needs refactor but it is more likely to fix it quickly there
There was a problem hiding this comment.
I don't recall any specifics regarding the download manager GET limitation. Is it essential we use the native download manager lib? it has nice functionality but perhaps we can use an alternative method to download the CSV file (I think that is what we are trying to do here?) then show our own custom notification
There was a problem hiding this comment.
Exactly, this is what I am working on right now, downloading seems to be working on iOS right now
rlinoz
left a comment
rlinoz
left a comment
There was a problem hiding this comment.
It is working well overall, lets just polish a few things
| } | ||
|
|
||
| const itemsToDelete = Object.keys(selectedItems ?? {}).filter((id) => selectedItems[id].canDelete); | ||
| const options: Array<DropdownOption<SearchHeaderOptionValue>> = [ |
There was a problem hiding this comment.
Since we are doing the native elsewhere, lets add the download option only if we are on web for now.
| body: formData, | ||
| }; | ||
|
|
||
| return fetch(url, fetchOptions) |
There was a problem hiding this comment.
Currently if some error occurs we are opening the the url in a new tab (line 59), lets show the generic error modal for the CSV download case
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
7 participants
Details
Fixed Issues
$ #44576
PROPOSAL:
Tests
Offline tests
QA 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop