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

Ensure files are only downloaded once #4404

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Ensure files are only downloaded once #4404

merged 1 commit into from
Jan 17, 2024

Conversation

melton-jason
Copy link
Contributor

@melton-jason melton-jason commented Jan 14, 2024

Fixes #2638

The issue here is that for certain browsers the load event was being triggered twice where the iframe.contentWindow was not null

iframe.addEventListener('load', () => {
if (iframe.contentWindow === null) return;
const element = iframe.contentWindow.document.createElement('a');
element.setAttribute(
'href',
`data:text/plain;charset=utf-8,${encodeURIComponent(text)}`
);
element.setAttribute('download', fileName);
element.style.display = 'none';
iframe.contentWindow.document.body.append(element);
element.click();
globalThis.setTimeout(() => {
iframe.remove();
resolve();
}, 100);
});

The load event was being fired and caught both when the iframe was appended to the document and when the content within the iframe was being closed. In both cases the iframe.contentWindow was not null, so the initial return was never caught and the element.click() was called twice.

document.body.append(iframe);
iframe.contentWindow?.document.open();
iframe.contentWindow?.document.write(html);
iframe.contentWindow?.document.close();

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

In each of Google Chrome, Firefox, and Safari, try actions which would download a file directly, not using the Notification system.
(#2638 did not affect files downloaded from the notification system)

Here are a few places in Specify where this can be accomplished:
(This likely do not all need to be tested, as they all utilize the same downloadFile function for functionality)

  • Exporting Query Results (when at least one result is selected)
  • Downloading Statistics as TSV
  • Downloading an AppResource
  • Downloading a Viewset
  • Downloading the content of the Crash Report Visualizer as HTML
    • Accessible by navigating to /specify/developer/crash-report-visualizer
  • Downloading System/Specify information from the About Specify page
    • Accessible via User Tools
  • Downloading the Specify datamodel/schema from the Database Schema page
    • Accessible via User Tools

If other browsers are tested, please mention them in your comment.

@melton-jason melton-jason requested review from a team January 14, 2024 18:09
Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

Google Chrome (Version 120.0.6099.216 (Official Build) (arm64))

  • Exporting Query Results (when at least one result is selected)
  • Downloading Statistics as TSV
  • Downloading an AppResource
  • Downloading a Viewset
  • Downloading the content of the Crash Report Visualizer as HTML
  • Accessible by navigating to /specify/developer/crash-report-visualizer
  • Downloading System/Specify information from the About Specify page
  • Downloading the Specify datamodel/schema from the Database Schema page

Firefox (Version 120.0.1 (64-bit))

  • Exporting Query Results (when at least one result is selected)
  • Downloading Statistics as TSV
  • Downloading an AppResource
  • Downloading a Viewset
  • Downloading the content of the Crash Report Visualizer as HTML
  • Accessible by navigating to /specify/developer/crash-report-visualizer
  • Downloading System/Specify information from the About Specify page
  • Downloading the Specify datamodel/schema from the Database Schema page

@grantfitzsimmons grantfitzsimmons requested a review from a team January 14, 2024 19:24
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

ah, now that makes sense
another mystery solved - thanks!

@maxpatiiuk
Copy link
Member

maxpatiiuk commented Jan 17, 2024

Also, see my ominous warning 😈:

* A hacky way to download a file on the front-end
* May stop working in the future

I just couldn't find any other way that didn't involve back-end, didn't force HTTPs, gave us control over the file name, didn't require third-party libraries, and worked accorss browsers (somewhat)

@grantfitzsimmons
Copy link
Member

Which release will this be included with? @CarolineDenis @melton-jason

This seems like another PR that can be included in 7.9.3, though I don't want to be expand the scope if you feel it would be better to add in 7.9.4.

@melton-jason melton-jason merged commit 1886a44 into production Jan 17, 2024
9 checks passed
@melton-jason melton-jason deleted the issue-2638 branch January 17, 2024 15:23
@grantfitzsimmons grantfitzsimmons added this to the 7.9.3 milestone Jan 17, 2024
@specifysoftware
Copy link

This pull request has been mentioned on Specify Community Forum. There might be relevant details there:

https://discourse.specifysoftware.org/t/specify-7-9-3-release-announcement/1499/1

maxpatiiuk added a commit to maxpatiiuk/text-hoarder that referenced this pull request Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Downloaded files being duplicated
5 participants