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.

Sign up for GitHub

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

Remove sideloading of Onyx data for attachment modal #30866

Merged
merged 12 commits into from
Nov 13, 2023
Merged

Remove sideloading of Onyx data for attachment modal #30866

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
34e3fa2
Remove sideloading of Onyx data for attachment modal
tgolen Nov 3, 2023
6df3088
Merge branch 'main' into tgolen-improve-proploading
tgolen Nov 7, 2023
d0b5968
Remove nested withOnyx
tgolen Nov 7, 2023
00d73e2
Update comment to be accurate
tgolen Nov 7, 2023
efcfe07
Fix proptypes, remove unused parameter
tgolen Nov 8, 2023
285f16f
Improve accuracy of attachment header title
tgolen Nov 9, 2023
2e403d0
Don't show the buttons until the file type is known
tgolen Nov 9, 2023
6dfa2c9
Merge branch 'main' into tgolen-improve-proploading
tgolen Nov 9, 2023
603c396
Merge branch 'main' into tgolen-improve-proploading
tgolen Nov 13, 2023
e5edd85
Use multiple withOnyx
tgolen Nov 13, 2023
a5495e0
Merge branch 'main' into tgolen-improve-proploading
tgolen Nov 13, 2023
00879f5
disable eslint for multiple onyx
tgolen Nov 13, 2023
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
13 changes: 13 additions & 0 deletions src/components/Attachments/AttachmentCarousel/attachmentCarouselPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import PropTypes from 'prop-types';
import transactionPropTypes from '@components/transactionPropTypes';
import reportActionPropTypes from '@pages/home/report/reportActionPropTypes';
import reportPropTypes from '@pages/reportPropTypes';

Expand All @@ -20,11 +21,23 @@ const propTypes = {

/** The report currently being looked at */
report: reportPropTypes.isRequired,

/** The parent of `report` */
parentReport: reportPropTypes,

/** The specific action that links to the parent report */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** The specific action that links to the parent report */
/** The specific actions that link to the parent report */

Unless I'm misunderstanding this. If we're only sending one action, can we send the action itself instead of this container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible to get the single action via withOnyx() so the component must receive all the parent report actions and then lookup the specific report action with const parentReportAction = parentReportActions[report.parentReportActionID];

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'll update the comment for this prop

parentReportActions: PropTypes.shape(reportActionPropTypes),
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of mistake is easy to miss

Suggested change
parentReportActions: PropTypes.shape(reportActionPropTypes),
parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you. I went back and forth on parentReportAction and parentReportActions and forgot to update this.


/** The transaction attached to the parent report action */
transaction: transactionPropTypes,
};

