Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MR-3573: Download all contracts button does not generate zip file on first click #1859

Conversation

JasonLin0991
Copy link
Contributor

@JasonLin0991 JasonLin0991 commented Aug 7, 2023

Summary

MR-3573
Figma

This bug occurs when the Download all document button link is an empty string. There were two ways to reproduce this bug.

  • Clicking on Download all before generating the bulk download URL was finished, and the link
  • Generating bulk download URL encountered an error.
    Both ways caused this bug because the code defaults the URL to an empty string. This empty link will then open a new tab with the submission summary page.

The fix was to add error handling and loading UI around generating the bulk URL.

  • When first loading the submission summary, Contract details and Rate details section will first show a Loading button. Once the bulk download URL has been generated, it will show the Download all button.
  • If there is an error in getBulkDlURL, then an inline warning will appear in place of the Download all button, and a warning banner will appear at the top of the page.
    • There will only be one document warning banner if both Contract and Rate bulk download encountered an error.
  • Fixed a few other issues:
    • Flickering on ActionButton component when displaying loading first.
    • ActionButton loading state size is larger then default size.
    • Preventing fetchZipUrl from executing on DRAFT and previous submission view.

Related issues

Screenshots

Test cases covered

  • DownloadButton.test.tsx

    • 'renders loading button'
  • ContractDetailsSummarySection.test.tsx, RateDetailsSummarySection.test.tsx

    • 'renders inline error when bulk URL is unavailable'
  • SubmissionSummary.test.tsx

    • 'renders document download warning banner'

QA guidance

  • You can test these error UI on the submissions created by Cypress. The submission with Q&A will have an invalid s3 url for a contract document which leads to errors when running getBulkDlURL.
  • Please test to make sure there is no regression in the fix for spam clicking the Continue or Save as Draft buttons.

@macrael
Copy link
Contributor

macrael commented Aug 15, 2023

Is it intentional that this submission on the review app generates an error? Error banner works: https://ddt0awkezsb2o.cloudfront.net/submissions/7055c843-32c2-48b0-b806-103488e3c291

Copy link
Contributor

@macrael macrael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solid

}

export const DownloadButton = ({
text,
zippedFilesURL,
}: DownloadButtonProps): React.ReactElement => {
const handleClick = () => {
if (zippedFilesURL) {
window.open(zippedFilesURL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we opening the link with JS? Shouldn't it be a simple Link in the end? Id worry about screen readers here going outside the normal semantics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Good call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@macrael I updated this to use the Link component again. From this blog, disabling the link might not be the way to go, so I'm rendering just a div with the loading spinner until the link is ready. Could use you're eyes on this again.

}: ContractDetailsSummarySectionProps): React.ReactElement => {
// Checks if submission is a previous submission
const isPreviousSubmission = usePreviousSubmission()
// Get the zip file for the contract
const { getKey, getBulkDlURL } = useS3()
const [zippedFilesURL, setZippedFilesURL] = useState<string>('')
const [zippedFilesURL, setZippedFilesURL] = useState<
string | undefined | Error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☺️

@@ -83,8 +93,14 @@ export const ContractDetailsSummarySection = ({
'HEALTH_PLAN_DOCS'
)
if (zippedURL instanceof Error) {
console.info('ERROR: TODO: DISPLAY AN ERROR MESSAGE')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

])

const renderDownloadButton = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull this out into its own function, please. Anonymous functions that render JSX are how we got a lot of our Cypress flakes b/c the component is unmounted and remounted on every render.

@@ -174,7 +179,10 @@ export const RateDetailsSummarySection = ({
}
}

useEffect(() => {
useDeepCompareEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this switch for the flickering? Makes sense that this would be useful but referential equality seems to usually be good enough for us

Copy link
Contributor Author

@JasonLin0991 JasonLin0991 Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was switched because of infinite re-rendering. I think it is because of isPreviousSubmission dependency, which is a hook and the referential equality was not enough.

])
const renderDownloadButton = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ibid


const documentsSummary = `${refreshedDocs.length} ${
refreshedDocs.length === 1 ? 'file' : 'files'
}`
// when there are no uncategorized supporting documents, remove this section entirely
if (refreshedDocs.length === 0) return null

const renderDownloadButton = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ibid

@JasonLin0991
Copy link
Contributor Author

Is it intentional that this submission on the review app generates an error? Error banner works: https://ddt0awkezsb2o.cloudfront.net/submissions/7055c843-32c2-48b0-b806-103488e3c291

For the Q&A tests, we use direct API requests to make quick submissions, which does not upload files, just inserts a string in for the URL.

Copy link
Contributor

@macrael macrael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me semantically!

@JasonLin0991 JasonLin0991 merged commit 4e85504 into main Aug 16, 2023
27 checks passed
@JasonLin0991 JasonLin0991 deleted the jl-3573-Download-all-contracts-button-does-not-generate-zip-file-on-first-click branch August 16, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants