Feat/2181 download attachments #2206
Conversation
tmastrom
force-pushed
the
feat/2181-download-attachments
branch
from
0200c81
to
2623114
Compare
There was a problem hiding this comment.
This looks great, review comments are all nits. I think we could additionally use a test for VerificationStatement to check that clicking the filename calls the router or has the right href, whatever makes more sense. There's the beginnings of an example of simulating a click in old jest here: https://github.com/bcgov/cas-ciip-portal/pull/2205/files#diff-bbd02c55400eb98b04e21e90329fba88eb8c6c035b0753fcb48e0dc503534c6cR107-R124
| const attachments = applicationRevision.attachmentsByApplicationIdAndVersionNumber | ||
| ? applicationRevision.attachmentsByApplicationIdAndVersionNumber.edges | ||
| : undefined; |
There was a problem hiding this comment.
Could use optional chaining to shorten this, something like const attachments = applicationRevision?.attachmentsByApplicationIdAndVersionNumber?.edges
And then instead of checking if attachments is false for conditional rendering, check if its length is 0
| <Collapse in={!isOpen}> | ||
| <Card.Body className="card-body"> | ||
| {attachments ? ( | ||
| attachments.map((opt) => ( |
There was a problem hiding this comment.
could use {node} as the map argument and then you don't have to repeat opt. everywhere
| handleDownload, | ||
| } from "server/middleware/attachmentDownloadRouter"; | ||
|
|
||
| const performQuery = require("../../../../server/postgraphile/graphql"); |
There was a problem hiding this comment.
I ~* think you might be able to shorten the require path to just server/postgraphile/graphql?
| const performQuery = require("../../../../server/postgraphile/graphql"); | ||
| const Storage = require("@google-cloud/storage"); | ||
|
|
||
| jest.mock("../../../../server/postgraphile/graphql"); |
| ); | ||
| }); | ||
|
|
||
| it("pipes the storage client's file", async () => { |
There was a problem hiding this comment.
This looks great! Thanks so much for figuring out the old jest test setup
| @@ -156,7 +170,72 @@ export const ApplicationDetailsComponent: React.FunctionComponent<Props> = ({ | |||
| /> | |||
| ))} | |||
| </div> | |||
| <Card | |||
There was a problem hiding this comment.
This looks super nice in the app
| @@ -21,32 +25,53 @@ describe("The Attachment Upload Component", () => { | |||
| createdAt: "date", | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
there should only be 1 attachment
There was a problem hiding this comment.
This makes sense for this PR, but FYI ultimately people do need to be able to upload more than one attachment. I'm fixing and testing that part in my PR, so this is good to go
There was a problem hiding this comment.
Thanks for letting me know, I've updated the testing to reflect this
| @@ -21,32 +25,53 @@ describe("The Attachment Upload Component", () => { | |||
| createdAt: "date", | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
This makes sense for this PR, but FYI ultimately people do need to be able to upload more than one attachment. I'm fixing and testing that part in my PR, so this is good to go
Card: #2181