const defaultProps = {
source: '',
reportActions: {},
parentReport: {},
parentReportActions: {},
transaction: {},
onNavigate: () => {},
onClose: () => {},
setDownloadButtonVisibility: () => {},
Expand Down
7 changes: 4 additions & 3 deletions src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ import CONST from '@src/CONST';
/**
* Constructs the initial component state from report actions
* @param {Object} report
* @param {Object} parentReportAction
* @param {Array} reportActions
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why this is array

* @param {Object} transaction
* @returns {Array}
*/
function extractAttachmentsFromReport(report, reportActions) {
const actions = [ReportActionsUtils.getParentReportAction(report), ...ReportActionsUtils.getSortedReportActions(_.values(reportActions))];
function extractAttachmentsFromReport(report, parentReportAction, reportActions, transaction) {
const actions = [parentReportAction, ...ReportActionsUtils.getSortedReportActions(_.values(reportActions))];
const attachments = [];

const htmlParser = new HtmlParser({
Expand Down Expand Up @@ -51,7 +53,6 @@ function extractAttachmentsFromReport(report, reportActions) {
return;
}

const transaction = TransactionUtils.getTransaction(transactionID);
if (TransactionUtils.hasReceipt(transaction)) {
const {image} = ReceiptUtils.getThumbnailAndImageURIs(transaction);
const isLocalFile = typeof image === 'string' && (image.startsWith('blob:') || image.startsWith('file:'));
Expand Down
33 changes: 25 additions & 8 deletions src/components/Attachments/AttachmentCarousel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import lodashGet from 'lodash/get';
import React, {useCallback, useEffect, useRef, useState} from 'react';
import {FlatList, Keyboard, PixelRatio, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -9,7 +10,6 @@ import withWindowDimensions from '@components/withWindowDimensions';
import compose from '@libs/compose';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import styles from '@styles/styles';
import variables from '@styles/variables';
import ONYXKEYS from '@src/ONYXKEYS';
Expand All @@ -27,7 +27,7 @@ const viewabilityConfig = {
itemVisiblePercentThreshold: 95,
};

function AttachmentCarousel({report, reportActions, source, onNavigate, setDownloadButtonVisibility, translate}) {
function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, translate, transaction}) {
const scrollRef = useRef(null);

const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen();
Expand All @@ -42,17 +42,16 @@ function AttachmentCarousel({report, reportActions, source, onNavigate, setDownl
const compareImage = useCallback(
(attachment) => {
if (attachment.isReceipt && isReceipt) {
const action = ReportActionsUtils.getParentReportAction(report);
const transactionID = _.get(action, ['originalMessage', 'IOUTransactionID']);
return attachment.transactionID === transactionID;
return attachment.transactionID === transaction.transactionID;
}
return attachment.source === source;
},
[source, report, isReceipt],
[source, isReceipt, transaction],
);

useEffect(() => {
const attachmentsFromReport = extractAttachmentsFromReport(report, reportActions);
const parentReportAction = parentReportActions[report.parentReportActionID];
const attachmentsFromReport = extractAttachmentsFromReport(report, parentReportAction, reportActions, transaction);

const initialPage = _.findIndex(attachmentsFromReport, compareImage);

Expand All @@ -72,7 +71,7 @@ function AttachmentCarousel({report, reportActions, source, onNavigate, setDownl
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [reportActions, compareImage]);
}, [reportActions, parentReportActions, compareImage]);

/**
* Updates the page state when the user navigates between attachments
Expand Down Expand Up @@ -224,11 +223,29 @@ AttachmentCarousel.propTypes = propTypes;
AttachmentCarousel.defaultProps = defaultProps;

export default compose(
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
dangrous marked this conversation as resolved.
Show resolved Hide resolved
withOnyx({
reportActions: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`,
canEvict: false,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report ? report.parentReportID : '0'}`,
},
parentReportActions: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : '0'}`,
canEvict: false,
},
}),

// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
dangrous marked this conversation as resolved.
Show resolved Hide resolved
transaction: {
key: ({report, parentReportActions}) => {
const parentReportAction = lodashGet(parentReportActions, [report.parentReportActionID]);
return `${ONYXKEYS.COLLECTION.TRANSACTION}${lodashGet(parentReportAction, 'originalMessage.IOUTransactionID', 0)}`;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding these changes, can you confirm that parentReportID, originalMessage.IOUTransactionID can never be empty string? Because if empty string, it will return all reportActions, transactions

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 would never be able to guarantee that they could never be empty strings, but the chances of them being an empty string is very very very unlikely. This is how all key functions load data from params, so I don't think there is anything special that needs to be done here. The most important part is that lodashGet() needs to provide a default value (which it does).

},
}),
withLocalize,
withWindowDimensions,
Expand Down
31 changes: 24 additions & 7 deletions src/components/Attachments/AttachmentCarousel/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import lodashGet from 'lodash/get';
import React, {useCallback, useEffect, useRef, useState} from 'react';
import {Keyboard, PixelRatio, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -7,7 +8,6 @@ import * as Illustrations from '@components/Icon/Illustrations';
import withLocalize from '@components/withLocalize';
import compose from '@libs/compose';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import styles from '@styles/styles';
import variables from '@styles/variables';
import ONYXKEYS from '@src/ONYXKEYS';
Expand All @@ -18,7 +18,7 @@ import extractAttachmentsFromReport from './extractAttachmentsFromReport';
import AttachmentCarouselPager from './Pager';
import useCarouselArrows from './useCarouselArrows';

function AttachmentCarousel({report, reportActions, source, onNavigate, onClose, setDownloadButtonVisibility, translate}) {
function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, translate, transaction, onClose}) {
const pagerRef = useRef(null);

const [containerDimensions, setContainerDimensions] = useState({width: 0, height: 0});
Expand All @@ -32,17 +32,16 @@ function AttachmentCarousel({report, reportActions, source, onNavigate, onClose,
const compareImage = useCallback(
(attachment) => {
if (attachment.isReceipt && isReceipt) {
const action = ReportActionsUtils.getParentReportAction(report);
const transactionID = _.get(action, ['originalMessage', 'IOUTransactionID']);
return attachment.transactionID === transactionID;
return attachment.transactionID === transaction.transactionID;
}
return attachment.source === source;
},
[source, report, isReceipt],
[source, isReceipt, transaction],
);

useEffect(() => {
const attachmentsFromReport = extractAttachmentsFromReport(report, reportActions);
const parentReportAction = parentReportActions[report.parentReportActionID];
const attachmentsFromReport = extractAttachmentsFromReport(report, parentReportAction, reportActions, transaction);

const initialPage = _.findIndex(attachmentsFromReport, compareImage);

Expand Down Expand Up @@ -171,11 +170,29 @@ AttachmentCarousel.propTypes = propTypes;
AttachmentCarousel.defaultProps = defaultProps;

export default compose(
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
reportActions: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`,
canEvict: false,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report ? report.parentReportID : '0'}`,
},
parentReportActions: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report ? report.parentReportID : '0'}`,
canEvict: false,
},
}),

// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
transaction: {
key: ({report, parentReportActions}) => {
const parentReportAction = lodashGet(parentReportActions, [report.parentReportActionID]);
return `${ONYXKEYS.COLLECTION.TRANSACTION}${lodashGet(parentReportAction, 'originalMessage.IOUTransactionID', 0)}`;
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that #28866 is completely removing multiple withOnyx.

}),
withLocalize,
)(AttachmentCarousel);
Loading