From 56bc9bb588dda8cbbc0a36936e941ccb359ceacc Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sun, 28 May 2023 11:44:42 +0300 Subject: [PATCH 1/5] Fix: ImageView is not passing `isAuthTokenRequired` It looks like `isAuthTokenRequired` was removed by mistake during a merge Reverts a change from: edd14554a32f711ca81e4955d4abca8e4c0373ea --- src/components/ImageView/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/ImageView/index.js b/src/components/ImageView/index.js index f1ec25485991..7138f4087ed4 100644 --- a/src/components/ImageView/index.js +++ b/src/components/ImageView/index.js @@ -282,6 +282,7 @@ class ImageView extends PureComponent { > Date: Sun, 28 May 2023 12:32:28 +0300 Subject: [PATCH 2/5] Fix: Enable AttachmentCarousel to work with all images Refactors AttachmentCarousel to work with images from all origins. The previous implementation was restrictive and only allowed attachments originating from Expensify. With these changes, attachments from any origin can be displayed in the carousel. This fix ensures that all Concierge attachments, regardless of their origin, will appear as expected. The parsing logic has been improved to use `htmlparser2` for accuracy and efficiency. --- src/components/AttachmentCarousel/index.js | 71 +++++++++------------- 1 file changed, 30 insertions(+), 41 deletions(-) diff --git a/src/components/AttachmentCarousel/index.js b/src/components/AttachmentCarousel/index.js index 56f592bc5865..faab1f7eb36d 100644 --- a/src/components/AttachmentCarousel/index.js +++ b/src/components/AttachmentCarousel/index.js @@ -3,6 +3,7 @@ import {View, FlatList, PixelRatio} from 'react-native'; import PropTypes from 'prop-types'; import {withOnyx} from 'react-native-onyx'; import _ from 'underscore'; +import {Parser as HtmlParser} from 'htmlparser2'; import * as Expensicons from '../Icon/Expensicons'; import styles from '../../styles/styles'; import themeColors from '../../styles/themes/default'; @@ -62,7 +63,7 @@ class AttachmentCarousel extends React.Component { this.updateZoomState = this.updateZoomState.bind(this); this.toggleArrowsVisibility = this.toggleArrowsVisibility.bind(this); - this.state = this.makeInitialState(); + this.state = this.createInitialState(); } componentDidMount() { @@ -159,39 +160,36 @@ class AttachmentCarousel extends React.Component { } /** - * Map report actions to attachment items and sets the initial carousel state + * Constructs the initial component state from report actions * @returns {{page: Number, attachments: Array, shouldShowArrow: Boolean, containerWidth: Number, isZoomed: Boolean}} */ - makeInitialState() { - let page = 0; - const actions = ReportActionsUtils.getSortedReportActions(_.values(this.props.reportActions), true); - - /** - * Looping to filter out attachments and retrieve the src URL and name of attachments. - */ + createInitialState() { + const actions = ReportActionsUtils.getSortedReportActions(_.values(this.props.reportActions)); const attachments = []; - _.forEach(actions, ({originalMessage, message}) => { - // Check for attachment which hasn't been deleted - if (!originalMessage || !originalMessage.html || _.some(message, (m) => m.isEdited)) { - return; - } - const matches = [...originalMessage.html.matchAll(CONST.REGEX.ATTACHMENT_DATA)]; - - // matchAll captured both source url and name of the attachment - if (matches.length === 2) { - const [originalSource, name] = _.map(matches, (m) => m[2]); - - // Update the image URL so the images can be accessed depending on the config environment. - // Eg: while using Ngrok the image path is from an Ngrok URL and not an Expensify URL. - const source = tryResolveUrlFromApiRoot(originalSource); - if (source === this.props.source) { - page = attachments.length; + + const htmlParser = new HtmlParser({ + onopentag: (name, attribs) => { + if (name !== 'img' || !attribs.src) { + return; } - attachments.push({source, file: {name}}); - } + const expensifySource = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]; + + // By iterating actions in chronological order and prepending each attachment + // we ensure correct order of attachments even across actions with multiple attachments. + attachments.unshift({ + source: tryResolveUrlFromApiRoot(expensifySource || attribs.src), + isAuthTokenRequired: Boolean(expensifySource), + file: {name: attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || attribs.src.split('/').pop()}, + }); + }, }); + _.forEach(actions, (action) => htmlParser.write(_.get(action, ['message', 0, 'html']))); + htmlParser.end(); + + const page = _.findIndex(attachments, (a) => a.source === this.props.source); + return { page, attachments, @@ -258,26 +256,17 @@ class AttachmentCarousel extends React.Component { /** * Defines how a single attachment should be rendered - * @param {{ source: String, file: { name: String } }} item + * @param {{ isAuthTokenRequired: Boolean, source: String, file: { name: String } }} item * @returns {JSX.Element} */ renderItem({item}) { - const authSource = addEncryptedAuthTokenToURL(item.source); - if (!this.canUseTouchScreen) { - return ( - - ); - } - return ( ); } From 81ee0e3f1f98417e30486156b968c383fb408845 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sun, 28 May 2023 12:47:33 +0300 Subject: [PATCH 3/5] AttachmentCarousel: Throw error when an image can't be mapped Enhances error handling in AttachmentCarousel to provide a clearer user experience when an image cannot be mapped to available content. Previously, if the carousel failed to find the correct image to display, it defaulted to showing the first image. This behaviour lead to user confusion as the displayed image did not correspond to the selected one. With this change, if a user tries to open an image that cannot be mapped, an error is thrown and an error page is displayed. This explicit error handling helps users understand when there's a problem opening an image, prompting them to contact support or attempt to view another image. --- src/components/AttachmentCarousel/index.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/AttachmentCarousel/index.js b/src/components/AttachmentCarousel/index.js index faab1f7eb36d..e3d5ba0ff4f3 100644 --- a/src/components/AttachmentCarousel/index.js +++ b/src/components/AttachmentCarousel/index.js @@ -189,6 +189,9 @@ class AttachmentCarousel extends React.Component { htmlParser.end(); const page = _.findIndex(attachments, (a) => a.source === this.props.source); + if (page === -1) { + throw new Error('Attachment not found'); + } return { page, From 0111200d6d51ddb73e6aea68d6f6aa8e755da1ee Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sun, 28 May 2023 13:06:30 +0300 Subject: [PATCH 4/5] AttachmentCarousel: Remove `getAttachment` No longer needed --- src/components/AttachmentCarousel/index.js | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/src/components/AttachmentCarousel/index.js b/src/components/AttachmentCarousel/index.js index e3d5ba0ff4f3..32fa78ae4899 100644 --- a/src/components/AttachmentCarousel/index.js +++ b/src/components/AttachmentCarousel/index.js @@ -70,21 +70,6 @@ class AttachmentCarousel extends React.Component { this.autoHideArrow(); } - /** - * Helps to navigate between next/previous attachments - * @param {Object} attachmentItem - * @returns {Object} - */ - getAttachment(attachmentItem) { - const source = _.get(attachmentItem, 'source', ''); - const file = _.get(attachmentItem, 'file', {name: ''}); - - return { - source, - file, - }; - } - /** * Calculate items layout information to optimize scrolling performance * @param {*} data @@ -221,7 +206,7 @@ class AttachmentCarousel extends React.Component { /** * Updates the page state when the user navigates between attachments - * @param {Array<{item: *, index: Number}>} viewableItems + * @param {Array<{item: {source, file}, index: Number}>} viewableItems */ updatePage({viewableItems}) { // Since we can have only one item in view at a time, we can use the first item in the array @@ -232,8 +217,7 @@ class AttachmentCarousel extends React.Component { } const page = entry.index; - const {source, file} = this.getAttachment(entry.item); - this.props.onNavigate({source: addEncryptedAuthTokenToURL(source), file}); + this.props.onNavigate({source: addEncryptedAuthTokenToURL(entry.item.source), file: entry.item.file}); this.setState({page, isZoomed: false}); } From fbbeddb7891d0ef3831a45c378281541a724f930 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Sun, 28 May 2023 13:49:59 +0300 Subject: [PATCH 5/5] Refactor: Improve Attachment Download Handling This commit refactors the handling of attachment downloads for a more robust and consistent experience. Previously, auth-related parameters were appended to every attachment, regardless of whether they were required. The logic was also improperly placed within the carousel component, which the attachment modal relied upon to set sources correctly. This implementation could create bugs, especially since not all attachments are rendered using the carousel. This commit addresses these issues by: The AttachmentModal now manages the state of the source and isAuthTokenRequired attributes of the attachment, ensuring that they're in sync with the attachment displayed in the carousel. The AttachmentModal now determines whether to append auth-related parameters to an attachment source during download, based on the isAuthTokenRequired flag. The responsibility of appending auth parameters has been removed from the AttachmentCarousel component, making it more reusable and focused on its core functionality. These changes ensure a more reliable and consistent attachment downloading experience, and avoid scattering logic across components. --- src/components/AttachmentCarousel/index.js | 3 +-- src/components/AttachmentModal.js | 28 +++++++++++++--------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/components/AttachmentCarousel/index.js b/src/components/AttachmentCarousel/index.js index 32fa78ae4899..6ae3214cd32a 100644 --- a/src/components/AttachmentCarousel/index.js +++ b/src/components/AttachmentCarousel/index.js @@ -11,7 +11,6 @@ import CarouselActions from './CarouselActions'; import Button from '../Button'; import * as ReportActionsUtils from '../../libs/ReportActionsUtils'; import AttachmentView from '../AttachmentView'; -import addEncryptedAuthTokenToURL from '../../libs/addEncryptedAuthTokenToURL'; import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; import CONST from '../../CONST'; import ONYXKEYS from '../../ONYXKEYS'; @@ -217,7 +216,7 @@ class AttachmentCarousel extends React.Component { } const page = entry.index; - this.props.onNavigate({source: addEncryptedAuthTokenToURL(entry.item.source), file: entry.item.file}); + this.props.onNavigate(entry.item); this.setState({page, isZoomed: false}); } diff --git a/src/components/AttachmentModal.js b/src/components/AttachmentModal.js index 642004913303..24a6ecfb3152 100755 --- a/src/components/AttachmentModal.js +++ b/src/components/AttachmentModal.js @@ -22,6 +22,7 @@ import withLocalize, {withLocalizePropTypes} from './withLocalize'; import ConfirmModal from './ConfirmModal'; import HeaderGap from './HeaderGap'; import SafeAreaConsumer from './SafeAreaConsumer'; +import addEncryptedAuthTokenToURL from '../libs/addEncryptedAuthTokenToURL'; /** * Modal render prop component that exposes modal launching triggers that can be used @@ -84,6 +85,7 @@ class AttachmentModal extends PureComponent { isModalOpen: false, shouldLoadAttachment: false, isAttachmentInvalid: false, + isAuthTokenRequired: props.isAuthTokenRequired, attachmentInvalidReasonTitle: null, attachmentInvalidReason: null, source: props.source, @@ -100,17 +102,17 @@ class AttachmentModal extends PureComponent { this.submitAndClose = this.submitAndClose.bind(this); this.closeConfirmModal = this.closeConfirmModal.bind(this); this.onNavigate = this.onNavigate.bind(this); + this.downloadAttachment = this.downloadAttachment.bind(this); this.validateAndDisplayFileToUpload = this.validateAndDisplayFileToUpload.bind(this); this.updateConfirmButtonVisibility = this.updateConfirmButtonVisibility.bind(this); } /** - * Helps to navigate between next/previous attachments - * by setting sourceURL and file in state - * @param {Object} attachmentData + * Keeps the attachment source in sync with the attachment displayed currently in the carousel. + * @param {{ source: String, isAuthTokenRequired: Boolean, file: { name: string } }} attachment */ - onNavigate(attachmentData) { - this.setState(attachmentData); + onNavigate(attachment) { + this.setState(attachment); } /** @@ -126,11 +128,15 @@ class AttachmentModal extends PureComponent { } /** - * @param {String} sourceURL + * Download the currently viewed attachment. */ - downloadAttachment(sourceURL) { - const originalFileName = lodashGet(this.state, 'file.name') || this.props.originalFileName; - fileDownload(sourceURL, originalFileName); + downloadAttachment() { + let sourceURL = this.state.source; + if (this.state.isAuthTokenRequired) { + sourceURL = addEncryptedAuthTokenToURL(sourceURL); + } + + fileDownload(sourceURL, this.state.file.name); // At ios, if the keyboard is open while opening the attachment, then after downloading // the attachment keyboard will show up. So, to fix it we need to dismiss the keyboard. @@ -274,7 +280,7 @@ class AttachmentModal extends PureComponent { title={this.props.headerTitle || this.props.translate('common.attachment')} shouldShowBorderBottom shouldShowDownloadButton={this.props.allowDownload} - onDownloadButtonPress={() => this.downloadAttachment(this.state.source)} + onDownloadButtonPress={this.downloadAttachment} onCloseButtonPress={() => this.setState({isModalOpen: false})} /> @@ -291,7 +297,7 @@ class AttachmentModal extends PureComponent {