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 props being incorrectly accessed in components #23394

Merged
merged 8 commits into from
Aug 9, 2023
20 changes: 16 additions & 4 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -2706,13 +2706,24 @@ function getParentReport(report) {
return lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`, {});
}

/**
* Return the errors we have when creating a chat or a workspace room
* @param {Object} report
* @returns {Object} errors
*/
function getAddWorkspaceRoomOrChatReportErrors(report) {
// We are either adding a workspace room, or we're creating a chat, it isn't possible for both of these to have errors for the same report at the same time, so
// simply looking up the first truthy value will get the relevant property if it's set.
return lodashGet(report, 'errorFields.addWorkspaceRoom') || lodashGet(report, 'errorFields.createChat');
}

/**
* Return true if the composer should be hidden
* @param {Object} report
* @param {Object} reportErrors
* @returns {Boolean}
*/
function shouldHideComposer(report, reportErrors) {
function shouldHideComposer(report) {
const reportErrors = getAddWorkspaceRoomOrChatReportErrors(report);
return isArchivedRoom(report) || !_.isEmpty(reportErrors) || !isAllowedToComment(report) || isAnonymousUser;
}

Expand All @@ -2728,15 +2739,15 @@ function getOriginalReportID(reportID, reportAction) {
}

/**
* Return the pendingAction and the errors when we have creating a chat or a workspace room offline
* Return the pendingAction and the errors we have when creating a chat or a workspace room offline
* @param {Object} report
* @returns {Object} pending action , errors
*/
function getReportOfflinePendingActionAndErrors(report) {
// We are either adding a workspace room, or we're creating a chat, it isn't possible for both of these to be pending, or to have errors for the same report at the same time, so
// simply looking up the first truthy value for each case will get the relevant property if it's set.
const addWorkspaceRoomOrChatPendingAction = lodashGet(report, 'pendingFields.addWorkspaceRoom') || lodashGet(report, 'pendingFields.createChat');
const addWorkspaceRoomOrChatErrors = lodashGet(report, 'errorFields.addWorkspaceRoom') || lodashGet(report, 'errorFields.createChat');
const addWorkspaceRoomOrChatErrors = getAddWorkspaceRoomOrChatReportErrors(report);
return {addWorkspaceRoomOrChatPendingAction, addWorkspaceRoomOrChatErrors};
}

Expand Down Expand Up @@ -2887,6 +2898,7 @@ export {
shouldHideComposer,
getOriginalReportID,
canAccessReport,
getAddWorkspaceRoomOrChatReportErrors,
getReportOfflinePendingActionAndErrors,
isDM,
getPolicy,
Expand Down
3 changes: 1 addition & 2 deletions src/libs/SidebarUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,8 @@ function getOptionData(report, personalDetails, preferredLocale, policy) {
result.isWaitingOnBankAccount = report.isWaitingOnBankAccount;
result.notificationPreference = report.notificationPreference || null;

const {addWorkspaceRoomOrChatErrors} = ReportUtils.getReportOfflinePendingActionAndErrors(report);
// If the composer is hidden then the user is not allowed to comment, same can be used to hide the draft icon.
result.isAllowedToComment = !ReportUtils.shouldHideComposer(report, addWorkspaceRoomOrChatErrors);
result.isAllowedToComment = !ReportUtils.shouldHideComposer(report);

const hasMultipleParticipants = participantPersonalDetailList.length > 1 || result.isChatRoom || result.isPolicyExpenseChat;
const subtitle = ReportUtils.getChatRoomSubtitle(report);
Expand Down
32 changes: 5 additions & 27 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import * as Report from '../../libs/actions/Report';
import ONYXKEYS from '../../ONYXKEYS';
import * as ReportUtils from '../../libs/ReportUtils';
import ReportActionsView from './report/ReportActionsView';
import CONST from '../../CONST';
import ReportActionsSkeletonView from '../../components/ReportActionsSkeletonView';
import reportActionPropTypes from './report/reportActionPropTypes';
import {withNetwork} from '../../components/OnyxProvider';
Expand Down Expand Up @@ -130,7 +129,6 @@ class ReportScreen extends React.Component {
this.state = {
skeletonViewContainerHeight: reportActionsListViewHeight,
isBannerVisible: true,
isReportRemoved: false,
};
this.firstRenderRef = React.createRef();
this.firstRenderRef.current = reportActionsListViewHeight === 0;
Expand Down Expand Up @@ -158,36 +156,17 @@ class ReportScreen extends React.Component {

componentDidUpdate(prevProps) {
// If composer should be hidden, hide emoji picker as well
if (ReportUtils.shouldHideComposer(this.props.report, this.props.errors)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

except this line and line 389 where errors are no longer passed to ReportFooter, the rest code in this file is reverting the PR that was blocking us - https://github.com/Expensify/App/pull/22787/files

if (ReportUtils.shouldHideComposer(this.props.report)) {
EmojiPickerAction.hideEmojiPicker(true);
}
const onyxReportID = this.props.report.reportID;
const prevOnyxReportID = prevProps.report.reportID;
const routeReportID = getReportID(this.props.route);

// navigate to concierge when the room removed from another device (e.g. user leaving a room)
// the report will not really null when removed, it will have defaultProps properties and values
if (
prevOnyxReportID &&
prevOnyxReportID === routeReportID &&
!onyxReportID &&
// non-optimistic case
(_.isEqual(this.props.report, defaultProps.report) ||
// optimistic case
(prevProps.report.statusNum === CONST.REPORT.STATUS.OPEN && this.props.report.statusNum === CONST.REPORT.STATUS.CLOSED))
) {
Navigation.goBack();
Report.navigateToConciergeChat();
// isReportRemoved will prevent <FullPageNotFoundView> showing when navigating
this.setState({isReportRemoved: true});
return;
}

// If you already have a report open and are deeplinking to a new report on native,
// the ReportScreen never actually unmounts and the reportID in the route also doesn't change.
// Therefore, we need to compare if the existing reportID is the same as the one in the route
// before deciding that we shouldn't call OpenReport.
if (onyxReportID === prevOnyxReportID && (!onyxReportID || onyxReportID === routeReportID)) {
const onyxReportID = this.props.report.reportID;
const routeReportID = getReportID(this.props.route);
if (onyxReportID === prevProps.report.reportID && (!onyxReportID || onyxReportID === routeReportID)) {
return;
}

Expand Down Expand Up @@ -315,7 +294,7 @@ class ReportScreen extends React.Component {
shouldEnableKeyboardAvoidingView={isTopMostReportId}
>
<FullPageNotFoundView
shouldShow={(!this.props.report.reportID && !this.props.report.isLoadingReportActions && !isLoading && !this.state.isReportRemoved) || shouldHideReport}
shouldShow={(!this.props.report.reportID && !this.props.report.isLoadingReportActions && !isLoading) || shouldHideReport}
subtitleKey="notFound.noAccess"
shouldShowCloseButton={false}
shouldShowBackButton={this.props.isSmallScreenWidth}
Expand Down Expand Up @@ -386,7 +365,6 @@ class ReportScreen extends React.Component {
{this.isReportReadyForDisplay() && (
<>
<ReportFooter
errors={addWorkspaceRoomOrChatErrors}
pendingAction={addWorkspaceRoomOrChatPendingAction}
isOffline={this.props.network.isOffline}
reportActions={this.props.reportActions}
Expand Down
7 changes: 2 additions & 5 deletions src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useState} from 'react';
import Animated, {useSharedValue, useAnimatedStyle, withTiming} from 'react-native-reanimated';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import InvertedFlatList from '../../../components/InvertedFlatList';
import compose from '../../../libs/compose';
import styles from '../../../styles/styles';
Expand Down Expand Up @@ -164,10 +163,8 @@ function ReportActionsList(props) {
const extraData = [props.isSmallScreenWidth ? props.newMarkerReportActionID : undefined, ReportUtils.isArchivedRoom(props.report)];
const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(props.personalDetailsList, props.report, props.currentUserPersonalDetails.accountID);

const errors = lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat');
const isArchivedRoom = ReportUtils.isArchivedRoom(props.report);
const hideComposer = ReportUtils.shouldHideComposer(props.report, errors);
const shouldOmitBottomSpace = hideComposer || isArchivedRoom;
// If composer is hidden, omit the bottom space.
const shouldOmitBottomSpace = ReportUtils.shouldHideComposer(props.report);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Archived report is already checked in shouldHideComposer so removed the duplicate condition.


return (
<Animated.View style={[animatedStyles, styles.flex1]}>
Expand Down
7 changes: 1 addition & 6 deletions src/pages/home/report/ReportFooter.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ const propTypes = {
/** Callback fired when the comment is submitted */
onSubmitComment: PropTypes.func,

/** Any errors associated with an attempt to create a chat */
// eslint-disable-next-line react/forbid-prop-types
errors: PropTypes.object,

/** The pending action when we are adding a chat */
pendingAction: PropTypes.string,

Expand All @@ -51,7 +47,6 @@ const defaultProps = {
report: {reportID: '0'},
reportActions: [],
onSubmitComment: () => {},
errors: {},
pendingAction: null,
shouldShowComposeInput: true,
shouldDisableCompose: false,
Expand All @@ -63,7 +58,7 @@ function ReportFooter(props) {
const isAnonymousUser = Session.isAnonymousUser();

const isSmallSizeLayout = props.windowWidth - (props.isSmallScreenWidth ? 0 : variables.sideBarWidth) < variables.anonymousReportFooterBreakpoint;
const hideComposer = ReportUtils.shouldHideComposer(props.report, props.errors);
const hideComposer = ReportUtils.shouldHideComposer(props.report);

return (
<>
Expand Down
25 changes: 1 addition & 24 deletions src/pages/iou/IOUCurrencySelection.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useState, useMemo, useCallback, useEffect} from 'react';
import React, {useState, useMemo, useCallback} from 'react';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
Expand All @@ -17,8 +17,6 @@ import * as CurrencyUtils from '../../libs/CurrencyUtils';
import ROUTES from '../../ROUTES';
import themeColors from '../../styles/themes/default';
import * as Expensicons from '../../components/Icon/Expensicons';
import reportPropTypes from '../reportPropTypes';
import * as ReportUtils from '../../libs/ReportUtils';

const greenCheckmark = {src: Expensicons.Checkmark, color: themeColors.success};

Expand All @@ -45,19 +43,10 @@ const propTypes = {
currency: PropTypes.string,
}),

/** The report on which the request is initiated on */
report: reportPropTypes,

/** Any errors associated with an attempt to create a chat */
// eslint-disable-next-line react/forbid-prop-types
errors: PropTypes.object,

...withLocalizePropTypes,
};

const defaultProps = {
report: {},
errors: {},
currencyList: {},
iou: {
currency: CONST.CURRENCY.USD,
Expand All @@ -71,15 +60,6 @@ function IOUCurrencySelection(props) {
const iouType = lodashGet(props.route, 'params.iouType', CONST.IOU.MONEY_REQUEST_TYPE.REQUEST);
const reportID = lodashGet(props.route, 'params.reportID', '');

const shouldDismissModal = ReportUtils.shouldHideComposer(props.report, props.errors);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mollfpr I removed this from IOUCurrencySelection page because we don't need to add it here. We check this in MoneyRequestAmountPage page and they both belong to the same stack of navigators so it works, I tested.

I even tried by pressing browser forward button but it did not let me go to that screen.

I did see your comment here and wondered why it was not working before. Anyway, please let me know if it doesn't work for you.

useEffect(() => {
if (!shouldDismissModal) {
return;
}
Navigation.dismissModal(reportID);
}, [shouldDismissModal, reportID]);

const confirmCurrencySelection = useCallback(
(option) => {
const backTo = lodashGet(props.route, 'params.backTo', '');
Expand Down Expand Up @@ -165,9 +145,6 @@ export default compose(
withOnyx({
currencyList: {key: ONYXKEYS.CURRENCY_LIST},
iou: {key: ONYXKEYS.IOU},
report: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(route, 'params.reportID', '')}`,
},
}),
withNetwork(),
)(IOUCurrencySelection);
4 changes: 2 additions & 2 deletions src/pages/iou/steps/MoneyRequestAmountPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,11 @@ function MoneyRequestAmountPage(props) {
* Check and dismiss modal
*/
useEffect(() => {
if (!ReportUtils.shouldHideComposer(props.report, props.errors)) {
if (!ReportUtils.shouldHideComposer(props.report)) {
return;
}
Navigation.dismissModal(reportID.current);
}, [props.errors, props.report]);
}, [props.report]);

/**
* Focus text input
Expand Down
Loading