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

Refactor Creating a Chat to be optimistic #11439

Merged
merged 44 commits into from
Oct 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
08195d0
Update OpenReport to take a participant list, and change the optimist…
stitesExpensify Sep 27, 2022
852132c
Create new optimistic chat report when selected
stitesExpensify Sep 27, 2022
8ab056b
Only create a new chat if it doesn't already exist
stitesExpensify Sep 27, 2022
856f728
Use new optimistic chat logic for group chats
stitesExpensify Sep 27, 2022
1699c18
Update banner to allow passing of child props (such as buttons)
stitesExpensify Sep 28, 2022
ca7581c
Add report action to delete report locally from onyx
stitesExpensify Sep 28, 2022
f2a824e
make participants optional
stitesExpensify Oct 3, 2022
0e84d21
Check for both chat errors, and workspace errors
stitesExpensify Oct 3, 2022
407b9d3
Rename method to include all report types, not just workspace
stitesExpensify Oct 3, 2022
d1a12ff
Add pending/error fields for creating a chat
stitesExpensify Oct 3, 2022
220fa6c
Style
stitesExpensify Oct 3, 2022
03d91ff
Comment and move method for style
stitesExpensify Oct 3, 2022
357b113
Prevent multiple calls to openReport
stitesExpensify Oct 3, 2022
f5a3dac
Don't fetch the chat because we always call openReport
stitesExpensify Oct 3, 2022
b2589b7
Re-add routing logic, this was preventing endless loading when typing…
stitesExpensify Oct 4, 2022
2b207bd
Use set when creating a new report since there is no data yet and it …
stitesExpensify Oct 4, 2022
3136a2d
Merge main
stitesExpensify Oct 4, 2022
036f4ac
Generalize errors and pending action to work for regular chats too
stitesExpensify Oct 4, 2022
09f6711
Remove testing lines
stitesExpensify Oct 6, 2022
ae4d03a
Revert podfile changes
stitesExpensify Oct 7, 2022
c19930a
Use the one from main instead of master :facepalm:
stitesExpensify Oct 7, 2022
7a864ff
Update documentation
stitesExpensify Oct 7, 2022
8a211b9
Move GetChatByParticipants to ReportUtils
stitesExpensify Oct 7, 2022
af65b36
Update comment for clarity
stitesExpensify Oct 7, 2022
8821c0a
Add @expensify.sms when using phone numbers
stitesExpensify Oct 7, 2022
2e01251
Remove debugger
stitesExpensify Oct 7, 2022
545fc3d
Merge branch 'main' into stites-refactorCreateChat
tylerkaraszewski Oct 11, 2022
0e6d523
always convert email to lowercase
tylerkaraszewski Oct 11, 2022
7fb11c5
use equals instead of concatenating everything
tylerkaraszewski Oct 12, 2022
df5942a
oops, don't use deleted variable name
tylerkaraszewski Oct 12, 2022
d110394
Update src/libs/actions/Report.js
tylerkaraszewski Oct 12, 2022
5332999
Refactor common code into helper function
tylerkaraszewski Oct 12, 2022
7dc733b
Add explanatory comment
tylerkaraszewski Oct 12, 2022
7f3bd92
Fix function name in comment
tylerkaraszewski Oct 12, 2022
1f33629
Attach to onyx for report objects
tylerkaraszewski Oct 12, 2022
4769383
Update src/libs/ReportUtils.js
tylerkaraszewski Oct 13, 2022
1f8964d
rename vars
tylerkaraszewski Oct 13, 2022
c40516b
ReportID should be a string everywhere
tylerkaraszewski Oct 13, 2022
f53dffa
Merge branch 'main' into stites-refactorCreateChat
tylerkaraszewski Oct 13, 2022
506cb31
Merge branch 'main' into stites-refactorCreateChat
tylerkaraszewski Oct 14, 2022
3b722df
Existing prop names seem incorrect.
tylerkaraszewski Oct 14, 2022
8e2ecad
Merge branch 'main' into stites-refactorCreateChat
tylerkaraszewski Oct 17, 2022
ae29164
re-change (again) prop names after merge
tylerkaraszewski Oct 17, 2022
b3105f3
Merge branch 'main' into stites-refactorCreateChat
tylerkaraszewski Oct 20, 2022
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
24 changes: 24 additions & 0 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ Onyx.connect({
},
});

let allReports;
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: val => allReports = val,
});

/**
* Returns the concatenated title for the PrimaryLogins of a report
*
Expand Down Expand Up @@ -958,6 +965,22 @@ function shouldReportBeInOptionList(report, reportIDFromRoute, isInGSDMode, curr
return true;
}

/**
* Attempts to find a report in onyx with the provided list of participants
* @param {Array} newParticipantList
* @returns {Array|undefined}
*/
function getChatByParticipants(newParticipantList) {
newParticipantList.sort();
return _.find(allReports, (report) => {
// If the report has been deleted, or there are no participants (like an empty #admins room) then skip it
if (!report || !report.participants) {
return false;
}
return _.isEqual(newParticipantList, report.participants.sort());
});
}

