PIMS-1803: Bulk upload API #2478
Conversation
…s for importinb parcels, one using manual row iteration and another using the _to_json util
…dling for converting PIDs and PINs
… processing then immediately return a status 200. Also, there is handling for deleting the uploaded file once the task finishes now.
…ndividual row failures do not make the entire upload fail. Results are collected into an array with action and reason for the action.
…ction types, and admin areas. Also modified the property save structure to not overwrite the existing CreatedBy data on update.
…progress/completion of the task
…netary units are not removed by accident
GrahamS-Quartech
requested review from
LawrenceLau2020,
Sharala-Perumal,
dbarkowsky,
TaylorFries and
ManishSihag
as code owners
GrahamS-Quartech
changed the title
There was a problem hiding this comment.
I like where this is going. It seems like it's going to be really efficient compared to the old way of doing it.
Would like to have more information on the results though. I don't see how we could effectively identify which row caused the error. A row number would be good.
express-api/package.json
Outdated
| @@ -22,6 +22,7 @@ | |||
| "dependencies": { | |||
| "@bcgov/citz-imb-kc-css-api": "https://github.com/bcgov/citz-imb-kc-css-api/releases/download/v1.4.0/bcgov-citz-imb-kc-css-api-1.4.0.tgz", | |||
| "@bcgov/citz-imb-sso-express": "1.0.0-rc2", | |||
| "@types/multer": "^1.4.11", | |||
There was a problem hiding this comment.
| "@types/multer": "^1.4.11", | |
| "@types/multer": "1.4.11", |
Should also be in dev dependencies.
| existentParcel, | ||
| ); | ||
| queuedParcels.push(parcelToUpsert); | ||
| results.push({ action: existentParcel ? 'updated' : 'inserted' }); |
There was a problem hiding this comment.
If we push this success action here, could it still fail when the queuedParcels/Buildings goes to insert?
I'm thinking this might return false positives.
There was a problem hiding this comment.
Well in theory, anything that would cause the parcelToUpsert object to be invalid would have already thrown an error inside the makeParcelUpsertObject function. But that's a good point, the insert could still fail for some unexpected reason when it actually makes the save call. I suppose this may be another reason the old API does everything one at a time, but in the ideal case where all rows from the spreadsheet contain new PIDs it's much faster to do batched insert. I'll have to think about this a bit.
There was a problem hiding this comment.
I definitely like the batch better. Just wondering if there's a way to see which items failed in that batch insert, then update the results maybe.
| @@ -27,6 +29,10 @@ router.route('/search/geo').get(activeUserCheck, catchErrors(getPropertiesForMap | |||
| router.route('/search/page').get(activeUserCheck, catchErrors(getPropertiesPaged)); | |||
| router.route('/search/page/filter').post(activeUserCheck, catchErrors(getPropertiesPagedFilter)); | |||
|
|
|||
| const upload = multer({ dest: 'uploads/' }); | |||
There was a problem hiding this comment.
Does this mean it will keep a local copy of the file in the container? Or it's just temporary?
There was a problem hiding this comment.
It does temporarily create a copy of the file in the container but propertiesController.ts:210 will attempt to destroy it before exiting after processing the file.
…ent feedback to the user.
🎯 Summary
PIMS-1803
Please check general business logic in properties services and see if it makes sense with your own understanding and the previous implementation in the .NET API.
Note that a rollback transaction is wrapping the upload process right now. I will remove this before merging this branch.
🔰 Checklist