[EASI-4488] GRB Review supporting docs table #2776
Conversation
mynar7
requested review from
samoddball,
aterstriep and
downeyn-cms
and removed request for
a team
| @@ -27,17 +27,17 @@ func GetSystemIntakeDocumentsByRequestID(ctx context.Context, id uuid.UUID) ([]* | |||
| } | |||
|
|
|||
| // GetURLForSystemIntakeDocument queries S3 for a presigned URL that can be used to fetch the document with the given s3Key | |||
| func GetURLForSystemIntakeDocument(ctx context.Context, store *storage.Store, s3Client *upload.S3Client, s3Key string) (string, error) { | |||
| func GetURLForSystemIntakeDocument(ctx context.Context, store *storage.Store, s3Client *upload.S3Client, s3Key string) (*string, error) { | |||
There was a problem hiding this comment.
Because I needed to allow for null document URLs in case the user is not allowed to view them. I modified the permissions code to not throw errors if the user can't view.
| S3Key: s3Key, | ||
| Bucket: s3Client.GetBucket(), | ||
| UploaderRole: uploaderRole, | ||
| SystemIntakeID: input.RequestID, |
There was a problem hiding this comment.
This was my bad, I didn't realize GQL needed the names to match to auto-resolve.
| @@ -1780,9 +1780,10 @@ type SystemIntakeDocument { | |||
| status: SystemIntakeDocumentStatus! | |||
| version: SystemIntakeDocumentVersion! | |||
| uploadedAt: Time! | |||
| url: String! | |||
| url: String | |||
There was a problem hiding this comment.
Yes, I need to be able to return null for URLs the user isn't allowed to view.
| canView | ||
| canDelete |
| export function downloadFileFromURL(downloadURL: string, fileName: string) { | ||
| return axios | ||
| export function downloadFileFromURL( | ||
| downloadURL: string | null, |
There was a problem hiding this comment.
going back to the ptr questions above - i am wondering if it might be easier to just len check instead of introducing null here?
There was a problem hiding this comment.
Idk, probably a question for @aterstriep
There was a problem hiding this comment.
I think returning null if there isn't a URL makes sense. We could forget to check the string length to account for an empty string, but having it nullable makes sure we get a ts error if we try to use the URL without checking that it exists.
| {/* Business Case Card */} | ||
| <div className="usa-card__container margin-left-0"> | ||
| <CardHeader> | ||
| <h3 className="display-inline-block margin-right-2"> |
There was a problem hiding this comment.
The "last updated" text breaks on mobile, so you could update this class to tablet:display-inline-block to keep that from happening.
…p into EASI-4488/grb-docs-table
| <h3 className="display-inline-block margin-right-2 margin-bottom-0"> | ||
| {t('businessCaseOverview.title')} | ||
| </h3> | ||
| {/* TODO: update these checks to use submittedAt when implemented */} | ||
| {businessCase.id && businessCase.updatedAt && ( | ||
| <span className="text-base tablet:display-inline-block"> |
There was a problem hiding this comment.
The "last updated" text is still breaking on mobile:
The tablet:display-inline-block class needs to be on the h3 tag, not the span.
| <h3 className="display-inline-block margin-right-2 margin-bottom-0"> | |
| {t('businessCaseOverview.title')} | |
| </h3> | |
| {/* TODO: update these checks to use submittedAt when implemented */} | |
| {businessCase.id && businessCase.updatedAt && ( | |
| <span className="text-base tablet:display-inline-block"> | |
| <h3 className="tablet:display-inline-block margin-right-2 margin-bottom-0"> | |
| {t('businessCaseOverview.title')} | |
| </h3> | |
| {/* TODO: update these checks to use submittedAt when implemented */} | |
| {businessCase.id && businessCase.updatedAt && ( | |
| <span className="text-base"> |
There was a problem hiding this comment.
I'm not able to reproduce this locally with display-inline-block set on both the span and h3. I'll test further, though.
| <Alert type="info" className="margin-left-5 margin-bottom-5"> | ||
| {t('businessCaseOverview.unsubmittedAlertText')} | ||
| </Alert> |
There was a problem hiding this comment.
This Alert should have the slim prop, and needs some padding on the right side to keep it from extending to the edge of the Card.
Current:
Figma:
You can remove the util classes and wrap it in a CardBody component to fix the padding:
| <Alert type="info" className="margin-left-5 margin-bottom-5"> | |
| {t('businessCaseOverview.unsubmittedAlertText')} | |
| </Alert> | |
| <CardBody> | |
| <Alert type="info" slim> | |
| {t('businessCaseOverview.unsubmittedAlertText')} | |
| </Alert> | |
| </CardBody> |
| {businessCase.id && | ||
| businessCase.businessNeed && | ||
| businessCase?.preferredSolution?.summary ? ( |
There was a problem hiding this comment.
If you submit a draft business case without the preferred solution fields filled in, it's showing the "there is not yet a Business Case for this request" alert. It should still display the business case as submitted even if those fields are empty. (Go to /governance-review-team/69357721-1e0c-4a37-a90f-64bb29814e7a/grb-review to see a mock data example)
The businessCase?.preferredSolution?.summary condition just needs to be removed here since that field is optional on a draft business case.
The businessCase.businessNeed field is required so you could leave that condition, but it's unnecessary as long as you're checking for the business case ID.
| {businessCase.id && | |
| businessCase.businessNeed && | |
| businessCase?.preferredSolution?.summary ? ( | |
| {businessCase.id ? ( |
There was a problem hiding this comment.
Might be a Zoe question, but should I hide the preferred solution UI if there is none?
There was a problem hiding this comment.
We leave it blank on the Business Case tab but might be worth checking with Zoe.
There was a problem hiding this comment.
Spoke with Zoe, added in e4f760e.
…p into EASI-4488/grb-docs-table
* [EASI-4488] Add new document types to intake form (#2748) * add new document types, adjust form and table, fix minio scripts * fix cypress, remove commented code * use translation to enforce enum options * [EASI-4488] Add permissions to GQL schema for intake documents (#2756) add permissions to GQL schema for intake documents * [EASI-4489] System intake document upload form (#2760) * Add version help text * Update form to match Figma * Add type prop to UploadForm * Admin upload form * Conditional return link * Validation schema * Easi 4580/email notifications (#2774) * UI for selecting if notification should go out * add bool check to BE * fix bool radio * typo * add condition * fix imports * refs * use `field.onChange` instead of `setValue` * trying to fix snapshot * put snap back * added UI tests for UploadForm notification display * add some tests for BE * fix ref, error condition, text * updates to validation * [EASI-4488] GRB Review supporting docs table (#2776) * update queries to include permissions for docs * add documents table * address feedback, update docs table in other places * update snapshot * fix failing cypress test * fix biz case solution rendering and alert padding * rename migration number * [EASI-4591] IT Gov admin route updates (#2781) * governance-review-team -> it-governance * Update subNavItems + context * Route updates * Remove hidden nav links from array * Unit test fixes * Unit tests for action buttons * Redirect for old admin links * Update url in e2e tests * Upload form route * Test file routes * [EASI-4597] Add software BOM to doc types (#2787) add software BOM to doc types * [EASI-4583] GRB review sub navigation links (#2792) * SideNavigation component * RequestOverview side nav - Updated to use SideNavigation component - Added Supporting Documents and Participants links * Child nav link functionality * AccordionNavigation updates * Remove unused CSS * Snapshot Accordion navigation button no longer renders on desktop * I definitely didn't forget this before * e2e test fix * e2e test fix - actions link selector * e2e test fix - lcid link selector * e2e test fix - dates link selector * NOREF - change email link URLs to reference new path (#2795) change email link URLs to reference new path * Update pre-signed S3 url timeout to 90 minutes --------- Co-authored-by: Ashley Terstriep <60187543+aterstriep@users.noreply.github.com> Co-authored-by: samoddball <156127704+samoddball@users.noreply.github.com> Co-authored-by: ClayBenson94 <clay.benson@oddball.io>
EASI-4488
Description
How to test this change
scripts/dev db:clean && scripts/dev db:seedscripts/dev minio:cleanA11Ywho is assigned as a reviewer to verify you cannot delete documents. You can also add documents as an admin that is not the requester with this job code for additional testing.PR Author Checklist
PR Reviewer Guidelines