export {
getReportParticipantsTitle,
isReportMessageAttachment,
Expand Down Expand Up @@ -998,4 +1021,5 @@ export {
buildOptimisticIOUReportAction,
buildOptimisticReportAction,
shouldReportBeInOptionList,
getChatByParticipants,
};
84 changes: 56 additions & 28 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -689,39 +689,67 @@ function addComment(reportID, text) {

/**
* Gets the latest page of report actions and updates the last read message
stitesExpensify marked this conversation as resolved.
Show resolved Hide resolved
* If a chat with the passed reportID is not found, we will create a chat based on the passed participantList
*
* @param {String} reportID
* @param {Array} participantList The list of users that are included in a new chat, not including the user creating it
* @param {Object} newReportObject The optimistic report object created when making a new chat, saved as optimistic data
*/
function openReport(reportID) {
function openReport(reportID, participantList = [], newReportObject = {}) {
const onyxData = {
optimisticData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: true,
lastVisitedTimestamp: Date.now(),
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
lastReadSequenceNumber: getMaxSequenceNumber(reportID),
},
}],
successData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: false,
pendingFields: {
createChat: null,
},
errorFields: {
createChat: null,
},
isOptimisticReport: false,
},
}],
Comment on lines +699 to +722
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a way to clear the failed red brick road error here? Like an error is set via PHP but how do you clear the failed created chat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the chat creation fails, we make you delete the report from the error so it clears everything from onyx

};

// If we are creating a new report, we need to add the optimistic report data and a report action
if (!_.isEmpty(newReportObject)) {
onyxData.optimisticData[0].value = {
...onyxData.optimisticData[0].value,
...newReportObject,
pendingFields: {
createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},
isOptimisticReport: true,
};

// Change the method to set for new reports because it doesn't exist yet, is faster,
// and we need the data to be available when we navigate to the chat page
onyxData.optimisticData[0].onyxMethod = CONST.ONYX.METHOD.SET;

// Also create a report action so that the page isn't endlessly loading
onyxData.optimisticData[1] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to clean up the create action too?

onyxMethod: CONST.ONYX.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: ReportUtils.buildOptimisticCreatedReportAction(newReportObject.ownerEmail),
};
}
API.write('OpenReport',
{
reportID,
emailList: participantList ? participantList.join(',') : '',
},
{
optimisticData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: true,
lastVisitedTimestamp: Date.now(),
lastReadSequenceNumber: getMaxSequenceNumber(reportID),
},
}],
successData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: false,
},
}],
failureData: [{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
isLoadingReportActions: false,
},
}],
});
onyxData);
}

