From f3ac6692da636b240d320699211491e55b25f739 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 31 Jul 2024 15:07:59 +0100 Subject: [PATCH] Handle media download errors better (#12848) * Handle media download errors better Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Add test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * Show error if media download failed Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> * More tests Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../views/messages/DownloadActionButton.tsx | 27 ++++++-- src/customisations/Media.ts | 10 ++- src/i18n/strings/en_EN.json | 2 + .../messages/DownloadActionButton-test.tsx | 68 +++++++++++++++++++ test/customisations/Media-test.ts | 39 +++++++++++ test/test-utils/test-utils.ts | 2 +- 6 files changed, 140 insertions(+), 8 deletions(-) create mode 100644 test/components/views/messages/DownloadActionButton-test.tsx create mode 100644 test/customisations/Media-test.ts diff --git a/src/components/views/messages/DownloadActionButton.tsx b/src/components/views/messages/DownloadActionButton.tsx index 457a79b8db..5bb6b17881 100644 --- a/src/components/views/messages/DownloadActionButton.tsx +++ b/src/components/views/messages/DownloadActionButton.tsx @@ -24,6 +24,8 @@ import { RovingAccessibleButton } from "../../../accessibility/RovingTabIndex"; import Spinner from "../elements/Spinner"; import { _t, _td, TranslationKey } from "../../../languageHandler"; import { FileDownloader } from "../../../utils/FileDownloader"; +import Modal from "../../../Modal"; +import ErrorDialog from "../dialogs/ErrorDialog"; interface IProps { mxEvent: MatrixEvent; @@ -53,6 +55,23 @@ export default class DownloadActionButton extends React.PureComponent => { + try { + await this.doDownload(); + } catch (e) { + Modal.createDialog(ErrorDialog, { + title: _t("timeline|download_failed"), + description: ( + <> +
{_t("timeline|download_failed_description")}
+
{e instanceof Error ? e.toString() : ""}
+ + ), + }); + this.setState({ loading: false }); + } + }; + + private async doDownload(): Promise { const mediaEventHelper = this.props.mediaEventHelperGet(); if (this.state.loading || !mediaEventHelper) return; @@ -64,15 +83,15 @@ export default class DownloadActionButton extends React.PureComponent { + private async downloadBlob(blob: Blob): Promise { await this.downloader.download({ blob, name: this.props.mediaEventHelperGet()!.fileName, diff --git a/src/customisations/Media.ts b/src/customisations/Media.ts index 25e8489658..dc3846d905 100644 --- a/src/customisations/Media.ts +++ b/src/customisations/Media.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { MatrixClient, ResizeMethod } from "matrix-js-sdk/src/matrix"; +import { MatrixClient, parseErrorResponse, ResizeMethod } from "matrix-js-sdk/src/matrix"; import { MediaEventContent } from "matrix-js-sdk/src/types"; import { Optional } from "matrix-events-sdk"; @@ -144,12 +144,16 @@ export class Media { * Downloads the source media. * @returns {Promise} Resolves to the server's response for chaining. */ - public downloadSource(): Promise { + public async downloadSource(): Promise { const src = this.srcHttp; if (!src) { throw new UserFriendlyError("error|download_media"); } - return fetch(src); + const res = await fetch(src); + if (!res.ok) { + throw parseErrorResponse(res, await res.text()); + } + return res; } } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index a17b04a9f6..0d6319d80a 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -3264,6 +3264,8 @@ "disambiguated_profile": "%(displayName)s (%(matrixId)s)", "download_action_decrypting": "Decrypting", "download_action_downloading": "Downloading", + "download_failed": "Download failed", + "download_failed_description": "An error occurred while downloading this file", "edits": { "tooltip_label": "Edited at %(date)s. Click to view edits.", "tooltip_sub": "Click to view edits", diff --git a/test/components/views/messages/DownloadActionButton-test.tsx b/test/components/views/messages/DownloadActionButton-test.tsx new file mode 100644 index 0000000000..a57edeb81e --- /dev/null +++ b/test/components/views/messages/DownloadActionButton-test.tsx @@ -0,0 +1,68 @@ +/* +Copyright 2024 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import React from "react"; +import { mocked } from "jest-mock"; +import fetchMockJest from "fetch-mock-jest"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { MatrixEvent } from "matrix-js-sdk/src/matrix"; + +import { stubClient } from "../../../test-utils"; +import DownloadActionButton from "../../../../src/components/views/messages/DownloadActionButton"; +import Modal from "../../../../src/Modal"; +import { MediaEventHelper } from "../../../../src/utils/MediaEventHelper"; +import ErrorDialog from "../../../../src/components/views/dialogs/ErrorDialog"; + +describe("DownloadActionButton", () => { + it("should show error if media API returns one", async () => { + const cli = stubClient(); + // eslint-disable-next-line no-restricted-properties + mocked(cli.mxcUrlToHttp).mockImplementation( + (mxc) => `https://matrix.org/_matrix/media/r0/download/${mxc.slice(6)}`, + ); + + fetchMockJest.get("https://matrix.org/_matrix/media/r0/download/matrix.org/1234", { + status: 404, + body: { errcode: "M_NOT_FOUND", error: "Not found" }, + }); + + const event = new MatrixEvent({ + room_id: "!room:id", + sender: "@user:id", + type: "m.room.message", + content: { + body: "test", + msgtype: "m.image", + url: "mxc://matrix.org/1234", + }, + }); + const mediaEventHelper = new MediaEventHelper(event); + + render( mediaEventHelper} />); + + const spy = jest.spyOn(Modal, "createDialog"); + + fireEvent.click(screen.getByRole("button")); + await waitFor(() => + expect(spy).toHaveBeenCalledWith( + ErrorDialog, + expect.objectContaining({ + title: "Download failed", + }), + ), + ); + }); +}); diff --git a/test/customisations/Media-test.ts b/test/customisations/Media-test.ts new file mode 100644 index 0000000000..d4db3677cf --- /dev/null +++ b/test/customisations/Media-test.ts @@ -0,0 +1,39 @@ +/* +Copyright 2024 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import fetchMockJest from "fetch-mock-jest"; +import { mocked } from "jest-mock"; + +import { mediaFromMxc } from "../../src/customisations/Media"; +import { stubClient } from "../test-utils"; + +describe("Media", () => { + it("should not download error if server returns one", async () => { + const cli = stubClient(); + // eslint-disable-next-line no-restricted-properties + mocked(cli.mxcUrlToHttp).mockImplementation( + (mxc) => `https://matrix.org/_matrix/media/r0/download/${mxc.slice(6)}`, + ); + + fetchMockJest.get("https://matrix.org/_matrix/media/r0/download/matrix.org/1234", { + status: 404, + body: { errcode: "M_NOT_FOUND", error: "Not found" }, + }); + + const media = mediaFromMxc("mxc://matrix.org/1234"); + await expect(media.downloadSource()).rejects.toThrow("Not found"); + }); +}); diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index 88e09b8ad5..7b9cdf725d 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -172,7 +172,7 @@ export function createTestClient(): MatrixClient { content: {}, }); }), - mxcUrlToHttp: (mxc: string) => `http://this.is.a.url/${mxc.substring(6)}`, + mxcUrlToHttp: jest.fn().mockImplementation((mxc: string) => `http://this.is.a.url/${mxc.substring(6)}`), scheduleAllGroupSessionsForBackup: jest.fn().mockResolvedValue(undefined), setAccountData: jest.fn(), setRoomAccountData: jest.fn(),