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 error when clicking on attachment note #40844

Merged
Merged
Show file tree
Hide file tree
Changes from 12 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
6 changes: 6 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1184,6 +1184,12 @@ const CONST = {
WEBP: 'image/webp',
JPEG: 'image/jpeg',
},
ATTACHMENT_TYPE: {
REPORT: 'r',
REPORT_ACTION: 'a',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give me example of type a, and s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, these types are not being used. I think we can remove these and add them in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why these types were added then?
Just confirming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them based on the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

@kidroca Can you help me understand why do we need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is just an example, you can skip the keys that don't have application here

Copy link
Contributor

Choose a reason for hiding this comment

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

@nkdengineer Can you make the change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shubham1206agra I removed it

NOTE: 'n',
SINGLE: 's',
},

IMAGE_OBJECT_POSITION: {
TOP: 'top',
Expand Down
7 changes: 4 additions & 3 deletions src/ROUTES.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,10 @@ const ROUTES = {
route: 'r/:reportID/details/shareCode',
getRoute: (reportID: string) => `r/${reportID}/details/shareCode` as const,
},
REPORT_ATTACHMENTS: {
route: 'r/:reportID/attachment',
getRoute: (reportID: string, source: string) => `r/${reportID}/attachment?source=${encodeURIComponent(source)}` as const,
ATTACHMENTS: {
route: 'attachment',
getRoute: (reportID: string, type: ValueOf<typeof CONST.ATTACHMENT_TYPE>, url: string, accountID?: number) =>
`attachment?source=${encodeURIComponent(url)}&type=${type}${reportID ? `&reportID=${reportID}` : ''}${accountID ? `&accountID=${accountID}` : ''}` as const,
},
REPORT_PARTICIPANTS: {
route: 'r/:reportID/participants',
Expand Down
2 changes: 1 addition & 1 deletion src/SCREENS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type DeepValueOf from './types/utils/DeepValueOf';
const PROTECTED_SCREENS = {
HOME: 'Home',
CONCIERGE: 'Concierge',
REPORT_ATTACHMENTS: 'ReportAttachments',
ATTACHMENTS: 'Attachments',
} as const;

const SCREENS = {
Expand Down
22 changes: 22 additions & 0 deletions src/components/AttachmentContext.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import {createContext} from 'react';
import type {ValueOf} from 'type-fest';
import type CONST from '@src/CONST';

type AttachmentContextProps = {
type?: ValueOf<typeof CONST.ATTACHMENT_TYPE>;
reportID?: string;
accountID?: number;
};

const AttachmentContext = createContext<AttachmentContextProps>({
type: undefined,
reportID: undefined,
accountID: undefined,
});

AttachmentContext.displayName = 'AttachmentContext';

export {
// eslint-disable-next-line import/prefer-default-export
AttachmentContext,
};
11 changes: 11 additions & 0 deletions src/components/AttachmentModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {GestureHandlerRootView} from 'react-native-gesture-handler';
import {withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import {useSharedValue} from 'react-native-reanimated';
import type {ValueOf} from 'type-fest';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useStyleUtils from '@hooks/useStyleUtils';
Expand Down Expand Up @@ -109,6 +110,12 @@ type AttachmentModalProps = AttachmentModalOnyxProps & {
/** The report that has this attachment */
report?: OnyxEntry<OnyxTypes.Report> | EmptyObject;

/** The type of the attachment */
type?: ValueOf<typeof CONST.ATTACHMENT_TYPE>;

/** If the attachment originates from a note, the accountID will represent the author of that note. */
accountID?: number;

/** Optional callback to fire when we want to do something after modal show. */
onModalShow?: () => void;

Expand Down Expand Up @@ -167,6 +174,8 @@ function AttachmentModal({
onModalClose = () => {},
isLoading = false,
shouldShowNotFoundPage = false,
type = undefined,
accountID = undefined,
}: AttachmentModalProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
Expand Down Expand Up @@ -529,6 +538,8 @@ function AttachmentModal({
)}
{!isEmptyObject(report) && !isReceiptAttachment ? (
<AttachmentCarousel
accountID={accountID}
type={type}
report={report}
onNavigate={onNavigate}
onClose={closeModal}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import {Parser as HtmlParser} from 'htmlparser2';
import type {Attachment} from '@components/Attachments/types';
import * as FileUtils from '@libs/fileDownload/FileUtils';
import {getReport} from '@libs/ReportUtils';
import tryResolveUrlFromApiRoot from '@libs/tryResolveUrlFromApiRoot';
import CONST from '@src/CONST';

/**
* Constructs the initial component state from report actions
*/
function extractAttachmentsFromNote(reportID: string, accountID: number) {
const report = getReport(reportID);
const privateNotes = report?.privateNotes;
const targetNote = privateNotes?.[Number(accountID)]?.note ?? '';
const attachments: Attachment[] = [];

// We handle duplicate image sources by considering the first instance as original. Selecting any duplicate
// and navigating back (<) shows the image preceding the first instance, not the selected duplicate's position.
const uniqueSources = new Set();

const htmlParser = new HtmlParser({
onopentag: (name, attribs) => {
if (name === 'video') {
const source = tryResolveUrlFromApiRoot(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]);
if (uniqueSources.has(source)) {
return;
}

uniqueSources.add(source);
const splittedUrl = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE].split('/');
attachments.unshift({
source: tryResolveUrlFromApiRoot(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]),
isAuthTokenRequired: Boolean(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]),
file: {name: splittedUrl[splittedUrl.length - 1]},
duration: Number(attribs[CONST.ATTACHMENT_DURATION_ATTRIBUTE]),
isReceipt: false,
hasBeenFlagged: false,
});
return;
}

if (name === 'img' && attribs.src) {
const expensifySource = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE];
const source = tryResolveUrlFromApiRoot(expensifySource || attribs.src);
if (uniqueSources.has(source)) {
return;
}

uniqueSources.add(source);
let fileName = attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${source}`);

// Public image URLs might lack a file extension in the source URL, without an extension our
// AttachmentView fails to recognize them as images and renders fallback content instead.
// We apply this small hack to add an image extension and ensure AttachmentView renders the image.
const fileInfo = FileUtils.splitExtensionFromFileName(fileName);
if (!fileInfo.fileExtension) {
fileName = `${fileInfo.fileName || 'image'}.jpg`;
}

// By iterating actions in chronological order and prepending each attachment
// we ensure correct order of attachments even across actions with multiple attachments.
attachments.unshift({
reportActionID: attribs['data-id'],
source,
isAuthTokenRequired: Boolean(expensifySource),
file: {name: fileName},
isReceipt: false,
hasBeenFlagged: attribs['data-flagged'] === 'true',
});
}
},
});

htmlParser.write(targetNote);
htmlParser.end();

return attachments.reverse();
nkdengineer marked this conversation as resolved.
Show resolved Hide resolved
}

export default extractAttachmentsFromNote;
18 changes: 12 additions & 6 deletions src/components/Attachments/AttachmentCarousel/index.native.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,17 @@ import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import Navigation from '@libs/Navigation/Navigation';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import CarouselButtons from './CarouselButtons';
import extractAttachmentsFromNote from './extractAttachmentsFromNote';
import extractAttachmentsFromReport from './extractAttachmentsFromReport';
import type {AttachmentCarouselPagerHandle} from './Pager';
import AttachmentCarouselPager from './Pager';
import type {AttachmentCaraouselOnyxProps, AttachmentCarouselProps} from './types';
import useCarouselArrows from './useCarouselArrows';

function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, onClose}: AttachmentCarouselProps) {
function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, onClose, type, accountID}: AttachmentCarouselProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const pagerRef = useRef<AttachmentCarouselPagerHandle>(null);
Expand All @@ -31,24 +33,28 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source,
useEffect(() => {
const parentReportAction = report.parentReportActionID && parentReportActions ? parentReportActions[report.parentReportActionID] : undefined;
const attachmentsFromReport = extractAttachmentsFromReport(parentReportAction, reportActions);

const initialPage = attachmentsFromReport.findIndex(compareImage);
let attachmentsFromNote: Attachment[] = [];
if (type === CONST.ATTACHMENT_TYPE.NOTE && accountID) {
attachmentsFromNote = extractAttachmentsFromNote(report.reportID, accountID);
}
const targetAttachments = type === CONST.ATTACHMENT_TYPE.REPORT ? attachmentsFromReport : attachmentsFromNote;
nkdengineer marked this conversation as resolved.
Show resolved Hide resolved
const initialPage = targetAttachments.findIndex(compareImage);

// Dismiss the modal when deleting an attachment during its display in preview.
if (initialPage === -1 && attachments.find(compareImage)) {
Navigation.dismissModal();
} else {
setPage(initialPage);
setAttachments(attachmentsFromReport);
setAttachments(targetAttachments);

// Update the download button visibility in the parent modal
if (setDownloadButtonVisibility) {
setDownloadButtonVisibility(initialPage !== -1);
}

// Update the parent modal's state with the source and name from the mapped attachments
if (attachmentsFromReport[initialPage] !== undefined && onNavigate) {
onNavigate(attachmentsFromReport[initialPage]);
if (targetAttachments[initialPage] !== undefined && onNavigate) {
onNavigate(targetAttachments[initialPage]);
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down
20 changes: 13 additions & 7 deletions src/components/Attachments/AttachmentCarousel/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import AttachmentCarouselCellRenderer from './AttachmentCarouselCellRenderer';
import CarouselActions from './CarouselActions';
import CarouselButtons from './CarouselButtons';
import CarouselItem from './CarouselItem';
import extractAttachmentsFromNote from './extractAttachmentsFromNote';
import extractAttachmentsFromReport from './extractAttachmentsFromReport';
import type {AttachmentCaraouselOnyxProps, AttachmentCarouselProps, UpdatePageProps} from './types';
import useCarouselArrows from './useCarouselArrows';
Expand All @@ -29,7 +30,7 @@ const viewabilityConfig = {
itemVisiblePercentThreshold: 95,
};

function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility}: AttachmentCarouselProps) {
function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, type, accountID}: AttachmentCarouselProps) {
const theme = useTheme();
const {translate} = useLocalize();
const styles = useThemeStyles();
Expand All @@ -49,31 +50,36 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source,
useEffect(() => {
const parentReportAction = report.parentReportActionID && parentReportActions ? parentReportActions[report.parentReportActionID] : undefined;
const attachmentsFromReport = extractAttachmentsFromReport(parentReportAction, reportActions ?? undefined);
let attachmentsFromNote: Attachment[] = [];
if (type === CONST.ATTACHMENT_TYPE.NOTE && accountID) {
attachmentsFromNote = extractAttachmentsFromNote(report.reportID, accountID);
}
const targetAttachments = type === CONST.ATTACHMENT_TYPE.REPORT ? attachmentsFromReport : attachmentsFromNote;

if (isEqual(attachments, attachmentsFromReport)) {
if (isEqual(attachments, targetAttachments)) {
return;
}

const initialPage = attachmentsFromReport.findIndex(compareImage);
const initialPage = targetAttachments.findIndex(compareImage);

// Dismiss the modal when deleting an attachment during its display in preview.
if (initialPage === -1 && attachments.find(compareImage)) {
Navigation.dismissModal();
} else {
setPage(initialPage);
setAttachments(attachmentsFromReport);
setAttachments(targetAttachments);

// Update the download button visibility in the parent modal
if (setDownloadButtonVisibility) {
setDownloadButtonVisibility(initialPage !== -1);
}

// Update the parent modal's state with the source and name from the mapped attachments
if (attachmentsFromReport[initialPage] !== undefined && onNavigate) {
onNavigate(attachmentsFromReport[initialPage]);
if (targetAttachments[initialPage] !== undefined && onNavigate) {
onNavigate(targetAttachments[initialPage]);
}
}
}, [reportActions, parentReportActions, compareImage, report.parentReportActionID, attachments, setDownloadButtonVisibility, onNavigate]);
}, [reportActions, parentReportActions, compareImage, report.parentReportActionID, attachments, setDownloadButtonVisibility, onNavigate, accountID, report.reportID, type]);

/** Updates the page state when the user navigates between attachments */
const updatePage = useCallback(
Expand Down
8 changes: 8 additions & 0 deletions src/components/Attachments/AttachmentCarousel/types.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import type {ViewToken} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import type {Attachment, AttachmentSource} from '@components/Attachments/types';
import type CONST from '@src/CONST';
import type {Report, ReportActions} from '@src/types/onyx';

type UpdatePageProps = {
Expand Down Expand Up @@ -28,6 +30,12 @@ type AttachmentCarouselProps = AttachmentCaraouselOnyxProps & {
/** The report currently being looked at */
report: Report;

/** The type of the attachment */
type?: ValueOf<typeof CONST.ATTACHMENT_TYPE>;

/** If the attachment originates from a note, the accountID will represent the author of that note. */
accountID?: number;

/** A callback that is called when swipe-down-to-close gesture happens */
onClose: () => void;
};
Expand Down
37 changes: 24 additions & 13 deletions src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {memo} from 'react';
import {withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import type {CustomRendererProps, TBlock} from 'react-native-render-html';
import {AttachmentContext} from '@components/AttachmentContext';
import * as Expensicons from '@components/Icon/Expensicons';
import PressableWithoutFocus from '@components/Pressable/PressableWithoutFocus';
import {ShowContextMenuContext, showContextMenuForReport} from '@components/ShowContextMenuContext';
Expand Down Expand Up @@ -78,19 +79,29 @@ function ImageRenderer({tnode}: ImageRendererProps) {
) : (
<ShowContextMenuContext.Consumer>
{({anchor, report, action, checkIfContextMenuActive}) => (
<PressableWithoutFocus
style={[styles.noOutline]}
onPress={() => {
const route = ROUTES.REPORT_ATTACHMENTS.getRoute(report?.reportID ?? '', source);
Navigation.navigate(route);
}}
onLongPress={(event) => showContextMenuForReport(event, anchor, report?.reportID ?? '', action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report))}
shouldUseHapticsOnLongPress
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={translate('accessibilityHints.viewAttachment')}
>
{thumbnailImageComponent}
</PressableWithoutFocus>
<AttachmentContext.Consumer>
{({reportID, accountID, type}) => (
<PressableWithoutFocus
style={[styles.noOutline]}
onPress={() => {
if (!source || !type) {
return;
}

if (reportID) {
const route = ROUTES.ATTACHMENTS?.getRoute(reportID, type, source, accountID);
Navigation.navigate(route);
}
}}
onLongPress={(event) => showContextMenuForReport(event, anchor, report?.reportID ?? '', action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report))}
shouldUseHapticsOnLongPress
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={translate('accessibilityHints.viewAttachment')}
>
{thumbnailImageComponent}
</PressableWithoutFocus>
)}
</AttachmentContext.Consumer>
)}
</ShowContextMenuContext.Consumer>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ function VideoRenderer({tnode, key}: VideoRendererProps) {
videoDimensions={{width, height}}
videoDuration={duration}
onShowModalPress={() => {
const route = ROUTES.REPORT_ATTACHMENTS.getRoute(report?.reportID ?? '', sourceURL);
const route = ROUTES.ATTACHMENTS.getRoute(report?.reportID ?? '', CONST.ATTACHMENT_TYPE.REPORT, sourceURL);
Navigation.navigate(route);
}}
/>
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ function AuthScreens({session, lastOpenedPublicRoomID, initialLastUpdateIDApplie
getComponent={loadConciergePage}
/>
<RootStack.Screen
name={SCREENS.REPORT_ATTACHMENTS}
name={SCREENS.ATTACHMENTS}
options={{
headerShown: false,
presentation: 'transparentModal',
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/dismissModal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function dismissModal(navigationRef: NavigationContainerRef<RootStackParamList>)
case NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR:
case NAVIGATORS.FEATURE_TRANING_MODAL_NAVIGATOR:
case SCREENS.NOT_FOUND:
case SCREENS.REPORT_ATTACHMENTS:
case SCREENS.ATTACHMENTS:
case SCREENS.TRANSACTION_RECEIPT:
case SCREENS.PROFILE_AVATAR:
case SCREENS.WORKSPACE_AVATAR:
Expand Down
Loading
Loading