feat: implement bulk upload for lookup-tables and case-data #764
Conversation
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
There was a problem hiding this comment.
Couple of quick comments @hunterachieng but I'll let @mtuchi take a closer look at the code
Are we going to deprecate submitXls now?
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
Signed-off-by: Hunter Achieng <[email protected]>
There was a problem hiding this comment.
This looks good @hunterachieng 👍🏽 , I have added unit test on bulk operation for checking the error if type is not case-data or lookup-table
* feat: implement bulk upload for lookup-tables and case-data Signed-off-by: Hunter Achieng <[email protected]> * deleted imports not required Signed-off-by: Hunter Achieng <[email protected]> * fix: remove lookup-table plural Signed-off-by: Hunter Achieng <[email protected]> * reduce example and rename test Signed-off-by: Hunter Achieng <[email protected]> * update example Signed-off-by: Hunter Achieng <[email protected]> * remove try...catch Signed-off-by: Hunter Achieng <[email protected]> * fix bulk description Signed-off-by: Hunter Achieng <[email protected]> * implement changeset Signed-off-by: Hunter Achieng <[email protected]> * add unit test for bulk --------- Signed-off-by: Hunter Achieng <[email protected]> Co-authored-by: Emmanuel Evance <[email protected]>
* feat:implement dynamic request function in commcare Signed-off-by: Hunter Achieng <[email protected]> * fix pr comments and add changeset Signed-off-by: Hunter Achieng <[email protected]> * feat: implement bulk upload for lookup-tables and case-data (#764) * feat: implement bulk upload for lookup-tables and case-data Signed-off-by: Hunter Achieng <[email protected]> * deleted imports not required Signed-off-by: Hunter Achieng <[email protected]> * fix: remove lookup-table plural Signed-off-by: Hunter Achieng <[email protected]> * reduce example and rename test Signed-off-by: Hunter Achieng <[email protected]> * update example Signed-off-by: Hunter Achieng <[email protected]> * remove try...catch Signed-off-by: Hunter Achieng <[email protected]> * fix bulk description Signed-off-by: Hunter Achieng <[email protected]> * implement changeset Signed-off-by: Hunter Achieng <[email protected]> * add unit test for bulk --------- Signed-off-by: Hunter Achieng <[email protected]> Co-authored-by: Emmanuel Evance <[email protected]> * format test * bump version and update changelog --------- Signed-off-by: Hunter Achieng <[email protected]> Co-authored-by: Emmanuel Evance <[email protected]>
There was a problem hiding this comment.
Hey @hunterachieng Just a couple of little comments for future reference.
Nothing here needs to be actioned, but maybe you could check the file name thing and raise an issue? I would be more comfortable if the file name was consistent across the API call. Even if it doesn't matter at the backend, it means the code looks buggy to anyone reading it
| path = `/a/${domain}/fixtures/fixapi/`; | ||
| file = 'file-to-upload'; | ||
| // append types and lookup-table name xlsx | ||
| Object.keys(json).forEach(sectionName => { |
There was a problem hiding this comment.
for (key in json) { is probably a simply way to to looks like this (you can use async in it as well - not useful here but useful in other places)
|
|
||
| const workbook = xlsx.utils.book_new(); | ||
|
|
||
| if (type.toLowerCase() === 'lookup-table') { |
There was a problem hiding this comment.
type.toLowerCase() is duplicated. I'd probably prefer either a case-insensitive regex (maybe too complicated in this case), or type being saved out into a new variable and lowercased
| }); | ||
| } else if (type.toLowerCase() === 'case-data') { | ||
| path = `/a/${domain}/importer/excel/bulk_upload_api/`; | ||
| file = 'file'; |
There was a problem hiding this comment.
I think file could just be a const file in both cases? The actual file name doesn't seem important. If it is important we probably need an option to take the filename from the user
There was a problem hiding this comment.
Also we actually call the file data.xlsx anyway 🤔
Summary
Implemented a
bulkupload for allowing to upload and updatelookup-tablesandcase-data.Fixes # #751
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
dev only changes don't need a changeset.