Fix #725, partial filename match for downloading fails #959
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BryceStevenWilley
approved these changes
Nov 22, 2024
lib/scope.js
Outdated
| @@ -2781,7 +2781,7 @@ module.exports = { | |||
| await scope.afterStep(scope); | |||
| }, // Ends scope.steps.sign() | |||
|
|
|||
| download: async ( scope, filename ) => { | |||
| download: async ( scope, filename_matcher ) => { | |||
There was a problem hiding this comment.
filename_matcher to me implies that it's some more powerful lambda function or regex or something, and not just a partial string. Maybe partial_filename would be better.
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 <ethanstrominger2@gmail.com>
plocket
force-pushed
the
725_download_match
branch
from
063817a to
8379344
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Close #725
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
fetchwhile still being able to download with a click. Maybe with an encrypted interview.In this PR, I have:
Reason for this PR
When using the "download" Step, authors use a filename to get the file link from the page. For example, "my_file.pdf". They are also supposed to be able to use a partial file name to match the file name - "my_fi" should work just as well. That seemed to cause problems, though.
Exploring the problem showed that the real problem was when comparing the downloaded PDF to a baseline PDF. An author would download "my_fi" and then compare "my_file.pdf" with "basline_file.pdf". The comparison would fail because "my_file.pdf" was missing.
The problem was that ALKiln was saving the downloaded file using the partial-match name, not the file's actual name. That is, ALKiln would save a file called "my_fi". Then, when ALKiln tried to find "my_file.pdf" for the comparison, it would only find "my_fi" and the comparison would fail.
This PR now uses the name that the download file gives in the
response.headers.Note: We might need to improve the message when ALKiln can't find the downloaded file. It wasn't clear about what happened. I might add that to the PR
Also see #725
Links to any solved or related issues
Close #725
Any manual testing I have done to ensure my PR is working
N/A