Script to Upload SDR Shipments Sheet #93
Conversation
| @@ -7,6 +7,10 @@ | |||
| src/extensions/documentation/documentation/1.0.0/full_documentation.json | |||
| src/extensions/documentation/public/index.html | |||
|
|
|||
| # Explicitly gnore data import csv files so we don't accidentally commit them. | |||
There was a problem hiding this comment.
Nit: typo, should be "ignore"
| Authorization: `Bearer ${STRAPI_ENV.KEY}`, | ||
| }, | ||
| }); | ||
| const body = await response.json(); |
There was a problem hiding this comment.
Might want to wrap this in a try/catch so you can add more information to the error if we can't parse the response body.
| }, | ||
| body: JSON.stringify({ data }), | ||
| }); | ||
| const body = await response.json(); |
There was a problem hiding this comment.
Ditto here for the try/catch
| value: PromiseSettledResult<T>, | ||
| ): value is PromiseRejectedResult => { | ||
| return value.status === "rejected"; | ||
| }; |
There was a problem hiding this comment.
I love, love, love to see type guard functions 🤩
| resultsMap[UploadWorkflowStatus.OTHER].push(workflowResult); | ||
| } | ||
| return resultsMap; | ||
| }, resultsMap); |
There was a problem hiding this comment.
I think my lodash suggestion would apply here as well.
| value: PromiseSettledResult<T>, | ||
| ): value is PromiseRejectedResult => { | ||
| return value.status === "rejected"; | ||
| }; |
There was a problem hiding this comment.
We could probably pull this out into a shared utility?
| Authorization: `Bearer ${STRAPI_ENV.KEY}`, | ||
| }, | ||
| }); | ||
| const body = await response.json(); |
There was a problem hiding this comment.
try/catch would probably be a good idea here
| }, | ||
| body: JSON.stringify({ data }), | ||
| }); | ||
| const body = await response.json(); |
There was a problem hiding this comment.
praise: @corasaurus-hex really great work! Love how Lodash simplifies a lot of those longer / more complex native JS functions. Really appreciate you stepping in to make the refactoring happen in this PR!
Found a few minor things and had a few questions on more meta comments like code readability / naming conventions. If you're at the tech hang I think we can run through them together to make some quick decisions and get this one merged in!
| return new Set( | ||
| shipments.flatMap((s) => | ||
| [s.sendingCountry, s.receivingCountry].filter((v) => v !== ""), | ||
| ), |
There was a problem hiding this comment.
question: Should we set some variable naming conventions? Personally I prefer to avoid shorthand variable names (clarity for other devs, code searchability, etc). For example in this case I'd use shipment or country instead of s and v. But I don't really care as long as we're consistent in how we approach it.
There was a problem hiding this comment.
I usually scale the length of my variable names with the complexity and length of the function, so for simple and smaller functions I use smaller variables because it's trivial to see where the value comes from. Does that seem fair? It's a subtle thing
There was a problem hiding this comment.
@corasaurus-hex that certainly sounds reasonable to me! Do you think this would be a good one to kick back to the wider team on slack to see if anyone has strong preferences? The we can note it as a convention in the readme or something, which will help folks who are new to our codebase (Hacktoberfest, etc).
There was a problem hiding this comment.
I'm reluctant to be very prescriptive on styles, given how subjective style is, and less prescriptive when it comes to folks with less experience, but we could certainly add a style guide as a task leading up to hacktoberfest so that folks can have some guidance.
I wonder how many of these things could be made linter rules, too?
| return value.status === "fulfilled"; | ||
| }; | ||
|
|
||
| export const _isRejected = <T>( |
There was a problem hiding this comment.
nitpick: Should remove the _ in the function name. That was originally included since we weren't using the function but it seemed useful to have around as a pairing with isFulfilled. Now that it's refactored out and exported we don't need to indicate that it's not being used.
|
question (blocking): Something in the last two commits may need investigation. The code is no longer working as expected when I double-checked it before basing my new feature branch from Taylor's. I have all the recent changes pulled down and both countries and shipments appear affected. See the attached image for my results when I run
|
jtfairbank
force-pushed
the
feat/89-90/rebase/upload-sdr-shipments-sheet
branch
from
5c35871 to
6107d38
Compare
|
Gonna merge this one in to keep things moving. Probably still needs work (@corasaurus-hex had good suggestions and @veryeli did good stuff on another script which should be ported back here) but I'll set those up as issues to iterate on. |
What changed?
Added a new script to manage the upload.
Resolves #89 and #90
How can you test this?
Add the attached shipments.csv export to the
src/scripts/import-sdr-shipments-sheetfolder, setup thesrc/scripts/.envfile, then run the dev server in one terminal andyarn script:import-sdr-shipments-sheetin another. The logs at the end should show you that most of the shipments were created (or already exists), and it's ok if there are a few data error cases. If you see a lot of "Other" or "Error" results.