Skip to content

Commit

Permalink
Merge pull request #12626 from Expensify/Rory-SortReportActionsByCreated
Browse files Browse the repository at this point in the history
Sort reportActions by created instead of sequenceNumber
  • Loading branch information
cead22 authored Dec 1, 2022
2 parents 76fbdeb + 9e876e8 commit 383ce45
Show file tree
Hide file tree
Showing 5 changed files with 205 additions and 41 deletions.
77 changes: 52 additions & 25 deletions src/libs/ReportActionsUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,46 @@ function isDeletedAction(reportAction) {
}

/**
* Sorts the report actions by sequence number, filters out any that should not be shown and formats them for display.
* 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} shouldSortInDescendingOrder
* @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();
function getSortedReportActions(reportActions, shouldSortInDescendingOrder = false) {
if (!_.isArray(reportActions)) {
throw new Error(`ReportActionsUtils.getSortedReportActions requires an array, received ${typeof reportActions}`);
}
const invertedMultiplier = shouldSortInDescendingOrder ? -1 : 1;
reportActions.sort((first, second) => {
if (first.created !== second.created) {
return (first.created < second.created ? -1 : 1) * invertedMultiplier;
}

return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier;
});
return reportActions;
}

/**
* Filter out any reportActions which should not be displayed.
*
* @param {Array} reportActions
* @returns {Array}
*/
function filterReportActionsForDisplay(reportActions) {
return _.filter(reportActions, (reportAction) => {
// Filter out any unsupported reportAction types
if (!_.has(CONST.REPORT.ACTIONS.TYPE, reportAction.actionName)) {
return false;
}

// All other actions are displayed except deleted, non-pending actions
const isDeleted = isDeletedAction(reportAction);
const isPending = !_.isEmpty(reportAction.pendingAction);
return !isDeleted || isPending;
});
}

/**
Expand All @@ -65,10 +88,13 @@ function getSortedReportActions(reportActions) {
* @returns {String}
*/
function getMostRecentIOUReportActionID(reportActions) {
return _.chain(reportActions)
.where({actionName: CONST.REPORT.ACTIONS.TYPE.IOU})
.max(action => action.sequenceNumber)
.value().reportActionID;
const iouActions = _.where(reportActions, {actionName: CONST.REPORT.ACTIONS.TYPE.IOU});
if (_.isEmpty(iouActions)) {
return null;
}

const sortedReportActions = getSortedReportActions(iouActions);
return _.last(sortedReportActions).reportActionID;
}

/**
Expand All @@ -82,7 +108,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
Expand All @@ -92,17 +118,17 @@ function isConsecutiveActionMadeByPreviousActor(reportActions, actionIndex) {
}

// Comments are only grouped if they happen within 5 minutes of each other
if (moment(currentAction.action.created).unix() - moment(previousAction.action.created).unix() > 300) {
if (moment(currentAction.created).unix() - moment(previousAction.created).unix() > 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;
}

/**
Expand All @@ -114,7 +140,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 = getSortedReportActions(actions);
const lastMessageIndex = _.findLastIndex(sortedActions, action => (
!isDeletedAction(action)
));
Expand Down Expand Up @@ -142,7 +168,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 = getSortedReportActions(actions);
const lastMessageIndex = _.findLastIndex(sortedActions, action => (
!isDeletedAction(action) && action.sequenceNumber <= lastReadSequenceNumber
));
Expand All @@ -156,9 +182,10 @@ function getOptimisticLastReadSequenceNumberForDeletedAction(reportID, actionsTo
}

export {
getSortedReportActions,
filterReportActionsForDisplay,
getOptimisticLastReadSequenceNumberForDeletedAction,
getLastVisibleMessageText,
getSortedReportActions,
getMostRecentIOUReportActionID,
isDeletedAction,
isConsecutiveActionMadeByPreviousActor,
Expand Down
21 changes: 7 additions & 14 deletions src/pages/home/report/ReportActionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -108,7 +102,7 @@ class ReportActionsList extends React.Component {
* @return {String}
*/
keyExtractor(item) {
return item.action.reportActionID;
return item.reportActionID;
}

/**
Expand All @@ -118,27 +112,26 @@ 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,
item: reportAction,
index,
}) {
// 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 (
<ReportActionItem
report={this.props.report}
action={item.action}
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}
/>
Expand Down
13 changes: 11 additions & 2 deletions src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class ReportActionsView extends React.Component {
};

this.currentScrollOffset = 0;
this.sortedReportActions = ReportActionsUtils.getSortedReportActions(props.reportActions);
this.sortedReportActions = this.getSortedReportActionsForDisplay(props.reportActions);
this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(props.reportActions);
this.trackScroll = this.trackScroll.bind(this);
this.toggleFloatingMessageCounter = this.toggleFloatingMessageCounter.bind(this);
Expand Down Expand Up @@ -130,7 +130,7 @@ class ReportActionsView extends React.Component {

shouldComponentUpdate(nextProps, nextState) {
if (!_.isEqual(nextProps.reportActions, this.props.reportActions)) {
this.sortedReportActions = ReportActionsUtils.getSortedReportActions(nextProps.reportActions);
this.sortedReportActions = this.getSortedReportActionsForDisplay(nextProps.reportActions);
this.mostRecentIOUReportActionID = ReportActionsUtils.getMostRecentIOUReportActionID(nextProps.reportActions);
return true;
}
Expand Down Expand Up @@ -247,6 +247,15 @@ 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 ReportActionsUtils.filterReportActionsForDisplay(sortedReportActions);
}

/**
* @returns {Boolean}
*/
Expand Down
12 changes: 12 additions & 0 deletions tests/ui/UnreadIndicatorsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
123 changes: 123 additions & 0 deletions tests/unit/ReportActionsUtilsTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import CONST from '../../src/CONST';
import * as ReportActionsUtils from '../../src/libs/ReportActionsUtils';

describe('ReportActionsUtils', () => {
describe('getSortedReportActions', () => {
const cases = [
[
[
// This is the highest created timestamp, so should appear last
{
created: '2022-11-09 22:27:01.825',
reportActionID: '8401445780099176',
},
{
created: '2022-11-09 22:27:01.600',
reportActionID: '6401435781022176',
},

// These reportActions were created in the same millisecond so should appear ordered by reportActionID
{
created: '2022-11-09 22:26:48.789',
reportActionID: '2962390724708756',
},
{
created: '2022-11-09 22:26:48.789',
reportActionID: '1609646094152486',
},
{
created: '2022-11-09 22:26:48.789',
reportActionID: '1661970171066218',
},
],
[
{
created: '2022-11-09 22:26:48.789',
reportActionID: '1609646094152486',
},
{
created: '2022-11-09 22:26:48.789',
reportActionID: '1661970171066218',
},
{
created: '2022-11-09 22:26:48.789',
reportActionID: '2962390724708756',
},
{
created: '2022-11-09 22:27:01.600',
reportActionID: '6401435781022176',
},
{
created: '2022-11-09 22:27:01.825',
reportActionID: '8401445780099176',
},
],
],
];

test.each(cases)('sorts by created, then actionName, then reportActionID', (input, expectedOutput) => {
const result = ReportActionsUtils.getSortedReportActions(input);
expect(result).toStrictEqual(expectedOutput);
});

test.each(cases)('in descending order', (input, expectedOutput) => {
const result = ReportActionsUtils.getSortedReportActions(input, true);
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);
});
});
});

0 comments on commit 383ce45

Please sign in to comment.