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

[HOLD for payment 2024-01-24] Refactor and Fix Attachment Handling in File Download and Carousel #31791

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ const propTypes = {
/** Additional information about the attachment file */
file: PropTypes.shape({
/** File name of the attachment */
name: PropTypes.string,
}),
name: PropTypes.string.isRequired,
}).isRequired,

/** Whether the attachment has been flagged */
hasBeenFlagged: PropTypes.bool,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {Parser as HtmlParser} from 'htmlparser2';
import _ from 'underscore';
import * as FileUtils from '@libs/fileDownload/FileUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import tryResolveUrlFromApiRoot from '@libs/tryResolveUrlFromApiRoot';
import CONST from '@src/CONST';
Expand All @@ -21,14 +22,16 @@ function extractAttachmentsFromReport(parentReportAction, reportActions) {
}

const expensifySource = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE];
const source = tryResolveUrlFromApiRoot(expensifySource || attribs.src);
const fileName = attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${source}`);

// 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,
reportActionID: attribs['data-id'],
source: tryResolveUrlFromApiRoot(expensifySource || attribs.src),
isAuthTokenRequired: Boolean(expensifySource),
file: {name: attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE]},
file: {name: fileName},
hasBeenFlagged: attribs['data-flagged'] === 'true',
});
},
Expand Down
4 changes: 2 additions & 2 deletions src/components/Attachments/propTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import PropTypes from 'prop-types';

const attachmentSourcePropType = PropTypes.oneOfType([PropTypes.string, PropTypes.func, PropTypes.number]);
const attachmentFilePropType = PropTypes.shape({
name: PropTypes.string,
name: PropTypes.string.isRequired,
});

const attachmentPropType = PropTypes.shape({
Expand All @@ -13,7 +13,7 @@ const attachmentPropType = PropTypes.shape({
source: attachmentSourcePropType.isRequired,

/** File object can be an instance of File or Object */
file: attachmentFilePropType,
file: attachmentFilePropType.isRequired,
});

const attachmentsPropType = PropTypes.arrayOf(attachmentPropType);
Expand Down
22 changes: 16 additions & 6 deletions src/libs/fileDownload/FileUtils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {Alert, Linking, Platform} from 'react-native';
import DateUtils from '@libs/DateUtils';
import * as Localize from '@libs/Localize';
import Log from '@libs/Log';
import CONST from '@src/CONST';
import type {ReadFileAsync, SplitExtensionFromFileName} from './types';

Expand Down Expand Up @@ -75,13 +76,22 @@ function showCameraPermissionsAlert() {
}

/**
* Generate a random file name with timestamp and file extension
* Extracts a filename from a given URL and sanitizes it for file system usage.
*
* This function takes a URL as input and performs the following operations:
* 1. Extracts the last segment of the URL, which could be a file name, a path segment,
* or a query string parameter.
* 2. Decodes the extracted segment from URL encoding to a plain string for better readability.
* 3. Replaces any characters in the decoded string that are illegal in file names
* with underscores.
*/
function getAttachmentName(url: string): string {
if (!url) {
return '';
function getFileName(url: string): string {
const fileName = url.split(/[#?/]/).pop() ?? '';
if (!fileName) {
Log.warn('[FileUtils] Could not get attachment name', {url});
}
return `${DateUtils.getDBTime()}.${url.split(/[#?]/)[0].split('.').pop()?.trim()}`;

return decodeURIComponent(fileName).replace(CONST.REGEX.ILLEGAL_FILENAME_CHARACTERS, '_');
}

function isImage(fileName: string): boolean {
Expand Down Expand Up @@ -231,7 +241,7 @@ export {
showPermissionErrorAlert,
showCameraPermissionsAlert,
splitExtensionFromFileName,
getAttachmentName,
getFileName,
getFileType,
cleanFileName,
appendTimeToFileName,
Expand Down
2 changes: 1 addition & 1 deletion src/libs/fileDownload/index.android.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function handleDownload(url: string, fileName: string): Promise<void> {

// Android files will download to Download directory
const path = dirs.DownloadDir;
const attachmentName = FileUtils.appendTimeToFileName(fileName) || FileUtils.getAttachmentName(url);
const attachmentName = FileUtils.appendTimeToFileName(fileName || FileUtils.getFileName(url));

const isLocalFile = url.startsWith('file://');

Expand Down
2 changes: 1 addition & 1 deletion src/libs/fileDownload/index.ios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ const fileDownload: FileDownload = (fileUrl, fileName) =>
new Promise((resolve) => {
let fileDownloadPromise;
const fileType = FileUtils.getFileType(fileUrl);
const attachmentName = FileUtils.appendTimeToFileName(fileName) || FileUtils.getAttachmentName(fileUrl);
const attachmentName = FileUtils.appendTimeToFileName(fileName || FileUtils.getFileName(fileUrl));

switch (fileType) {
case CONST.ATTACHMENT_FILE_TYPE.IMAGE:
Expand Down
5 changes: 1 addition & 4 deletions src/libs/fileDownload/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ const fileDownload: FileDownload = (url, fileName) => {
// adding href to anchor
link.href = href;
link.style.display = 'none';
link.setAttribute(
'download',
FileUtils.appendTimeToFileName(fileName) || FileUtils.getAttachmentName(url), // generating the file name
);
link.download = FileUtils.appendTimeToFileName(fileName || FileUtils.getFileName(url));

// Append to html link element page
document.body.appendChild(link);
Expand Down
Loading