Skip to content

Commit

Permalink
Merge pull request Expensify#14455 from Expensify/Rory-PickyReportAct…
Browse files Browse the repository at this point in the history 
…ionsOnyxUpdates

[No QA] Filter out reportActionID-keyed Onyx updates
  • Loading branch information
@puneetlath
puneetlath authored Jan 23, 2023
2 parents b9e4fe9 + 727a4dc commit 309a240
Show file tree
Hide file tree
Showing 5 changed files with 216 additions and 8 deletions.
41 changes: 41 additions & 0 deletions src/libs/ReportActionsUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,46 @@ function getLastVisibleMessageText(reportID, actionsToMerge = {}) {
return ReportUtils.formatReportLastMessageText(messageText);
}

/**
* Given an array of Onyx updates, this function will filter out any which attempt to update reportActions data using reportActionID as the key.
* It is a TEMPORARY measure while we migrate the shape of reportActions Onyx data. Additional context:
*
* - In GitHub: https://github.com/Expensify/App/issues/14452
* - In Slack: https://expensify.slack.com/archives/C04DC6LU2UB/p1674243288556829?thread_ts=1674242808.964579&cid=C04DC6LU2UB
*
* @param {Array} onyxUpdates – each Onyx update typically has shape: {onyxMethod: string, key: string, value: *}
* @returns {Array}
*/
function filterReportActionIDKeyedOnyxUpdates(onyxUpdates) {
return _.reduce(
onyxUpdates,
(memo, onyxUpdate) => {
if (!onyxUpdate.key.startsWith(ONYXKEYS.COLLECTION.REPORT_ACTIONS)) {
memo.push(onyxUpdate);
return memo;
}

const newValue = {};
_.each(onyxUpdate.value, (reportAction, key) => {
if (reportAction && reportAction.reportActionID === key) {
return;
}
newValue[key] = reportAction;
});

if (_.isEmpty(newValue)) {
return memo;
}

// eslint-disable-next-line no-param-reassign
onyxUpdate.value = newValue;
memo.push(onyxUpdate);
return memo;
},
[],
);
}

