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

Fix - Attachment - Unable to download a txt attachment until the browser is refreshed #49898

Merged
merged 10 commits into from
Oct 16, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ type BaseAnchorForAttachmentsOnlyProps = AnchorForAttachmentsOnlyProps & {
function BaseAnchorForAttachmentsOnly({style, source = '', displayName = '', onPressIn, onPressOut}: BaseAnchorForAttachmentsOnlyProps) {
const sourceURLWithAuth = addEncryptedAuthTokenToURL(source);
const sourceID = (source.match(CONST.REGEX.ATTACHMENT_ID) ?? [])[1];

const [download] = useOnyx(`${ONYXKEYS.COLLECTION.DOWNLOAD}${sourceID}`);

const {isOffline} = useNetwork();
const styles = useThemeStyles();

Expand All @@ -35,7 +37,7 @@ function BaseAnchorForAttachmentsOnly({style, source = '', displayName = '', onP
<ShowContextMenuContext.Consumer>
{({anchor, report, reportNameValuePairs, action, checkIfContextMenuActive, isDisabled}) => (
<PressableWithoutFeedback
style={[style, isOffline && styles.cursorDefault]}
style={[style, (isOffline || !sourceID) && styles.cursorDefault]}
onPress={() => {
if (isDownloading || isOffline || !sourceID) {
return;
Expand All @@ -61,6 +63,7 @@ function BaseAnchorForAttachmentsOnly({style, source = '', displayName = '', onP
shouldShowDownloadIcon={!!sourceID && !isOffline}
shouldShowLoadingSpinnerIcon={isDownloading}
isUsedAsChatAttachment
isUploading={!sourceID}
/>
</PressableWithoutFeedback>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ type DefaultAttachmentViewProps = {
containerStyles?: StyleProp<ViewStyle>;

icon?: IconAsset;

/** Flag indicating if the attachment is being uploaded. */
isUploading?: boolean;
};

function DefaultAttachmentView({fileName = '', shouldShowLoadingSpinnerIcon = false, shouldShowDownloadIcon, containerStyles, icon}: DefaultAttachmentViewProps) {
function DefaultAttachmentView({fileName = '', shouldShowLoadingSpinnerIcon = false, shouldShowDownloadIcon, containerStyles, icon, isUploading}: DefaultAttachmentViewProps) {
const theme = useTheme();
const styles = useThemeStyles();
const {translate} = useLocalize();
Expand All @@ -53,7 +56,7 @@ function DefaultAttachmentView({fileName = '', shouldShowLoadingSpinnerIcon = fa
)}
{shouldShowLoadingSpinnerIcon && (
<View style={styles.ml2}>
<Tooltip text={translate('common.downloading')}>
<Tooltip text={isUploading ? translate('common.uploading') : translate('common.downloading')}>
<ActivityIndicator
size="small"
color={theme.textSupporting}
Expand Down
87 changes: 42 additions & 45 deletions src/components/Attachments/AttachmentView/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import {Str} from 'expensify-common';
import React, {memo, useContext, useEffect, useState} from 'react';
import type {GestureResponderEvent, StyleProp, ViewStyle} from 'react-native';
import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import {useOnyx} from 'react-native-onyx';
import AttachmentCarouselPagerContext from '@components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPagerContext';
import type {Attachment, AttachmentSource} from '@components/Attachments/types';
import DistanceEReceipt from '@components/DistanceEReceipt';
Expand All @@ -24,60 +23,58 @@ import * as TransactionUtils from '@libs/TransactionUtils';
import type {ColorValue} from '@styles/utils/types';
import variables from '@styles/variables';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Transaction} from '@src/types/onyx';
import AttachmentViewImage from './AttachmentViewImage';
import AttachmentViewPdf from './AttachmentViewPdf';
import AttachmentViewVideo from './AttachmentViewVideo';
import DefaultAttachmentView from './DefaultAttachmentView';
import HighResolutionInfo from './HighResolutionInfo';

type AttachmentViewOnyxProps = {
transaction: OnyxEntry<Transaction>;
};
type AttachmentViewProps = Attachment & {
/** Whether this view is the active screen */
isFocused?: boolean;

type AttachmentViewProps = AttachmentViewOnyxProps &
Attachment & {
/** Whether this view is the active screen */
isFocused?: boolean;
/** Function for handle on press */
onPress?: (e?: GestureResponderEvent | KeyboardEvent) => void;

/** Function for handle on press */
onPress?: (e?: GestureResponderEvent | KeyboardEvent) => void;
/** Whether the attachment is used in attachment modal */
isUsedInAttachmentModal?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

we missed to add a comment to this prop when it was added, could we add it please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


isUsedInAttachmentModal?: boolean;
/** Flag to show/hide download icon */
shouldShowDownloadIcon?: boolean;

/** Flag to show/hide download icon */
shouldShowDownloadIcon?: boolean;
/** Flag to show the loading indicator */
shouldShowLoadingSpinnerIcon?: boolean;

/** Flag to show the loading indicator */
shouldShowLoadingSpinnerIcon?: boolean;
/** Notify parent that the UI should be modified to accommodate keyboard */
onToggleKeyboard?: (shouldFadeOut: boolean) => void;

/** Notify parent that the UI should be modified to accommodate keyboard */
onToggleKeyboard?: (shouldFadeOut: boolean) => void;
/** A callback when the PDF fails to load */
onPDFLoadError?: () => void;

/** A callback when the PDF fails to load */
onPDFLoadError?: () => void;
/** Extra styles to pass to View wrapper */
containerStyles?: StyleProp<ViewStyle>;

/** Extra styles to pass to View wrapper */
containerStyles?: StyleProp<ViewStyle>;
/** Denotes whether it is a workspace avatar or not */
isWorkspaceAvatar?: boolean;

/** Denotes whether it is a workspace avatar or not */
isWorkspaceAvatar?: boolean;
/** Denotes whether it is an icon (ex: SVG) */
maybeIcon?: boolean;

/** Denotes whether it is an icon (ex: SVG) */
maybeIcon?: boolean;
/** Fallback source to use in case of error */
fallbackSource?: AttachmentSource;

/** Fallback source to use in case of error */
fallbackSource?: AttachmentSource;
/* Whether it is hovered or not */
isHovered?: boolean;

/* Whether it is hovered or not */
isHovered?: boolean;
/** Whether the attachment is used as a chat attachment */
isUsedAsChatAttachment?: boolean;

/** Whether the attachment is used as a chat attachment */
isUsedAsChatAttachment?: boolean;
/* Flag indicating whether the attachment has been uploaded. */
isUploaded?: boolean;

/* Flag indicating whether the attachment has been uploaded. */
isUploaded?: boolean;
};
/** Flag indicating if the attachment is being uploaded. */
isUploading?: boolean;
};

function AttachmentView({
source,
Expand All @@ -95,16 +92,20 @@ function AttachmentView({
isWorkspaceAvatar,
maybeIcon,
fallbackSource,
transaction,
transactionID = '-1',
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a conversation about the usage of -1 as default here: https://expensify.slack.com/archives/C01GTK53T8Q/p1729014019344209?thread_ts=1728329641.957179&cid=C01GTK53T8Q

In the future I understand we want undefined or null (not sure which one), so I don't know if we should be using null or undefined or -1 here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We will continue with -1 for now and migrate later.

reportActionID,
isHovered,
duration,
isUsedAsChatAttachment,
isUploaded = true,
isUploading = false,
}: AttachmentViewProps) {
const {translate} = useLocalize();
const {updateCurrentlyPlayingURL} = usePlaybackContext();
const attachmentCarouselPagerContext = useContext(AttachmentCarouselPagerContext);

const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`);

const theme = useTheme();
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
Expand Down Expand Up @@ -288,20 +289,16 @@ function AttachmentView({
<DefaultAttachmentView
fileName={file?.name}
shouldShowDownloadIcon={shouldShowDownloadIcon}
shouldShowLoadingSpinnerIcon={shouldShowLoadingSpinnerIcon}
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
shouldShowLoadingSpinnerIcon={shouldShowLoadingSpinnerIcon || isUploading}
containerStyles={containerStyles}
isUploading={isUploading}
/>
);
}

AttachmentView.displayName = 'AttachmentView';

export default memo(
withOnyx<AttachmentViewProps, AttachmentViewOnyxProps>({
transaction: {
key: ({transactionID}) => `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
},
})(AttachmentView),
);
export default memo(AttachmentView);

export type {AttachmentViewProps};
1 change: 1 addition & 0 deletions src/languages/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ const translations = {
close: 'Close',
download: 'Download',
downloading: 'Downloading',
uploading: 'Uploading',
pin: 'Pin',
unPin: 'Unpin',
back: 'Back',
Expand Down
1 change: 1 addition & 0 deletions src/languages/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ const translations = {
close: 'Cerrar',
download: 'Descargar',
downloading: 'Descargando',
uploading: 'Subiendo',
pin: 'Fijar',
unPin: 'Desfijar',
back: 'Volver',
Expand Down
Loading