From 8e0663685e616168a208c204510360ca45417188 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 9 Nov 2022 14:44:24 -0800 Subject: [PATCH 01/24] Implement sortReportActions function --- src/CONST.js | 8 ++ src/libs/ReportActionsUtils.js | 28 +++++++ tests/unit/ReportActionsUtilsTest.js | 111 +++++++++++++++++++++++++++ 3 files changed, 147 insertions(+) create mode 100644 tests/unit/ReportActionsUtilsTest.js diff --git a/src/CONST.js b/src/CONST.js index bd710cef41d5..4a4cbb06e01c 100755 --- a/src/CONST.js +++ b/src/CONST.js @@ -861,4 +861,12 @@ const CONST = { TFA_CODE_LENGTH: 6, }; +CONST.REPORT_ACTION_TYPE_SORT_ORDER = { + [CONST.REPORT.ACTIONS.TYPE.CREATED]: 1, + [CONST.REPORT.ACTIONS.TYPE.RENAMED]: 2, + [CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT]: 3, + [CONST.REPORT.ACTIONS.TYPE.IOU]: 4, + [CONST.REPORT.ACTIONS.TYPE.CLOSED]: 5, +}; + export default CONST; diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 6616d03511e8..73cfa4220084 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -147,6 +147,33 @@ function getOptimisticLastReadSequenceNumberForDeletedAction(reportID, actionsTo return sortedActions[lastMessageIndex].sequenceNumber; } +/** + * Sort an array of reportActions by their created timestamp, + * using actionName as a secondary sorting parameter, + * finally falling back on reportActionID in case multiple reportActions of the same actionName are created in the same millisecond. + * + * Note: this sorts the array in-place instead of returning a copy + * + * @param {Array} reportActions + */ +function sortReportActions(reportActions) { + if (!_.isArray(reportActions)) { + throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); + } + + reportActions.sort((first, second) => { + if (first.created !== second.created) { + return first.created < second.created ? -1 : 1; + } + + if (first.actionName !== second.actionName) { + return CONST.REPORT_ACTION_TYPE_SORT_ORDER[first.actionName] - CONST.REPORT_ACTION_TYPE_SORT_ORDER[second.actionName]; + } + + return first.reportActionID < second.reportActionID ? -1 : 1; + }); +} + export { getOptimisticLastReadSequenceNumberForDeletedAction, getLastVisibleMessageText, @@ -154,4 +181,5 @@ export { getMostRecentIOUReportSequenceNumber, isDeletedAction, isConsecutiveActionMadeByPreviousActor, + sortReportActions, }; diff --git a/tests/unit/ReportActionsUtilsTest.js b/tests/unit/ReportActionsUtilsTest.js new file mode 100644 index 000000000000..f3421f789cdd --- /dev/null +++ b/tests/unit/ReportActionsUtilsTest.js @@ -0,0 +1,111 @@ +import CONST from '../../src/CONST'; +import * as ReportActionsUtils from '../../src/libs/ReportActionsUtils'; + +describe('ReportActionsUtils', () => { + describe('sortReportActions', () => { + test('sorts by created, then actionName, then reportActionID', () => { + const reportActions = [ + // This is the highest created timestamp, so should appear last + { + created: '2022-11-09 22:27:01.825', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '8401445780099176', + }, + + // These are all created in the same millisecond, but have different actionName + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, + reportActionID: '6805724708869158', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, + reportActionID: '4175125197039964', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '3583459600062524', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '7052827918713536', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + reportActionID: '7004168689538312', + }, + + // Then lastly, some reportActions created in the same millisecond and with the same actionName + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '2962390724708756', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1609646094152486', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1661970171066218', + }, + ]; + + ReportActionsUtils.sortReportActions(reportActions); + + expect(reportActions).toStrictEqual([ + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + reportActionID: '7004168689538312', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, + reportActionID: '4175125197039964', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '3583459600062524', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '7052827918713536', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, + reportActionID: '6805724708869158', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1609646094152486', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1661970171066218', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '2962390724708756', + }, + { + created: '2022-11-09 22:27:01.825', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '8401445780099176', + }, + ]); + }); + }); +}); From 117732aa7805ef67fa4cea4a337fd5ecbcc6e168 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 9 Nov 2022 14:50:30 -0800 Subject: [PATCH 02/24] Move sortReportActions to the top of ReportActionsUtils --- src/libs/ReportActionsUtils.js | 56 +++++++++++++++++----------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 73cfa4220084..ca5045821471 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -21,6 +21,33 @@ Onyx.connect({ }, }); +/** + * Sort an array of reportActions by their created timestamp, + * using actionName as a secondary sorting parameter, + * finally falling back on reportActionID in case multiple reportActions of the same actionName are created in the same millisecond. + * + * Note: this sorts the array in-place instead of returning a copy + * + * @param {Array} reportActions + */ +function sortReportActions(reportActions) { + if (!_.isArray(reportActions)) { + throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); + } + + reportActions.sort((first, second) => { + if (first.created !== second.created) { + return first.created < second.created ? -1 : 1; + } + + if (first.actionName !== second.actionName) { + return CONST.REPORT_ACTION_TYPE_SORT_ORDER[first.actionName] - CONST.REPORT_ACTION_TYPE_SORT_ORDER[second.actionName]; + } + + return first.reportActionID < second.reportActionID ? -1 : 1; + }); +} + /** * @param {Object} reportAction * @returns {Boolean} @@ -147,39 +174,12 @@ function getOptimisticLastReadSequenceNumberForDeletedAction(reportID, actionsTo return sortedActions[lastMessageIndex].sequenceNumber; } -/** - * Sort an array of reportActions by their created timestamp, - * using actionName as a secondary sorting parameter, - * finally falling back on reportActionID in case multiple reportActions of the same actionName are created in the same millisecond. - * - * Note: this sorts the array in-place instead of returning a copy - * - * @param {Array} reportActions - */ -function sortReportActions(reportActions) { - if (!_.isArray(reportActions)) { - throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); - } - - reportActions.sort((first, second) => { - if (first.created !== second.created) { - return first.created < second.created ? -1 : 1; - } - - if (first.actionName !== second.actionName) { - return CONST.REPORT_ACTION_TYPE_SORT_ORDER[first.actionName] - CONST.REPORT_ACTION_TYPE_SORT_ORDER[second.actionName]; - } - - return first.reportActionID < second.reportActionID ? -1 : 1; - }); -} - export { + sortReportActions, getOptimisticLastReadSequenceNumberForDeletedAction, getLastVisibleMessageText, getSortedReportActions, getMostRecentIOUReportSequenceNumber, isDeletedAction, isConsecutiveActionMadeByPreviousActor, - sortReportActions, }; From e1ac5b2994762552fa7bde4a0fd0a1cf9ae414ed Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 9 Nov 2022 14:55:10 -0800 Subject: [PATCH 03/24] Preemptively re-implement getMostRecentIOUReportActionID --- src/libs/ReportActionsUtils.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index ca5045821471..a4c1f2703867 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -82,14 +82,17 @@ function getSortedReportActions(reportActions) { * Finds most recent IOU report action number. * * @param {Array} reportActions - * @returns {Number} + * @returns {String} */ -function getMostRecentIOUReportSequenceNumber(reportActions) { - return _.chain(reportActions) - .sortBy('sequenceNumber') - .filter(action => action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) - .max(action => action.sequenceNumber) - .value().sequenceNumber; +function getMostRecentIOUReportActionID(reportActions) { + // TODO: HOLD on https://github.com/Expensify/App/pull/12604 + const iouActions = _.where(reportActions, {actionName: CONST.REPORT.ACTIONS.TYPE.IOU}); + if (_.empty(iouActions)) { + return null; + } + + sortReportActions(iouActions); + return _.last(iouActions).reportActionID; } /** @@ -179,7 +182,7 @@ export { getOptimisticLastReadSequenceNumberForDeletedAction, getLastVisibleMessageText, getSortedReportActions, - getMostRecentIOUReportSequenceNumber, + getMostRecentIOUReportActionID, isDeletedAction, isConsecutiveActionMadeByPreviousActor, }; From f7652b71ab651c2d16ee9ffd9b124f249534a699 Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 9 Nov 2022 15:19:40 -0800 Subject: [PATCH 04/24] use sortReportActions to sort view --- src/libs/ReportActionsUtils.js | 24 ++-------------------- src/pages/home/report/ReportActionsList.js | 17 +++++---------- src/pages/home/report/ReportActionsView.js | 4 ++-- 3 files changed, 9 insertions(+), 36 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 840ee190fca9..5b840e7a0315 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -29,12 +29,12 @@ Onyx.connect({ * Note: this sorts the array in-place instead of returning a copy * * @param {Array} reportActions + * @returns {Array} */ function sortReportActions(reportActions) { if (!_.isArray(reportActions)) { throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); } - reportActions.sort((first, second) => { if (first.created !== second.created) { return first.created < second.created ? -1 : 1; @@ -46,6 +46,7 @@ function sortReportActions(reportActions) { return first.reportActionID < second.reportActionID ? -1 : 1; }); + return reportActions; } /** @@ -58,26 +59,6 @@ function isDeletedAction(reportAction) { return message.length === 0 || lodashGet(message, [0, 'html']) === ''; } -/** - * Sorts the report actions by sequence number, filters out any that should not be shown and formats them for display. - * - * @param {Array} reportActions - * @returns {Array} - */ -function getSortedReportActions(reportActions) { - return _.chain(reportActions) - .sortBy('sequenceNumber') - .filter(action => action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU - - // All comment actions are shown unless they are deleted and non-pending - || (action.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && (!isDeletedAction(action) || !_.isEmpty(action.pendingAction))) - || action.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED - || action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) - .map((item, index) => ({action: item, index})) - .value() - .reverse(); -} - /** * Finds most recent IOU report action number. * @@ -180,7 +161,6 @@ export { sortReportActions, getOptimisticLastReadSequenceNumberForDeletedAction, getLastVisibleMessageText, - getSortedReportActions, getMostRecentIOUReportActionID, isDeletedAction, isConsecutiveActionMadeByPreviousActor, diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index df0fd3c2da40..a7c6febee860 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -30,13 +30,7 @@ const propTypes = { report: reportPropTypes.isRequired, /** Sorted actions prepared for display */ - sortedReportActions: PropTypes.arrayOf(PropTypes.shape({ - /** Index of the action in the array */ - index: PropTypes.number, - - /** The action itself */ - action: PropTypes.shape(reportActionPropTypes), - })).isRequired, + sortedReportActions: PropTypes.arrayOf(PropTypes.shape(reportActionPropTypes)).isRequired, /** The ID of the most recent IOU report action connected with the shown report */ mostRecentIOUReportActionID: PropTypes.string, @@ -119,24 +113,23 @@ class ReportActionsList extends React.Component { * See: https://reactnative.dev/docs/optimizing-flatlist-configuration#avoid-anonymous-function-on-renderitem * * @param {Object} args - * @param {Object} args.item * @param {Number} args.index * * @returns {React.Component} */ renderItem({ - item, index, + ...reportAction }) { // When the new indicator should not be displayed we explicitly set it to 0. The marker should never be shown above the // created action (which will have sequenceNumber of 0) so we use 0 to indicate "hidden". const shouldDisplayNewIndicator = this.props.newMarkerSequenceNumber > 0 - && item.action.sequenceNumber === this.props.newMarkerSequenceNumber - && !ReportActionsUtils.isDeletedAction(item.action); + && reportAction.sequenceNumber === this.props.newMarkerSequenceNumber + && !ReportActionsUtils.isDeletedAction(reportAction); return ( Date: Wed, 9 Nov 2022 15:26:09 -0800 Subject: [PATCH 05/24] Fix typo and some outdated item.action references --- src/libs/ReportActionsUtils.js | 10 +++++----- src/pages/home/report/ReportActionsList.js | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 5b840e7a0315..bf39e928dfd8 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -67,7 +67,7 @@ function isDeletedAction(reportAction) { */ function getMostRecentIOUReportActionID(reportActions) { const iouActions = _.where(reportActions, {actionName: CONST.REPORT.ACTIONS.TYPE.IOU}); - if (_.empty(iouActions)) { + if (_.isEmpty(iouActions)) { return null; } @@ -94,17 +94,17 @@ function isConsecutiveActionMadeByPreviousActor(reportActions, actionIndex) { } // Comments are only grouped if they happen within 5 minutes of each other - if (currentAction.action.timestamp - previousAction.action.timestamp > 300) { + if (currentAction.timestamp - previousAction.timestamp > 300) { return false; } // Do not group if previous or current action was a renamed action - if (previousAction.action.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED - || currentAction.action.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED) { + if (previousAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED + || currentAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED) { return false; } - return currentAction.action.actorEmail === previousAction.action.actorEmail; + return currentAction.actorEmail === previousAction.actorEmail; } /** diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index a7c6febee860..8b84f80b014b 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -98,12 +98,12 @@ class ReportActionsList extends React.Component { * Create a unique key for each action in the FlatList. * We use the reportActionID that is a string representation of a random 64-bit int, which should be * random enough to avoid collisions - * @param {Object} item - * @param {Object} item.action + * @param {Object} reportAction + * @param {String} reportAction.reportActionID * @return {String} */ - keyExtractor(item) { - return `${item.action.clientID}${item.action.reportActionID}`; + keyExtractor(reportAction) { + return reportAction.reportActionID; } /** @@ -132,7 +132,7 @@ class ReportActionsList extends React.Component { action={reportAction} displayAsGroup={ReportActionsUtils.isConsecutiveActionMadeByPreviousActor(this.props.sortedReportActions, index)} shouldDisplayNewIndicator={shouldDisplayNewIndicator} - isMostRecentIOUReportAction={item.action.reportActionID === this.props.mostRecentIOUReportActionID} + isMostRecentIOUReportAction={reportAction.reportActionID === this.props.mostRecentIOUReportActionID} hasOutstandingIOU={this.props.report.hasOutstandingIOU} index={index} /> From ec49005ee9e1c34e3422ee34783e200090fac9aa Mon Sep 17 00:00:00 2001 From: rory Date: Wed, 9 Nov 2022 15:56:35 -0800 Subject: [PATCH 06/24] Fix typo and add FIXME for created --- src/libs/ReportActionsUtils.js | 1 + src/pages/home/report/ReportActionsList.js | 2 +- tests/unit/ReportActionsUtilsTest.js | 10 ++++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index bf39e928dfd8..9e74ca8ef8c6 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -32,6 +32,7 @@ Onyx.connect({ * @returns {Array} */ function sortReportActions(reportActions) { + // FIXME: sorting order is broken because `created` looks like "Sep 28 2022 9:26am PDT" instead of "2022-09-28 09:26:XX.xx" if (!_.isArray(reportActions)) { throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); } diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 8b84f80b014b..738fea6dbf43 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -118,8 +118,8 @@ class ReportActionsList extends React.Component { * @returns {React.Component} */ renderItem({ + item: reportAction, index, - ...reportAction }) { // When the new indicator should not be displayed we explicitly set it to 0. The marker should never be shown above the // created action (which will have sequenceNumber of 0) so we use 0 to indicate "hidden". diff --git a/tests/unit/ReportActionsUtilsTest.js b/tests/unit/ReportActionsUtilsTest.js index f3421f789cdd..efe9c58b61e3 100644 --- a/tests/unit/ReportActionsUtilsTest.js +++ b/tests/unit/ReportActionsUtilsTest.js @@ -11,6 +11,11 @@ describe('ReportActionsUtils', () => { actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '8401445780099176', }, + { + created: '2022-11-09 22:27:01.600', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '6401435781022176', + }, // These are all created in the same millisecond, but have different actionName { @@ -100,6 +105,11 @@ describe('ReportActionsUtils', () => { actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '2962390724708756', }, + { + created: '2022-11-09 22:27:01.600', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '6401435781022176', + }, { created: '2022-11-09 22:27:01.825', actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, From 02a3d344932e6c16dcd564ca9a8d44e370a5e9e8 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 22 Nov 2022 14:43:41 -0800 Subject: [PATCH 07/24] Remove outdated FIXME --- src/libs/ReportActionsUtils.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 917b2abbc7db..20c3ba238220 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -39,7 +39,6 @@ Onyx.connect({ * @returns {Array} */ function sortReportActions(reportActions) { - // FIXME: sorting order is broken because `created` looks like "Sep 28 2022 9:26am PDT" instead of "2022-09-28 09:26:XX.xx" if (!_.isArray(reportActions)) { throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); } From c8f36fb5d0737a65a12baea43d6f850911ca5f92 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 22 Nov 2022 14:58:11 -0800 Subject: [PATCH 08/24] Update a few new usages of sortBy with sequenceNumber --- src/libs/ReportActionsUtils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 20c3ba238220..29834eca4f3c 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -125,7 +125,7 @@ function isConsecutiveActionMadeByPreviousActor(reportActions, actionIndex) { function getLastVisibleMessageText(reportID, actionsToMerge = {}) { const parser = new ExpensiMark(); const actions = _.toArray(lodashMerge({}, allReportActions[reportID], actionsToMerge)); - const sortedActions = _.sortBy(actions, 'sequenceNumber'); + const sortedActions = sortReportActions(actions); const lastMessageIndex = _.findLastIndex(sortedActions, action => ( !isDeletedAction(action) )); @@ -153,7 +153,7 @@ function getOptimisticLastReadSequenceNumberForDeletedAction(reportID, actionsTo // Otherwise, we must find the first previous index of an action that is not deleted and less than the lastReadSequenceNumber const actions = _.toArray(lodashMerge({}, allReportActions[reportID], actionsToMerge)); - const sortedActions = _.sortBy(actions, 'sequenceNumber'); + const sortedActions = sortReportActions(actions); const lastMessageIndex = _.findLastIndex(sortedActions, action => ( !isDeletedAction(action) && action.sequenceNumber <= lastReadSequenceNumber )); From 4cc45c4aa5e64c1ea9736dfe0604b4b94e35d0b4 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 22 Nov 2022 15:12:56 -0800 Subject: [PATCH 09/24] Fix a few bugs from bad merge --- src/libs/ReportActionsUtils.js | 2 +- src/pages/home/report/ReportActionsList.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 29834eca4f3c..c546e87f46e3 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -93,7 +93,7 @@ function getMostRecentIOUReportActionID(reportActions) { function isConsecutiveActionMadeByPreviousActor(reportActions, actionIndex) { // Find the next non-pending deletion report action, as the pending delete action means that it is not displayed in the UI, but still is in the report actions list. // If we are offline, all actions are pending but shown in the UI, so we take the previous action, even if it is a delete. - const previousAction = _.find(_.drop(reportActions, actionIndex + 1), action => isNetworkOffline || (action.action.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)); + const previousAction = _.find(_.drop(reportActions, actionIndex + 1), action => isNetworkOffline || (action.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)); const currentAction = reportActions[actionIndex]; // It's OK for there to be no previous action, and in that case, false will be returned diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index e698104b737f..15bb034251eb 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -102,7 +102,7 @@ class ReportActionsList extends React.Component { * @return {String} */ keyExtractor(item) { - return item.action.reportActionID; + return item.reportActionID; } /** From 588568605d31de9cb32aeb76aa7f6ec5bc95b4c9 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 22 Nov 2022 15:29:33 -0800 Subject: [PATCH 10/24] Reverse sorting order in ReportActionsView for inverted FlatList --- src/pages/home/report/ReportActionsView.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 0ac10a7a5020..8b1aee4f2b9e 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -74,7 +74,7 @@ class ReportActionsView extends React.Component { }; this.currentScrollOffset = 0; - this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(props.reportActions)); + this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(props.reportActions)).reverse(); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(props.reportActions); this.trackScroll = this.trackScroll.bind(this); this.toggleFloatingMessageCounter = this.toggleFloatingMessageCounter.bind(this); @@ -130,7 +130,7 @@ class ReportActionsView extends React.Component { shouldComponentUpdate(nextProps, nextState) { if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) { - this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(nextProps.reportActions)); + this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(nextProps.reportActions)).reverse(); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions); return true; } From 878b2600c9a92e8ed193387f2ae22e99693ebb3e Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 22 Nov 2022 15:41:32 -0800 Subject: [PATCH 11/24] Implement faster inverted sorting order --- src/libs/ReportActionsUtils.js | 10 +- src/pages/home/report/ReportActionsView.js | 4 +- tests/unit/ReportActionsUtilsTest.js | 227 +++++++++++---------- 3 files changed, 126 insertions(+), 115 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index c546e87f46e3..2d08a936295b 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -36,22 +36,24 @@ Onyx.connect({ * Note: this sorts the array in-place instead of returning a copy * * @param {Array} reportActions + * @param {Boolean} [inverted] * @returns {Array} */ -function sortReportActions(reportActions) { +function sortReportActions(reportActions, inverted = false) { if (!_.isArray(reportActions)) { throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); } + const invertedMultiplier = inverted ? -1 : 1; reportActions.sort((first, second) => { if (first.created !== second.created) { - return first.created < second.created ? -1 : 1; + return (first.created < second.created ? -1 : 1) * invertedMultiplier; } if (first.actionName !== second.actionName) { - return CONST.REPORT_ACTION_TYPE_SORT_ORDER[first.actionName] - CONST.REPORT_ACTION_TYPE_SORT_ORDER[second.actionName]; + return (CONST.REPORT_ACTION_TYPE_SORT_ORDER[first.actionName] - CONST.REPORT_ACTION_TYPE_SORT_ORDER[second.actionName]) * invertedMultiplier; } - return first.reportActionID < second.reportActionID ? -1 : 1; + return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier; }); return reportActions; } diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 8b1aee4f2b9e..bce46d123461 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -74,7 +74,7 @@ class ReportActionsView extends React.Component { }; this.currentScrollOffset = 0; - this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(props.reportActions)).reverse(); + this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(props.reportActions), true); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(props.reportActions); this.trackScroll = this.trackScroll.bind(this); this.toggleFloatingMessageCounter = this.toggleFloatingMessageCounter.bind(this); @@ -130,7 +130,7 @@ class ReportActionsView extends React.Component { shouldComponentUpdate(nextProps, nextState) { if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) { - this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(nextProps.reportActions)).reverse(); + this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(nextProps.reportActions), true); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions); return true; } diff --git a/tests/unit/ReportActionsUtilsTest.js b/tests/unit/ReportActionsUtilsTest.js index efe9c58b61e3..6d5c467a77fa 100644 --- a/tests/unit/ReportActionsUtilsTest.js +++ b/tests/unit/ReportActionsUtilsTest.js @@ -3,119 +3,128 @@ import * as ReportActionsUtils from '../../src/libs/ReportActionsUtils'; describe('ReportActionsUtils', () => { describe('sortReportActions', () => { - test('sorts by created, then actionName, then reportActionID', () => { - const reportActions = [ - // This is the highest created timestamp, so should appear last - { - created: '2022-11-09 22:27:01.825', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '8401445780099176', - }, - { - created: '2022-11-09 22:27:01.600', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, - reportActionID: '6401435781022176', - }, + const cases = [ + [ + [ + // This is the highest created timestamp, so should appear last + { + created: '2022-11-09 22:27:01.825', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '8401445780099176', + }, + { + created: '2022-11-09 22:27:01.600', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '6401435781022176', + }, - // These are all created in the same millisecond, but have different actionName - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, - reportActionID: '6805724708869158', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, - reportActionID: '4175125197039964', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '3583459600062524', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, - reportActionID: '7052827918713536', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, - reportActionID: '7004168689538312', - }, + // These are all created in the same millisecond, but have different actionName + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, + reportActionID: '6805724708869158', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, + reportActionID: '4175125197039964', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '3583459600062524', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '7052827918713536', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + reportActionID: '7004168689538312', + }, - // Then lastly, some reportActions created in the same millisecond and with the same actionName - { - created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '2962390724708756', - }, - { - created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '1609646094152486', - }, - { - created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '1661970171066218', - }, - ]; + // Then lastly, some reportActions created in the same millisecond and with the same actionName + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '2962390724708756', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1609646094152486', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1661970171066218', + }, + ], + [ + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + reportActionID: '7004168689538312', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, + reportActionID: '4175125197039964', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '3583459600062524', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '7052827918713536', + }, + { + created: '2022-11-09 22:25:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, + reportActionID: '6805724708869158', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1609646094152486', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '1661970171066218', + }, + { + created: '2022-11-09 22:26:48.789', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '2962390724708756', + }, + { + created: '2022-11-09 22:27:01.600', + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + reportActionID: '6401435781022176', + }, + { + created: '2022-11-09 22:27:01.825', + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + reportActionID: '8401445780099176', + }, + ], + ], + ]; - ReportActionsUtils.sortReportActions(reportActions); + test.each(cases)('sorts by created, then actionName, then reportActionID', (input, expectedOutput) => { + const result = ReportActionsUtils.sortReportActions(input); + expect(result).toStrictEqual(expectedOutput); + }); - expect(reportActions).toStrictEqual([ - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, - reportActionID: '7004168689538312', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, - reportActionID: '4175125197039964', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '3583459600062524', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, - reportActionID: '7052827918713536', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, - reportActionID: '6805724708869158', - }, - { - created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '1609646094152486', - }, - { - created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '1661970171066218', - }, - { - created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '2962390724708756', - }, - { - created: '2022-11-09 22:27:01.600', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, - reportActionID: '6401435781022176', - }, - { - created: '2022-11-09 22:27:01.825', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '8401445780099176', - }, - ]); + test.each(cases)('inverted parameter', (input, expectedOutput) => { + const result = ReportActionsUtils.sortReportActions(input, true); + expect(result).toStrictEqual(expectedOutput.reverse()); }); }); }); From 78e5342dfdf9a0ff8814853d75e03abc2ef06a75 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 22 Nov 2022 15:49:38 -0800 Subject: [PATCH 12/24] Fix variable naming --- src/libs/ReportActionsUtils.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 2d08a936295b..ead298acb645 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -36,14 +36,14 @@ Onyx.connect({ * Note: this sorts the array in-place instead of returning a copy * * @param {Array} reportActions - * @param {Boolean} [inverted] + * @param {Boolean} [shouldInvertSortingOrder] * @returns {Array} */ -function sortReportActions(reportActions, inverted = false) { +function sortReportActions(reportActions, shouldInvertSortingOrder = false) { if (!_.isArray(reportActions)) { throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); } - const invertedMultiplier = inverted ? -1 : 1; + const invertedMultiplier = shouldInvertSortingOrder ? -1 : 1; reportActions.sort((first, second) => { if (first.created !== second.created) { return (first.created < second.created ? -1 : 1) * invertedMultiplier; From d7b94a8eb6cf8f6669e76cf186b88a3ad9f146b7 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 22 Nov 2022 15:50:34 -0800 Subject: [PATCH 13/24] Remove outdated comment --- src/libs/ReportActionsUtils.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index ead298acb645..f38e933968a1 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -33,8 +33,6 @@ Onyx.connect({ * using actionName as a secondary sorting parameter, * finally falling back on reportActionID in case multiple reportActions of the same actionName are created in the same millisecond. * - * Note: this sorts the array in-place instead of returning a copy - * * @param {Array} reportActions * @param {Boolean} [shouldInvertSortingOrder] * @returns {Array} From 54e042387de6aa593f75504acd9372f0619ecdc5 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 28 Nov 2022 15:09:27 -0800 Subject: [PATCH 14/24] Remove actionName from sorting criteria and tests --- src/CONST.js | 8 ---- src/libs/ReportActionsUtils.js | 4 -- tests/unit/ReportActionsUtilsTest.js | 65 +--------------------------- 3 files changed, 1 insertion(+), 76 deletions(-) diff --git a/src/CONST.js b/src/CONST.js index a17be0027027..833985247776 100755 --- a/src/CONST.js +++ b/src/CONST.js @@ -866,12 +866,4 @@ const CONST = { TFA_CODE_LENGTH: 6, }; -CONST.REPORT_ACTION_TYPE_SORT_ORDER = { - [CONST.REPORT.ACTIONS.TYPE.CREATED]: 1, - [CONST.REPORT.ACTIONS.TYPE.RENAMED]: 2, - [CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT]: 3, - [CONST.REPORT.ACTIONS.TYPE.IOU]: 4, - [CONST.REPORT.ACTIONS.TYPE.CLOSED]: 5, -}; - export default CONST; diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index f38e933968a1..b2f95b8b0118 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -47,10 +47,6 @@ function sortReportActions(reportActions, shouldInvertSortingOrder = false) { return (first.created < second.created ? -1 : 1) * invertedMultiplier; } - if (first.actionName !== second.actionName) { - return (CONST.REPORT_ACTION_TYPE_SORT_ORDER[first.actionName] - CONST.REPORT_ACTION_TYPE_SORT_ORDER[second.actionName]) * invertedMultiplier; - } - return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier; }); return reportActions; diff --git a/tests/unit/ReportActionsUtilsTest.js b/tests/unit/ReportActionsUtilsTest.js index 6d5c467a77fa..d3bb9047ce75 100644 --- a/tests/unit/ReportActionsUtilsTest.js +++ b/tests/unit/ReportActionsUtilsTest.js @@ -1,4 +1,3 @@ -import CONST from '../../src/CONST'; import * as ReportActionsUtils from '../../src/libs/ReportActionsUtils'; describe('ReportActionsUtils', () => { @@ -9,108 +8,46 @@ describe('ReportActionsUtils', () => { // This is the highest created timestamp, so should appear last { created: '2022-11-09 22:27:01.825', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '8401445780099176', }, { created: '2022-11-09 22:27:01.600', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, reportActionID: '6401435781022176', }, - // These are all created in the same millisecond, but have different actionName - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, - reportActionID: '6805724708869158', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, - reportActionID: '4175125197039964', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '3583459600062524', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, - reportActionID: '7052827918713536', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, - reportActionID: '7004168689538312', - }, - - // Then lastly, some reportActions created in the same millisecond and with the same actionName + // These reportActions were created in the same millisecond so should appear ordered by reportActionID { created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '2962390724708756', }, { created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '1609646094152486', }, { created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '1661970171066218', }, ], [ - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, - reportActionID: '7004168689538312', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, - reportActionID: '4175125197039964', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, - reportActionID: '3583459600062524', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, - reportActionID: '7052827918713536', - }, - { - created: '2022-11-09 22:25:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, - reportActionID: '6805724708869158', - }, { created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '1609646094152486', }, { created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '1661970171066218', }, { created: '2022-11-09 22:26:48.789', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '2962390724708756', }, { created: '2022-11-09 22:27:01.600', - actionName: CONST.REPORT.ACTIONS.TYPE.IOU, reportActionID: '6401435781022176', }, { created: '2022-11-09 22:27:01.825', - actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, reportActionID: '8401445780099176', }, ], From f31d0371028e17466619345c480bd98b4dcde890 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 28 Nov 2022 16:31:37 -0800 Subject: [PATCH 15/24] Address review comments --- src/libs/ReportActionsUtils.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index b2f95b8b0118..da53a0977738 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -29,9 +29,8 @@ Onyx.connect({ }); /** - * Sort an array of reportActions by their created timestamp, - * using actionName as a secondary sorting parameter, - * finally falling back on reportActionID in case multiple reportActions of the same actionName are created in the same millisecond. + * Sort an array of reportActions by their created timestamp first, and reportActionID second + * This gives us a stable order even in the case of multiple reportActions created on the same millisecond * * @param {Array} reportActions * @param {Boolean} [shouldInvertSortingOrder] @@ -39,7 +38,7 @@ Onyx.connect({ */ function sortReportActions(reportActions, shouldInvertSortingOrder = false) { if (!_.isArray(reportActions)) { - throw new Error(`ReportActionsUtils::sortReportActions requires an array, received ${typeof reportActions}`); + throw new Error(`ReportActionsUtils.sortReportActions requires an array, received ${typeof reportActions}`); } const invertedMultiplier = shouldInvertSortingOrder ? -1 : 1; reportActions.sort((first, second) => { @@ -74,8 +73,8 @@ function getMostRecentIOUReportActionID(reportActions) { return null; } - sortReportActions(iouActions); - return _.last(iouActions).reportActionID; + const sortedReportActions = sortReportActions(iouActions); + return _.last(sortedReportActions).reportActionID; } /** From 90b18f955000816d14b4889c1ca2dfa667308d00 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 28 Nov 2022 16:33:15 -0800 Subject: [PATCH 16/24] Rename shouldInvertSortingOrder to shouldSortInDescendingOrder --- src/libs/ReportActionsUtils.js | 6 +++--- tests/unit/ReportActionsUtilsTest.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index da53a0977738..7763c6fbd3fa 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -33,14 +33,14 @@ Onyx.connect({ * This gives us a stable order even in the case of multiple reportActions created on the same millisecond * * @param {Array} reportActions - * @param {Boolean} [shouldInvertSortingOrder] + * @param {Boolean} shouldSortInDescendingOrder * @returns {Array} */ -function sortReportActions(reportActions, shouldInvertSortingOrder = false) { +function sortReportActions(reportActions, shouldSortInDescendingOrder = false) { if (!_.isArray(reportActions)) { throw new Error(`ReportActionsUtils.sortReportActions requires an array, received ${typeof reportActions}`); } - const invertedMultiplier = shouldInvertSortingOrder ? -1 : 1; + const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1; reportActions.sort((first, second) => { if (first.created !== second.created) { return (first.created < second.created ? -1 : 1) * invertedMultiplier; diff --git a/tests/unit/ReportActionsUtilsTest.js b/tests/unit/ReportActionsUtilsTest.js index d3bb9047ce75..e474eb91d759 100644 --- a/tests/unit/ReportActionsUtilsTest.js +++ b/tests/unit/ReportActionsUtilsTest.js @@ -59,7 +59,7 @@ describe('ReportActionsUtils', () => { expect(result).toStrictEqual(expectedOutput); }); - test.each(cases)('inverted parameter', (input, expectedOutput) => { + test.each(cases)('in descending order', (input, expectedOutput) => { const result = ReportActionsUtils.sortReportActions(input, true); expect(result).toStrictEqual(expectedOutput.reverse()); }); From 0deab40ab723640bbeed6c35ce953ee20f628cb8 Mon Sep 17 00:00:00 2001 From: rory Date: Mon, 28 Nov 2022 16:36:35 -0800 Subject: [PATCH 17/24] Rename sortReportActions to getSortedReportActions --- src/libs/ReportActionsUtils.js | 12 ++++++------ src/pages/home/report/ReportActionsView.js | 4 ++-- tests/unit/ReportActionsUtilsTest.js | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 7763c6fbd3fa..8f9eefdc70a2 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -36,9 +36,9 @@ Onyx.connect({ * @param {Boolean} shouldSortInDescendingOrder * @returns {Array} */ -function sortReportActions(reportActions, shouldSortInDescendingOrder = false) { +function getSortedReportActions(reportActions, shouldSortInDescendingOrder = false) { if (!_.isArray(reportActions)) { - throw new Error(`ReportActionsUtils.sortReportActions requires an array, received ${typeof reportActions}`); + throw new Error(`ReportActionsUtils.getSortedReportActions requires an array, received ${typeof reportActions}`); } const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1; reportActions.sort((first, second) => { @@ -73,7 +73,7 @@ function getMostRecentIOUReportActionID(reportActions) { return null; } - const sortedReportActions = sortReportActions(iouActions); + const sortedReportActions = getSortedReportActions(iouActions); return _.last(sortedReportActions).reportActionID; } @@ -120,7 +120,7 @@ function isConsecutiveActionMadeByPreviousActor(reportActions, actionIndex) { function getLastVisibleMessageText(reportID, actionsToMerge = {}) { const parser = new ExpensiMark(); const actions = _.toArray(lodashMerge({}, allReportActions[reportID], actionsToMerge)); - const sortedActions = sortReportActions(actions); + const sortedActions = getSortedReportActions(actions); const lastMessageIndex = _.findLastIndex(sortedActions, action => ( !isDeletedAction(action) )); @@ -148,7 +148,7 @@ function getOptimisticLastReadSequenceNumberForDeletedAction(reportID, actionsTo // Otherwise, we must find the first previous index of an action that is not deleted and less than the lastReadSequenceNumber const actions = _.toArray(lodashMerge({}, allReportActions[reportID], actionsToMerge)); - const sortedActions = sortReportActions(actions); + const sortedActions = getSortedReportActions(actions); const lastMessageIndex = _.findLastIndex(sortedActions, action => ( !isDeletedAction(action) && action.sequenceNumber <= lastReadSequenceNumber )); @@ -162,7 +162,7 @@ function getOptimisticLastReadSequenceNumberForDeletedAction(reportID, actionsTo } export { - sortReportActions, + getSortedReportActions, getOptimisticLastReadSequenceNumberForDeletedAction, getLastVisibleMessageText, getMostRecentIOUReportActionID, diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index bce46d123461..b70aaa2d43dd 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -74,7 +74,7 @@ class ReportActionsView extends React.Component { }; this.currentScrollOffset = 0; - this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(props.reportActions), true); + this.sortedReportActions = ReportActionsUtils.getSortedReportActions(_.values(props.reportActions), true); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(props.reportActions); this.trackScroll = this.trackScroll.bind(this); this.toggleFloatingMessageCounter = this.toggleFloatingMessageCounter.bind(this); @@ -130,7 +130,7 @@ class ReportActionsView extends React.Component { shouldComponentUpdate(nextProps, nextState) { if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) { - this.sortedReportActions = ReportActionsUtils.sortReportActions(_.values(nextProps.reportActions), true); + this.sortedReportActions = ReportActionsUtils.getSortedReportActions(_.values(nextProps.reportActions), true); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions); return true; } diff --git a/tests/unit/ReportActionsUtilsTest.js b/tests/unit/ReportActionsUtilsTest.js index e474eb91d759..945b7033614e 100644 --- a/tests/unit/ReportActionsUtilsTest.js +++ b/tests/unit/ReportActionsUtilsTest.js @@ -1,7 +1,7 @@ import * as ReportActionsUtils from '../../src/libs/ReportActionsUtils'; describe('ReportActionsUtils', () => { - describe('sortReportActions', () => { + describe('getSortedReportActions', () => { const cases = [ [ [ @@ -55,12 +55,12 @@ describe('ReportActionsUtils', () => { ]; test.each(cases)('sorts by created, then actionName, then reportActionID', (input, expectedOutput) => { - const result = ReportActionsUtils.sortReportActions(input); + const result = ReportActionsUtils.getSortedReportActions(input); expect(result).toStrictEqual(expectedOutput); }); test.each(cases)('in descending order', (input, expectedOutput) => { - const result = ReportActionsUtils.sortReportActions(input, true); + const result = ReportActionsUtils.getSortedReportActions(input, true); expect(result).toStrictEqual(expectedOutput.reverse()); }); }); From f6bca7cd06b4292ae1a768cc143861591859e308 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 29 Nov 2022 14:05:45 -0800 Subject: [PATCH 18/24] Move function back to where it was to reduce diff --- src/libs/ReportActionsUtils.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 8f9eefdc70a2..bbf77fb026be 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -28,6 +28,16 @@ Onyx.connect({ callback: val => isNetworkOffline = lodashGet(val, 'isOffline', false), }); +/** + * @param {Object} reportAction + * @returns {Boolean} + */ +function isDeletedAction(reportAction) { + // A deleted comment has either an empty array or an object with html field with empty string as value + const message = lodashGet(reportAction, 'message', []); + return message.length === 0 || lodashGet(message, [0, 'html']) === ''; +} + /** * Sort an array of reportActions by their created timestamp first, and reportActionID second * This gives us a stable order even in the case of multiple reportActions created on the same millisecond @@ -51,16 +61,6 @@ function getSortedReportActions(reportActions, shouldSortInDescendingOrder = fal return reportActions; } -/** - * @param {Object} reportAction - * @returns {Boolean} - */ -function isDeletedAction(reportAction) { - // A deleted comment has either an empty array or an object with html field with empty string as value - const message = lodashGet(reportAction, 'message', []); - return message.length === 0 || lodashGet(message, [0, 'html']) === ''; -} - /** * Finds most recent IOU report action number. * From 07e5fabaaf40e41120c525cb2516999595a7cb9e Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 29 Nov 2022 14:19:41 -0800 Subject: [PATCH 19/24] Add filtering back to reportActions --- src/pages/home/report/ReportActionsView.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index b70aaa2d43dd..68dfb8d80525 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -74,7 +74,7 @@ class ReportActionsView extends React.Component { }; this.currentScrollOffset = 0; - this.sortedReportActions = ReportActionsUtils.getSortedReportActions(_.values(props.reportActions), true); + this.sortedReportActions = this.getSortedReportActionsForDisplay(props.reportActions); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(props.reportActions); this.trackScroll = this.trackScroll.bind(this); this.toggleFloatingMessageCounter = this.toggleFloatingMessageCounter.bind(this); @@ -130,7 +130,7 @@ class ReportActionsView extends React.Component { shouldComponentUpdate(nextProps, nextState) { if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) { - this.sortedReportActions = ReportActionsUtils.getSortedReportActions(_.values(nextProps.reportActions), true); + this.sortedReportActions = this.getSortedReportActionsForDisplay(nextProps.reportActions); this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions); return true; } @@ -247,6 +247,20 @@ class ReportActionsView extends React.Component { Report.unsubscribeFromReportChannel(this.props.report.reportID); } + /** + * @param {Object} reportActions + * @returns {Array} + */ + getSortedReportActionsForDisplay(reportActions) { + const sortedReportActions = ReportActionsUtils.getSortedReportActions(_.values(reportActions), true); + return _.filter(sortedReportActions, (reportAction) => { + // All actions are displayed except deleted, non-pending actions + const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction); + const isPendingAction = !_.isEmpty(reportAction.pendingAction); + return !(isDeletedAction && !isPendingAction); + }); + } + /** * @returns {Boolean} */ From 3f78a8c70c717a13508c6b20a3bbb038ba0fefe7 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 29 Nov 2022 14:23:02 -0800 Subject: [PATCH 20/24] Filter out any unsupported action types --- src/pages/home/report/ReportActionsView.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index 68dfb8d80525..bc88b3aa1500 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -254,7 +254,12 @@ class ReportActionsView extends React.Component { getSortedReportActionsForDisplay(reportActions) { const sortedReportActions = ReportActionsUtils.getSortedReportActions(_.values(reportActions), true); return _.filter(sortedReportActions, (reportAction) => { - // All actions are displayed except deleted, non-pending actions + // First, filter out any unsupported reportAction types + if (!_.has(CONST.REPORT.ACTIONS.TYPE, reportAction.actionName)) { + return false; + } + + // Then all actions are displayed except deleted, non-pending actions const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction); const isPendingAction = !_.isEmpty(reportAction.pendingAction); return !(isDeletedAction && !isPendingAction); From 21bac929f0f52ca2cf70f1225a8eb10406659e40 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 29 Nov 2022 14:26:45 -0800 Subject: [PATCH 21/24] Extract filter function to ReportActionsUtils --- src/libs/ReportActionsUtils.js | 21 +++++++++++++++++++++ src/pages/home/report/ReportActionsView.js | 12 +----------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index bbf77fb026be..fbb65673469d 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -61,6 +61,26 @@ function getSortedReportActions(reportActions, shouldSortInDescendingOrder = fal return reportActions; } +/** + * Filter out any reportActions which should not be displayed. + * + * @param {Array} reportActions + * @returns {Array} + */ +function filterReportActionsForDisplay(reportActions) { + return _.filter(reportActions, (reportAction) => { + // First, filter out any unsupported reportAction types + if (!_.has(CONST.REPORT.ACTIONS.TYPE, reportAction.actionName)) { + return false; + } + + // Then all actions are displayed except deleted, non-pending actions + const isDeleted = isDeletedAction(reportAction); + const isPending = !_.isEmpty(reportAction.pendingAction); + return !(isDeleted && !isPending); + }); +} + /** * Finds most recent IOU report action number. * @@ -163,6 +183,7 @@ function getOptimisticLastReadSequenceNumberForDeletedAction(reportID, actionsTo export { getSortedReportActions, + filterReportActionsForDisplay, getOptimisticLastReadSequenceNumberForDeletedAction, getLastVisibleMessageText, getMostRecentIOUReportActionID, diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js index bc88b3aa1500..ff8304bcd281 100755 --- a/src/pages/home/report/ReportActionsView.js +++ b/src/pages/home/report/ReportActionsView.js @@ -253,17 +253,7 @@ class ReportActionsView extends React.Component { */ getSortedReportActionsForDisplay(reportActions) { const sortedReportActions = ReportActionsUtils.getSortedReportActions(_.values(reportActions), true); - return _.filter(sortedReportActions, (reportAction) => { - // First, filter out any unsupported reportAction types - if (!_.has(CONST.REPORT.ACTIONS.TYPE, reportAction.actionName)) { - return false; - } - - // Then all actions are displayed except deleted, non-pending actions - const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction); - const isPendingAction = !_.isEmpty(reportAction.pendingAction); - return !(isDeletedAction && !isPendingAction); - }); + return ReportActionsUtils.filterReportActionsForDisplay(sortedReportActions); } /** From 0f826a73bd1bc9f8af613d15cf5a3dcc3d75c3bc Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 29 Nov 2022 14:38:19 -0800 Subject: [PATCH 22/24] Add tests for filter function --- tests/unit/ReportActionsUtilsTest.js | 56 ++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/unit/ReportActionsUtilsTest.js b/tests/unit/ReportActionsUtilsTest.js index 945b7033614e..ccda75b52587 100644 --- a/tests/unit/ReportActionsUtilsTest.js +++ b/tests/unit/ReportActionsUtilsTest.js @@ -1,3 +1,4 @@ +import CONST from '../../src/CONST'; import * as ReportActionsUtils from '../../src/libs/ReportActionsUtils'; describe('ReportActionsUtils', () => { @@ -64,4 +65,59 @@ describe('ReportActionsUtils', () => { expect(result).toStrictEqual(expectedOutput.reverse()); }); }); + + describe('filterReportActionsForDisplay', () => { + it('should filter out non-whitelisted actions', () => { + const input = [ + { + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + message: [{html: 'Hello world'}], + }, + { + actionName: CONST.REPORT.ACTIONS.TYPE.CLOSED, + message: [{html: 'Hello world'}], + }, + { + actionName: CONST.REPORT.ACTIONS.TYPE.CREATED, + message: [{html: 'Hello world'}], + }, + { + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + message: [{html: 'Hello world'}], + }, + { + actionName: CONST.REPORT.ACTIONS.TYPE.RENAMED, + message: [{html: 'Hello world'}], + }, + { + actionName: 'REIMBURSED', + message: [{html: 'Hello world'}], + }, + ]; + const result = ReportActionsUtils.filterReportActionsForDisplay(input); + input.pop(); + expect(result).toStrictEqual(input); + }); + + it('should filter out deleted, non-pending comments', () => { + const input = [ + { + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + message: [{html: 'Hello world'}], + }, + { + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + message: [{html: ''}], + pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, + }, + { + actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, + message: [{html: ''}], + }, + ]; + const result = ReportActionsUtils.filterReportActionsForDisplay(input); + input.pop(); + expect(result).toStrictEqual(input); + }); + }); }); From 9db4179203eacce0ec561ba02012b0f42acf08a2 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 29 Nov 2022 15:24:51 -0800 Subject: [PATCH 23/24] Fix automated test --- tests/ui/UnreadIndicatorsTest.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ui/UnreadIndicatorsTest.js b/tests/ui/UnreadIndicatorsTest.js index 81c954d6f3fd..b4ba16870cf7 100644 --- a/tests/ui/UnreadIndicatorsTest.js +++ b/tests/ui/UnreadIndicatorsTest.js @@ -145,6 +145,18 @@ function signInAndGetAppWithUnreadChat() { sequenceNumber: 0, created: MOMENT_TEN_MINUTES_AGO.format(MOMENT_FORMAT), reportActionID: NumberUtils.rand64(), + message: [ + { + style: 'strong', + text: '__FAKE__', + type: 'TEXT', + }, + { + style: 'normal', + text: 'created this report', + type: 'TEXT', + }, + ], }, 1: TestHelper.buildTestReportComment(USER_B_EMAIL, 1, MOMENT_TEN_MINUTES_AGO.add(10, 'seconds').format(MOMENT_FORMAT), USER_B_ACCOUNT_ID), 2: TestHelper.buildTestReportComment(USER_B_EMAIL, 2, MOMENT_TEN_MINUTES_AGO.add(20, 'seconds').format(MOMENT_FORMAT), USER_B_ACCOUNT_ID), From 9e876e8e6b9e9cad4b9583da27c81eb7446edbb4 Mon Sep 17 00:00:00 2001 From: rory Date: Tue, 29 Nov 2022 17:26:45 -0800 Subject: [PATCH 24/24] Simplify boolean --- src/libs/ReportActionsUtils.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index fbb65673469d..7946b1cbb710 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -69,15 +69,15 @@ function getSortedReportActions(reportActions, shouldSortInDescendingOrder = fal */ function filterReportActionsForDisplay(reportActions) { return _.filter(reportActions, (reportAction) => { - // First, filter out any unsupported reportAction types + // Filter out any unsupported reportAction types if (!_.has(CONST.REPORT.ACTIONS.TYPE, reportAction.actionName)) { return false; } - // Then all actions are displayed except deleted, non-pending actions + // All other actions are displayed except deleted, non-pending actions const isDeleted = isDeletedAction(reportAction); const isPending = !_.isEmpty(reportAction.pendingAction); - return !(isDeleted && !isPending); + return !isDeleted || isPending; }); }