export {
getSortedReportActions,
filterReportActionsForDisplay,
Expand All @@ -181,4 +221,5 @@ export {
getMostRecentIOUReportActionID,
isDeletedAction,
isConsecutiveActionMadeByPreviousActor,
filterReportActionIDKeyedOnyxUpdates,
};
3 changes: 2 additions & 1 deletion src/libs/actions/Report.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ function getReportChannelName(reportID) {
function subscribeToReportCommentPushNotifications() {
PushNotification.onReceived(PushNotification.TYPE.REPORT_COMMENT, ({reportID, onyxData}) => {
Log.info('[Report] Handled event sent by Airship', false, {reportID});
Onyx.update(onyxData);
const filteredOnyxData = ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates(onyxData);
Onyx.update(filteredOnyxData);
});

// Open correct report when push notification is clicked
Expand Down
5 changes: 3 additions & 2 deletions src/libs/actions/User.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,9 @@ function subscribeToUserEvents() {
// Receive any relevant Onyx updates from the server
PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.ONYX_API_UPDATE, currentUserAccountID, (pushJSON) => {
SequentialQueue.getCurrentRequest().then(() => {
Onyx.update(pushJSON);
triggerNotifications(pushJSON);
const filteredOnyxUpdate = ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates(pushJSON);
Onyx.update(filteredOnyxUpdate);
triggerNotifications(filteredOnyxUpdate);
});
});

Expand Down
11 changes: 6 additions & 5 deletions tests/actions/ReportTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import * as PersistedRequests from '../../src/libs/actions/PersistedRequests';
import * as User from '../../src/libs/actions/User';
import * as ReportUtils from '../../src/libs/ReportUtils';
import DateUtils from '../../src/libs/DateUtils';
import * as NumberUtils from '../../src/libs/NumberUtils';

jest.mock('../../src/libs/actions/Report', () => {
const originalModule = jest.requireActual('../../src/libs/actions/Report');
Expand Down Expand Up @@ -65,16 +66,16 @@ describe('actions/Report', () => {
const TEST_USER_ACCOUNT_ID = 1;
const TEST_USER_LOGIN = 'test@test.com';
const REPORT_ID = 1;
const ACTION_ID = 1;
const REPORT_ACTION = {
reportActionID: NumberUtils.rand64(),
actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT,
actorAccountID: TEST_USER_ACCOUNT_ID,
actorEmail: TEST_USER_LOGIN,
automatic: false,
avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_3.png',
message: [{type: 'COMMENT', html: 'Testing a comment', text: 'Testing a comment'}],
person: [{type: 'TEXT', style: 'strong', text: 'Test User'}],
sequenceNumber: ACTION_ID,
sequenceNumber: 1,
shouldShow: true,
};

Expand Down Expand Up @@ -131,7 +132,7 @@ describe('actions/Report', () => {
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
value: {
[clientID]: null,
[ACTION_ID]: actionWithoutLoading,
1: actionWithoutLoading,
},
},
]);
Expand All @@ -145,7 +146,7 @@ describe('actions/Report', () => {
// Verify there is only one action and our optimistic comment has been removed
expect(_.size(reportActions)).toBe(1);

const resultAction = reportActions[ACTION_ID];
const resultAction = reportActions[1];

// Verify that our action is no longer in the loading state
expect(resultAction.pendingAction).not.toBeDefined();
Expand Down Expand Up @@ -354,9 +355,9 @@ describe('actions/Report', () => {
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`,
value: {
[_.toArray(reportActions)[0].clientID]: null,
[_.toArray(reportActions)[1].clientID]: null,
[_.toArray(reportActions)[2].clientID]: null,
[_.toArray(reportActions)[3].clientID]: null,
2: {
...USER_1_BASE_ACTION,
message: [{type: 'COMMENT', html: 'Current User Comment 1', text: 'Current User Comment 1'}],
Expand Down
164 changes: 164 additions & 0 deletions tests/unit/ReportActionsUtilsTest.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import CONST from '../../src/CONST';
import * as ReportActionsUtils from '../../src/libs/ReportActionsUtils';
import ONYXKEYS from '../../src/ONYXKEYS';

describe('ReportActionsUtils', () => {
describe('getSortedReportActions', () => {
Expand Down Expand Up @@ -192,4 +193,167 @@ describe('ReportActionsUtils', () => {
expect(result).toStrictEqual(input);
});
});

describe('filterReportActionIDKeyedOnyxUpdates', () => {
it('should not error with an empty value', () => {
expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates()).toStrictEqual([]);
});

it('should not error with an empty array', () => {
expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates([])).toStrictEqual([]);
});

it('should not affect any Onyx update not including reportActions data', () => {
const onyxUpdates = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}1234`,
value: {
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
errors: null,
},
},
];
expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates(onyxUpdates)).toStrictEqual(onyxUpdates);
});

it('should not affect a sequenceNumber-keyed Onyx update', () => {
const onyxUpdates = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}1234`,
value: {
reportID: 1234,
},
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1234`,
value: {
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
];
expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates(onyxUpdates)).toStrictEqual(onyxUpdates);
});

it('should reject a reportActionID-keyed Onyx update', () => {
expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates([
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1234`,
value: {
54321: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
])).toStrictEqual([]);
});

it('should work with multiple reportActions Onyx updates', () => {
const onyxUpdates = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1234`,
value: {
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}4321`,
value: {
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
];
expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates(onyxUpdates)).toStrictEqual(onyxUpdates);
});

it('should filter out reportAction Onyx updates keyed by reportActionID, leaving those keyed by sequenceNumber in place', () => {
const input = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1234`,
value: {
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
54321: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}4321`,
value: {
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
54321: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
];

const expectedOutput = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}1234`,
value: {
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}4321`,
value: {
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
];

expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates(input)).toStrictEqual(expectedOutput);
});

it('should not filter out the removal of any clientID-keyed reportAction', () => {
const onyxUpdates = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}4321`,
value: {
123135135: null,
1: {
sequenceNumber: 1,
reportActionID: '54321',
},
},
},
];
expect(ReportActionsUtils.filterReportActionIDKeyedOnyxUpdates(onyxUpdates)).toStrictEqual(onyxUpdates);
});
});
});

0 comments on commit 309a240

Please sign in to comment.