/**
Expand Down Expand Up @@ -1271,7 +1299,7 @@ function addPolicyReport(policy, reportName, visibility) {
/**
* @param {String} reportID The reportID of the policy report (workspace room)
*/
function navigateToConciergeChatAndDeletePolicyReport(reportID) {
function navigateToConciergeChatAndDeleteReport(reportID) {
navigateToConciergeChat();
Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, null);
Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, null);
Expand Down Expand Up @@ -1483,7 +1511,7 @@ export {
navigateToConciergeChat,
setReportWithDraft,
addPolicyReport,
navigateToConciergeChatAndDeletePolicyReport,
navigateToConciergeChatAndDeleteReport,
setIsComposerFullSize,
markCommentAsUnread,
readNewestAction,
Expand Down
27 changes: 21 additions & 6 deletions src/pages/NewChatPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as OptionsListUtils from '../libs/OptionsListUtils';
import ONYXKEYS from '../ONYXKEYS';
import styles from '../styles/styles';
import * as Report from '../libs/actions/Report';
import * as ReportUtils from '../libs/ReportUtils';
import CONST from '../CONST';
import withWindowDimensions, {windowDimensionsPropTypes} from '../components/withWindowDimensions';
import HeaderWithCloseButton from '../components/HeaderWithCloseButton';
Expand All @@ -18,6 +19,7 @@ import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
import compose from '../libs/compose';
import personalDetailsPropType from './personalDetailsPropType';
import reportPropTypes from './reportPropTypes';
import ROUTES from '../ROUTES';

const propTypes = {
/** Whether screen is used to create group chat */
Expand Down Expand Up @@ -129,6 +131,23 @@ class NewChatPage extends Component {
return sections;
}

/**
* This will find an existing chat, or create a new one if none exists, for the given user or set of users. It will then navigate to this chat.
*
* @param {Array} userLogins list of user logins.
*/
getOrCreateChatReport(userLogins) {
const formattedUserLogins = _.map(userLogins, login => OptionsListUtils.addSMSDomainIfPhoneNumber(login).toLowerCase());
let newChat = {};
const chat = ReportUtils.getChatByParticipants(formattedUserLogins);
if (!chat) {
newChat = ReportUtils.buildOptimisticChatReport(formattedUserLogins);
}
const reportID = chat ? chat.reportID : newChat.reportID;
Report.openReport(reportID, newChat.participants, newChat);
Navigation.navigate(ROUTES.getReportRoute(reportID));
}

/**
* Removes a selected option from list if already selected. If not already selected add this option to the list.
* @param {Object} option
Expand Down Expand Up @@ -179,10 +198,7 @@ class NewChatPage extends Component {
* @param {Object} option
*/
createChat(option) {
Report.fetchOrCreateChatReport([
this.props.session.email,
option.login,
]);
this.getOrCreateChatReport([option.login]);
}

/**
Expand All @@ -194,8 +210,7 @@ class NewChatPage extends Component {
if (userLogins.length < 1) {
return;
}

Report.fetchOrCreateChatReport([this.props.session.email, ...userLogins]);
this.getOrCreateChatReport(userLogins);
}

render() {
Expand Down
14 changes: 8 additions & 6 deletions src/pages/home/ReportScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,11 @@ class ReportScreen extends React.Component {
return null;
}

// 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 reportID = getReportID(this.props.route);
const addWorkspaceRoomPendingAction = lodashGet(this.props.report, 'pendingFields.addWorkspaceRoom');
const addWorkspaceRoomErrors = lodashGet(this.props.report, 'errorFields.addWorkspaceRoom');
const addWorkspaceRoomOrChatPendingAction = lodashGet(this.props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(this.props.report, 'pendingFields.createChat');
const addWorkspaceRoomOrChatErrors = lodashGet(this.props.report, 'errorFields.addWorkspaceRoom') || lodashGet(this.props.report, 'errorFields.createChat');
const screenWrapperStyle = [styles.appContent, {marginTop: this.state.viewportOffsetTop}];
return (
<Freeze
Expand Down Expand Up @@ -241,8 +243,8 @@ class ReportScreen extends React.Component {
}}
>
<OfflineWithFeedback
pendingAction={addWorkspaceRoomPendingAction}
errors={addWorkspaceRoomErrors}
pendingAction={addWorkspaceRoomOrChatPendingAction}
errors={addWorkspaceRoomOrChatErrors}
errorRowStyles={styles.dNone}
>
<HeaderView
Expand Down Expand Up @@ -295,8 +297,8 @@ class ReportScreen extends React.Component {
isDrawerOpen={this.props.isDrawerOpen}
/>
<ReportFooter
addWorkspaceRoomErrors={addWorkspaceRoomErrors}
addWorkspaceRoomPendingAction={addWorkspaceRoomPendingAction}
errors={addWorkspaceRoomOrChatErrors}
pendingAction={addWorkspaceRoomOrChatPendingAction}
isOffline={this.props.network.isOffline}
reportActions={this.props.reportActions}
report={this.props.report}
Expand Down
6 changes: 3 additions & 3 deletions src/pages/home/report/ReportActionItemCreated.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ const ReportActionItemCreated = (props) => {
const icons = ReportUtils.getIcons(props.report, props.personalDetails, props.policies);
return (
<OfflineWithFeedback
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom')}
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom')}
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom') || lodashGet(props.report, 'pendingFields.createChat')}
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom') || lodashGet(props.report, 'errorFields.createChat')}
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
errorRowStyles={styles.addWorkspaceRoomErrorRow}
onClose={() => Report.navigateToConciergeChatAndDeletePolicyReport(props.report.reportID)}
onClose={() => Report.navigateToConciergeChatAndDeleteReport(props.report.reportID)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Child reports do not get deleted here when there is an error because we do not pass shouldDeleteChildReports to true here. More details here #45625

>
<View
accessibilityLabel="Chat welcome message"
Expand Down
18 changes: 14 additions & 4 deletions src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class ReportActionsView extends React.Component {
this.loadMoreChats = this.loadMoreChats.bind(this);
this.recordTimeToMeasureItemLayout = this.recordTimeToMeasureItemLayout.bind(this);
this.scrollToBottomAndMarkReportAsRead = this.scrollToBottomAndMarkReportAsRead.bind(this);
this.openReportIfNecessary = this.openReportIfNecessary.bind(this);
}

componentDidMount() {
Expand All @@ -95,7 +96,7 @@ class ReportActionsView extends React.Component {

// If the app user becomes active and they have no unread actions we clear the new marker to sync their device
// e.g. they could have read these messages on another device and only just become active here
Report.openReport(this.props.report.reportID);
this.openReportIfNecessary();
this.setState({newMarkerSequenceNumber: 0});
});

Expand All @@ -108,7 +109,7 @@ class ReportActionsView extends React.Component {
});

if (this.getIsReportFullyVisible()) {
Report.openReport(this.props.report.reportID);
this.openReportIfNecessary();
}
}

Expand Down Expand Up @@ -167,7 +168,7 @@ class ReportActionsView extends React.Component {
const wasNetworkChangeDetected = lodashGet(prevProps.network, 'isOffline') && !lodashGet(this.props.network, 'isOffline');
if (wasNetworkChangeDetected) {
if (isReportFullyVisible) {
Report.openReport(this.props.report.reportID);
this.openReportIfNecessary();
} else {
Report.reconnect(this.props.report.reportID);
}
Expand Down Expand Up @@ -215,7 +216,7 @@ class ReportActionsView extends React.Component {
? 0
: this.props.report.lastReadSequenceNumber + 1,
});
Report.openReport(this.props.report.reportID);
this.openReportIfNecessary();
}

// When the user navigates to the LHN the ReportActionsView doesn't unmount and just remains hidden.
Expand Down Expand Up @@ -255,6 +256,15 @@ class ReportActionsView extends React.Component {
return Visibility.isVisible() && !isSidebarCoveringReportView;
}

// If the report is optimistic (AKA not yet created) we don't need to call openReport again
openReportIfNecessary() {
if (this.props.report.isOptimisticReport) {
return;
}

Report.openReport(this.props.report.reportID);
}

Comment on lines +259 to +267
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we should move this conditional to openReport

Copy link
Contributor

Choose a reason for hiding this comment

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

The existing comment got dismissed but I will repeat it here. I think that it's confusing to have openReport sometimes not open the report, but specifically in this case:
https://github.com/Expensify/App/pull/11439/files#diff-f7eec01ae30248000c28c7d3f68c7ea00ba10e5fc64f2854474a350beb2e9556R147

We are passing the (newly created) optimistic report to openReport there, and we want it to open in that case. Adding an isOptimistic flag doesn't really work because sometimes we actually want to open the optimistic report.

What we're talking about is passing an actuallyOpenTheReport flag to OpenReport which seems obtuse.

We could also just do something like:

if (this.props.report.isOptimisticReport) {
            Report.openReport(this.props.report.reportID);
}

Instead of creating this function, but I think having this function in one place is clearer because we can have a single comment explaining it.

But basically, simply knowing whether the report is optimistic or not is not enough information to know if we neeed to open it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, because sometimes we want to create a new optimistic report, and sometimes we have are looking at an optimistic report and therefore we don't need to open it. openReportIfNecessary makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the additional context. I think that makes sense in this case then.

/**
* Retrieves the next set of report actions for the chat once we are nearing the end of what we are currently
* displaying.
Expand Down
16 changes: 8 additions & 8 deletions src/pages/home/report/ReportFooter.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ const propTypes = {
/** Callback fired when the comment is submitted */
onSubmitComment: PropTypes.func.isRequired,

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

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

/** Whether the composer input should be shown */
shouldShowComposeInput: PropTypes.bool,
Expand All @@ -47,8 +47,8 @@ const propTypes = {

const defaultProps = {
shouldShowComposeInput: true,
addWorkspaceRoomErrors: {},
addWorkspaceRoomPendingAction: null,
errors: {},
pendingAction: null,
};

class ReportFooter extends React.Component {
Expand All @@ -65,7 +65,7 @@ class ReportFooter extends React.Component {
if (isArchivedRoom) {
reportClosedAction = lodashFindLast(this.props.reportActions, action => action.actionName === CONST.REPORT.ACTIONS.TYPE.CLOSED);
}
const hideComposer = isArchivedRoom || !_.isEmpty(this.props.addWorkspaceRoomErrors);
const hideComposer = isArchivedRoom || !_.isEmpty(this.props.errors);
return (
<>
{(isArchivedRoom || hideComposer) && (
Expand All @@ -89,7 +89,7 @@ class ReportFooter extends React.Component {
<View style={[this.getChatFooterStyles(), this.props.isComposerFullSize && styles.chatFooterFullCompose]}>
<SwipeableView onSwipeDown={Keyboard.dismiss}>
<OfflineWithFeedback
pendingAction={this.props.addWorkspaceRoomPendingAction}
pendingAction={this.props.pendingAction}
style={this.props.isComposerFullSize ? styles.chatItemFullComposeRow : {}}
contentContainerStyle={this.props.isComposerFullSize ? styles.flex1 : {}}
>
Expand Down
Loading