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

[NM-9] Replace file download with native browser download #1034

Merged
merged 5 commits into from
Nov 21, 2024
Merged

[NM-9] Replace file download with native browser download #1034

merged 5 commits into from
Nov 21, 2024

Conversation

krisdoyon
Copy link
Contributor

@krisdoyon krisdoyon commented Nov 20, 2024

Description

Changes download behavior for output files from using store actions to use native browser download. Creates an anchor element with href set as a relative path using the file's download id on the button components which simulates a user click when the download function is fired. Seems to be working as expected in Firefox and Chrome, though currently finding it difficult to manually test a slow file download in Chrome despite trying to throttle the network using the DevTools.

If there is an error preparing the file on the backend, the button will be disabled and the corresponding error will be shown in an error alert below the button. You can test this by updating hintr_version to point to the "download-error" branch, which will always throw an error for the coarse output file

File download tests are now just expecting the button to be enabled once the file is prepared (no longer simulating store actions being called). Not sure if there's a more robust way that we want to test the actual browser download behavior (might work to add something like this as a Playwright test?)

Type of version change

Patch

Checklist

  • I have incremented version number, or version needs no increment
  • The build passed successfully, or failed because of ADR tests

Copy link

Copy link
Collaborator

@r-ash r-ash left a comment

Choose a reason for hiding this comment

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

Thanks for this Kris, I have deployed this on preview and it is looking really good there.

I can see progress working nicely on Chrome. I think this is going to make a huge difference to countries with big download files.

download-progress

With the testing, I think you're right. The tests you've added look good but it misses running the actual download function itself.

You could add a unit test which mocks out the link creation, instead of download itself. Something like

it("downloads file", async() => {
        const downloadSummary = mockDownloadResultsDependency({
            downloadId: "123"
        });

        const props = {
            file: downloadSummary,
            modalOpen: false,
            translateKey: downloadTranslate,
            disabled: false
        }

        const wrapper = getWrapper(props);

        // Mock the element creation
        const mockClick = vi.fn();
        const mockRemove = vi.fn();
        const mockLink = {click: mockClick, remove: mockRemove};
        vi.spyOn(document, "createElement").mockReturnValue(mockLink as any);

        const button = wrapper.find("button");
        await button.trigger("click")
        expect(document.createElement).toHaveBeenCalledTimes(1);
        expect(document.createElement).toHaveBeenCalledWith("a");
        expect(mockClick).toHaveBeenCalledTimes(1);
        expect((mockLink as any).href).toBe("/download/result/123");
        expect((mockLink as any).download).toBe("downloaded_file");
    });
}

would work. This is ok as it runs the download function, but it is quite ugly, and will break if we change the impl at all! So with that I think an integration test would be better. Adding a playwright test for this would be really good.

I think Mantra's changes on #1036 will change the UI though so we should wait until those are in to write that integration test.

const fileUrl = `/download/result/${props.file.downloadId}`
const link = document.createElement("a");
link.href = fileUrl;
link.download = props.file.name || "downloaded_file";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think props.file.name is ever set here? So I think fine to hard code this

Comment on lines -170 to -174
downloadComparisonReport: mapActionByName("downloadResults", "downloadComparisonReport"),
downloadSpectrumOutput: mapActionByName("downloadResults", "downloadSpectrumOutput"),
downloadSummaryReport: mapActionByName("downloadResults", "downloadSummaryReport"),
downloadCoarseOutput: mapActionByName("downloadResults", "downloadCoarseOutput"),
downloadAgywTool: mapActionByName("downloadResults", "downloadAgywTool"),
Copy link
Collaborator

@r-ash r-ash Nov 21, 2024

Choose a reason for hiding this comment

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

Normally you could remove the actions themselves too as they're not being used anymore. But let's also do this after Mantra's changes as otherwise I think it will clash

@r-ash r-ash self-requested a review November 21, 2024 15:15
@r-ash r-ash merged commit 57f24b9 into main Nov 21, 2024
8 of 9 checks passed
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