From 837934465f7ecd74cd808e0ce85ab72f763ce86c Mon Sep 17 00:00:00 2001 From: plocket <52798256+plocket@users.noreply.github.com> Date: Thu, 21 Nov 2024 10:13:02 -0500 Subject: [PATCH] Fix #725, partial filename match for downloading fails The failure is actually during the comparison - the file was being saved using the partial name the author gave in the Step when it should have been using the file's downloaded name. Note that our current download method will, in ideal cases, let an author re-name the downloaded file. Since it might not be consistent, we might still avoid implementing this at this point. Also, in ideal cases, the Step does already continue without waiting for the whole file to download, which addresses #492. That happened a while ago. It's worth considering closing that issue at this point. We have to research under what circumstances a file would fail to download with a `fetch` while still being able to download with a click. Maybe with an encrypted interview. Co-authored-by: ethanstrominger --- .github/workflows/github_server.yml | 6 +- .github/workflows/playground.yml | 6 +- CHANGELOG.md | 9 ++- .../data/sources/observation_steps.feature | 13 ++-- lib/scope.js | 61 +++++++++++++++---- lib/steps.js | 3 +- package.json | 2 +- 7 files changed, 74 insertions(+), 26 deletions(-) diff --git a/.github/workflows/github_server.yml b/.github/workflows/github_server.yml index d4cf9764..fb0ddd31 100644 --- a/.github/workflows/github_server.yml +++ b/.github/workflows/github_server.yml @@ -52,8 +52,8 @@ jobs: EMPTY_STRING: "" NORMAL_USER_EMAIL: alkiln@example.com NORMAL_USER_PASSWORD: User123^ - WRONG_EMAIL=wrong_email@example.com - WRONG_PASSWORD=wrong_password + WRONG_EMAIL: wrong_email@example.com + WRONG_PASSWORD: wrong_password steps: # Place the root directory in this branch to access @@ -160,7 +160,7 @@ jobs: #### Developer note: You can probably leave the rest out #### To learn more, see https://assemblyline.suffolklitlab.org/docs/alkiln/writing/#optional-inputs ALKILN_TAG_EXPRESSION: "${{ env.ALKILN_TAG_EXPRESSION }}" - ALKILN_VERSION: delete + ALKILN_VERSION: download #### Developer note: Example of making an issue when tests fail #### that includes the text of the failure output file diff --git a/.github/workflows/playground.yml b/.github/workflows/playground.yml index caa4f881..cca4ff6d 100644 --- a/.github/workflows/playground.yml +++ b/.github/workflows/playground.yml @@ -47,8 +47,8 @@ jobs: NORMAL_USER_PASSWORD: ${{ secrets.USER_NO_PERMISSIONS_PASSWORD }} NORMAL_USER_API_KEY: ${{ secrets.USER_NO_PERMISSIONS_API_KEY }} INVALID_API_KEY: invalidAPIkey - WRONG_EMAIL=wrong_email@example.com - WRONG_PASSWORD=wrong_password + WRONG_EMAIL: wrong_email@example.com + WRONG_PASSWORD: wrong_password EMPTY_STRING: "" SECRET_VAR1: secret-var1-value SECRET_VAR2: secret-var2-value @@ -81,4 +81,4 @@ jobs: # want to check up on this. ALKILN_TAG_EXPRESSION: "${{ env.ALKILN_TAG_EXPRESSION }}" #### Developer note: You can probably leave this out - ALKILN_VERSION: delete + ALKILN_VERSION: download diff --git a/CHANGELOG.md b/CHANGELOG.md index 530c8679..32685b88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,13 @@ Format: ## [Unreleased] + + +### Fixed +- Fixes download Step unable to use a partial filename match. The Step needed the whole name, including the extension. See [#725](https://github.com/SuffolkLITLab/ALKiln/issues/725). + +## [5.13.1] - 2024-10-23 + ### Changed - Moves "expected" status error to only be visible to internal test errors. Closes [#933](https://github.com/SuffolkLITLab/ALKiln/issues/933). @@ -52,7 +59,6 @@ Format: ### Fixed - Detects failed sign in. Closes [#918](https://github.com/SuffolkLITLab/ALKiln/issues/918). -- Updates pdfjs-dist for node v20 and v22. See [#952](https://github.com/SuffolkLITLab/ALKiln/issues/952). ### Internal @@ -63,6 +69,7 @@ Format: - Adds decision docs - Updated CONTRIBUTING.md - Added example.env, closes [#374](https://github.com/SuffolkLITLab/ALKiln/issues/374) +- Updates pdfjs-dist for node v20 and v22. See [#952](https://github.com/SuffolkLITLab/ALKiln/issues/952). ## [5.13.0] - 2024-07-11 diff --git a/docassemble/ALKilnTests/data/sources/observation_steps.feature b/docassemble/ALKilnTests/data/sources/observation_steps.feature index 6afeba9f..69875ffa 100644 --- a/docassemble/ALKilnTests/data/sources/observation_steps.feature +++ b/docassemble/ALKilnTests/data/sources/observation_steps.feature @@ -239,19 +239,19 @@ Scenario: I compare different PDFs Given the final Scenario status should be "failed" And the Scenario report should include: """ - Could not find the existing PDF at DOES_NOT_EXIST.pdf + ALK0156 """ And the Scenario report should include: """ - The PDFs were not the same. + ALK0157 """ - And the Scenario report should include: + And the Scenario report should include: """ - The new PDF added: + - diff """ And the Scenario report should include: """ - - diff + ALK0093 """ Given I start the interview at "test_pdf" Then the question id should be "proxy vars" @@ -268,6 +268,7 @@ Scenario: I compare different PDFs And I tap to continue # Next page Then the question id should be "2_signature download" - When I download "2_signature.pdf" + # Match a partial name + When I download "2_signatu" And I expect the baseline PDF "DOES_NOT_EXIST.pdf" and the new PDF "2_signature.pdf" to be the same And I expect the baseline PDF "linear_2_signature-Baseline.pdf" and the new PDF "2_signature.pdf" to be the same diff --git a/lib/scope.js b/lib/scope.js index 65cf65b2..65279c64 100644 --- a/lib/scope.js +++ b/lib/scope.js @@ -2781,7 +2781,7 @@ module.exports = { await scope.afterStep(scope); }, // Ends scope.steps.sign() - download: async ( scope, filename ) => { + download: async ( scope, full_or_partial_file_href ) => { /* Taps the link that leads to the given filename to trigger downloading. * and waits till the file has been downloaded before allowing the tests to continue. * WARNING: Cannot download the same file twice in a single scenario. @@ -2791,47 +2791,86 @@ module.exports = { * TODO: Properly wait for download to complete. See notes in * scope.js scope.detectDownloadComplete() */ - let [elem] = await scope.page.$$(`xpath/.//a[contains(@href, "${ filename }")]`); + let [elem] = await scope.page.$$(`xpath/.//a[contains(@href, "${ full_or_partial_file_href }")]`); - let msg = `"${ filename }" seems to be missing. Cannot find a link to that document.`; + let msg = `"${ full_or_partial_file_href }" seems to be missing on the page. Cannot find a link to that document.`; if ( !elem ) { reports.addToReport(scope, { type: `error`, code: `ALK0152`, value: msg }); } expect( elem, msg ).to.exist; let failed_to_download = false; let err_msg = ""; try { - const binaryStr = await scope.page.evaluate(el => { + + const { disposition, binaryStr } = await scope.page.evaluate(async function ( el ) { const url = el.getAttribute("href"); return new Promise(async (resolve, reject) => { const response = await fetch(url, {method: "GET"}); const reader = new FileReader(); reader.readAsBinaryString(await response.blob()); - reader.onload = () => resolve(reader.result); + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition + reader.onload = () => resolve({ + disposition: response.headers.get(`Content-Disposition`), + binaryStr: reader.result + }); reader.onerror = () => reject(`šŸ¤• ALK0153 ERROR: Error occurred on page when downloading ${ url }: ${ reader.error }`); }); }, elem); + + err_msg = `could not get the actual name of the file from the response headers.`; + let actual_filename = await scope.steps.get_response_filename({ + disposition, default_name: full_or_partial_file_href + }); + if (binaryStr !== '') { + err_msg = `binary data for download was empty`; const fileData = Buffer.from(binaryStr, 'binary'); - fs.writeFileSync(`${ scope.paths.scenario }/${ filename }`, fileData); - reports.addToReport(scope, { type: `row info`, code: `ALK0154`, value: `Downloaded ${ filename } (${ fileData.length } bytes) to ${ scope.paths.scenario }`}); + fs.writeFileSync(`${ scope.paths.scenario }/${ actual_filename }`, fileData); + reports.addToReport(scope, { type: `row info`, code: `ALK0154`, value: `Downloaded "${ actual_filename }" (${ fileData.length } bytes) to ${ scope.paths.scenario }`}); } else { failed_to_download = true; - err_msg = `Couldn't download ${ filename }, binary data for download was empty`; } + } catch (error) { failed_to_download = true; err_msg = error; } if (failed_to_download) { - reports.addToReport(scope, { type: `warning`, code: `ALK0155`, value: `Could not download file using fetch (${ err_msg }). ALKiln will now fallback to the click download method.` }); - scope.toDownload = filename; + reports.addToReport(scope, { type: `warning`, code: `ALK0155`, value: `Could not download a file matching the name "${ full_or_partial_file_href }" using fetch (${ err_msg }). ALKiln will now fallback to the click download method.` }); + scope.toDownload = full_or_partial_file_href; // Should this be using `scope.tapElement`? await elem.evaluate( elem => { return elem.click(); }); await scope.detectDownloadComplete( scope ); } }, // Ends scope.steps.download() + get_response_filename: async function({ disposition, default_name=`found_no_file_name.pdf` }) { + /** Given a fetch response headers' Content-Disposition str, return the + * filename of the response's file. + * + * @return { string | null } - Name of fetched file + * */ + let filename = default_name; + if ( disposition ) { + filename = disposition.split(`;`)[1].split(`=`)[1]; + } + + // Handle potential UTF-8 encoded filenames + if ( filename.toLowerCase().startsWith( `utf-8''` )) { + filename = decodeURIComponent( filename.replace( /utf-8''/i, `` )); + } else { + // Replace starting and ending quotes if they exist + filename = filename.replace( /^['"]/, `` ).replace( /['"]$/, `` ); + } + + // TODO: Add debug log here + // console.log(`filename:`, filename); + + // TODO: Add to the report if we had to use the default name + + return filename; + }, // Ends scope.steps.get_response_filename() + compare_pdfs: async function (scope, {existing_pdf_path, new_pdf_path}) { let existing_paths = await scope.findFiles(scope, {to_find_names: [existing_pdf_path]}); if (existing_paths.length == 0) { @@ -2852,7 +2891,7 @@ module.exports = { let removed_str = diffs.filter(part => part.removed).reduce((err_str, part) => { return err_str + `- ${ part.value }\n` }, 'The new PDF removed: \n'); - let msg = `The PDFs were not the same.\n${ added_str }\n${removed_str}\n\n You can see the full PDFs at ${ full_existing_path } and ${ full_new_pdf_path}`; + let msg = `The PDFs were different.\nAdded:\n${ added_str }\nRemoved:\n${removed_str}\n\nThere might be more information if you actually look at the files. You can see the full PDFs at ${ full_existing_path } and ${ full_new_pdf_path}`; reports.addToReport(scope, { type: `error`, code: `ALK0157`, value: msg }); scope.failed_pdf_compares.push(msg); } diff --git a/lib/steps.js b/lib/steps.js index 54e2ee96..cd7e83a2 100644 --- a/lib/steps.js +++ b/lib/steps.js @@ -1285,10 +1285,11 @@ After(async function(scenario) { if ( scope.failed_pdf_compares.length > 0) { let msg = scope.failed_pdf_compares.reduce((str, new_msg) => `${ str }\nā€•ā€•ā€•\n${ new_msg }`) changeable_test_status = `FAILED`; + // TODO: This may be redundant and therefore confusing reports.addToReport(scope, { type: `error`, code: `ALK0093`, - value: `PDF comparison failed ${ scope.failed_pdf_compares.length } time(s)\nā€•ā€•ā€•\n${ msg }\nā€•ā€•ā€•\n` + value: `ALKiln ran into an error when it tried to compare ${ scope.failed_pdf_compares.length } PDF(s)\nā€•ā€•ā€•\n${ msg }\nā€•ā€•ā€•\n` }); } diff --git a/package.json b/package.json index 0c34bbe4..a35c539c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@suffolklitlab/alkiln", - "version": "5.13.1", + "version": "5.13.1-fix-download-name", "description": "Integrated automated end-to-end testing with docassemble, puppeteer, and cucumber.", "main": "lib/index.js", "scripts": {