From d6504caa7b1db4cedfbbbf0bc9fd3f3bb9bd3aea Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 20 Oct 2023 11:51:51 +0800 Subject: [PATCH 1/8] restructure report actions drafts onyx + migration --- src/ONYXKEYS.ts | 2 +- src/libs/actions/Report.js | 2 +- src/libs/migrateOnyx.js | 3 +- .../KeyReportActionsDraftByReportActionID.js | 53 ++++++++++++++++ src/pages/home/report/ReportActionItem.js | 4 +- src/types/onyx/ReportActionsDrafts.ts | 3 + src/types/onyx/index.ts | 2 + tests/unit/MigrationTest.js | 63 +++++++++++++++++++ 8 files changed, 127 insertions(+), 5 deletions(-) create mode 100644 src/libs/migrations/KeyReportActionsDraftByReportActionID.js create mode 100644 src/types/onyx/ReportActionsDrafts.ts diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index 68b3bd047ad8..8d7344f8a2d7 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -392,7 +392,7 @@ type OnyxValues = { [ONYXKEYS.COLLECTION.REPORT]: OnyxTypes.Report; [ONYXKEYS.COLLECTION.REPORT_METADATA]: OnyxTypes.ReportMetadata; [ONYXKEYS.COLLECTION.REPORT_ACTIONS]: OnyxTypes.ReportActions; - [ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS]: string; + [ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS]: OnyxTypes.ReportActionsDrafts; [ONYXKEYS.COLLECTION.REPORT_ACTIONS_REACTIONS]: OnyxTypes.ReportActionReactions; [ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT]: string; [ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT_NUMBER_OF_LINES]: number; diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js index ffb3238f004b..15ce79c373df 100644 --- a/src/libs/actions/Report.js +++ b/src/libs/actions/Report.js @@ -1300,7 +1300,7 @@ function editReportComment(reportID, originalReportAction, textForNewComment) { */ function saveReportActionDraft(reportID, reportAction, draftMessage) { const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction); - Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}_${reportAction.reportActionID}`, draftMessage); + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`, {[reportAction.reportActionID]: draftMessage}); } /** diff --git a/src/libs/migrateOnyx.js b/src/libs/migrateOnyx.js index aaa706e71fb2..2ac9f0d92d91 100644 --- a/src/libs/migrateOnyx.js +++ b/src/libs/migrateOnyx.js @@ -2,6 +2,7 @@ import _ from 'underscore'; import Log from './Log'; import PersonalDetailsByAccountID from './migrations/PersonalDetailsByAccountID'; import RenameReceiptFilename from './migrations/RenameReceiptFilename'; +import KeyReportActionsDraftByReportActionID from './migrations/KeyReportActionsDraftByReportActionID'; export default function () { const startTime = Date.now(); @@ -9,7 +10,7 @@ export default function () { return new Promise((resolve) => { // Add all migrations to an array so they are executed in order - const migrationPromises = [PersonalDetailsByAccountID, RenameReceiptFilename]; + const migrationPromises = [PersonalDetailsByAccountID, RenameReceiptFilename, KeyReportActionsDraftByReportActionID]; // Reduce all promises down to a single promise. All promises run in a linear fashion, waiting for the // previous promise to finish before moving onto the next one. diff --git a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js new file mode 100644 index 000000000000..1a2b81d84891 --- /dev/null +++ b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js @@ -0,0 +1,53 @@ +import _ from 'underscore'; +import Onyx from 'react-native-onyx'; +import Log from '../Log'; +import ONYXKEYS from '../../ONYXKEYS'; + +/** + * This migration updates reportActionsDrafts data to be keyed by reportActionID. + * + * Before: reportActionsDrafts_reportID_reportActionID: value + * After: reportActionsDrafts_reportID: {[reportActionID]: value} + * + * @returns {Promise} + */ +export default function () { + return new Promise((resolve) => { + const connectionID = Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS, + waitForCollectionCallback: true, + callback: (allReportActionsDrafts) => { + Onyx.disconnect(connectionID); + + if (!allReportActionsDrafts) { + Log.info('[Migrate Onyx] Skipped migration KeyReportActionsDraftByReportActionID because there were no reportActionsDrafts'); + return resolve(); + } + + const newReportActionsDrafts = {}; + _.each(allReportActionsDrafts, (reportActionDraft, onyxKey) => { + if (!_.isString(reportActionDraft)) { + return; + } + newReportActionsDrafts[onyxKey] = null; + + const reportActionID = onyxKey.split('_').pop(); + const newOnyxKey = onyxKey.replace(`_${reportActionID}`, ''); + newReportActionsDrafts[newOnyxKey] = { + ...(newReportActionsDrafts[newOnyxKey] || allReportActionsDrafts[newOnyxKey]), + [reportActionID]: reportActionDraft, + }; + }); + + if (_.isEmpty(newReportActionsDrafts)) { + Log.info('[Migrate Onyx] Skipped migration KeyReportActionsDraftByReportActionID because there are no actions drafts to migrate'); + return resolve(); + } + + Log.info(`[Migrate Onyx] Re-keying reportActionsDrafts by reportActionID for ${_.keys(newReportActionsDrafts).length} actions drafts`); + // eslint-disable-next-line rulesdir/prefer-actions-set-data + return Onyx.multiSet(newReportActionsDrafts).then(resolve); + }, + }); + }); +} diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js index 42bcfd49f207..16968fa9b610 100644 --- a/src/pages/home/report/ReportActionItem.js +++ b/src/pages/home/report/ReportActionItem.js @@ -716,8 +716,8 @@ export default compose( propName: 'draftMessage', transformValue: (drafts, props) => { const originalReportID = ReportUtils.getOriginalReportID(props.report.reportID, props.action); - const draftKey = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}_${props.action.reportActionID}`; - return lodashGet(drafts, draftKey, ''); + const draftKey = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`; + return lodashGet(drafts, [draftKey, props.action.reportActionID], ''); }, }), withOnyx({ diff --git a/src/types/onyx/ReportActionsDrafts.ts b/src/types/onyx/ReportActionsDrafts.ts new file mode 100644 index 000000000000..e40007b6b47a --- /dev/null +++ b/src/types/onyx/ReportActionsDrafts.ts @@ -0,0 +1,3 @@ +type ReportActionsDrafts = Record; + +export default ReportActionsDrafts; diff --git a/src/types/onyx/index.ts b/src/types/onyx/index.ts index 4603c4579343..cbe1074f630d 100644 --- a/src/types/onyx/index.ts +++ b/src/types/onyx/index.ts @@ -38,6 +38,7 @@ import PolicyCategory from './PolicyCategory'; import Report from './Report'; import ReportMetadata from './ReportMetadata'; import ReportAction, {ReportActions} from './ReportAction'; +import ReportActionsDrafts from './ReportActionsDrafts'; import ReportActionReactions from './ReportActionReactions'; import SecurityGroup from './SecurityGroup'; import Transaction from './Transaction'; @@ -89,6 +90,7 @@ export type { ReportMetadata, ReportAction, ReportActions, + ReportActionsDrafts, ReportActionReactions, SecurityGroup, Transaction, diff --git a/tests/unit/MigrationTest.js b/tests/unit/MigrationTest.js index d0e7f19d3d3f..5c048fea5ac7 100644 --- a/tests/unit/MigrationTest.js +++ b/tests/unit/MigrationTest.js @@ -4,6 +4,7 @@ import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; import Log from '../../src/libs/Log'; import PersonalDetailsByAccountID from '../../src/libs/migrations/PersonalDetailsByAccountID'; import CheckForPreviousReportActionID from '../../src/libs/migrations/CheckForPreviousReportActionID'; +import KeyReportActionsDraftByReportActionID from '../../src/libs/migrations/KeyReportActionsDraftByReportActionID'; import ONYXKEYS from '../../src/ONYXKEYS'; jest.mock('../../src/libs/getPlatform'); @@ -622,4 +623,66 @@ describe('Migrations', () => { }); })); }); + + describe('KeyReportActionsDraftByReportActionID', () => { + it("Should work even if there's no reportActionsDrafts data in Onyx", () => + KeyReportActionsDraftByReportActionID().then(() => + expect(LogSpy).toHaveBeenCalledWith('[Migrate Onyx] Skipped migration KeyReportActionsDraftByReportActionID because there were no reportActionsDrafts'), + )); + + it('Should move individual draft to a draft collection of report', () => + Onyx.multiSet({ + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_1`]: 'a', + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_2`]: 'b', + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2`]: {3: 'c'}, + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2_4`]: 'd', + }) + .then(KeyReportActionsDraftByReportActionID) + .then(() => { + const connectionID = Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS, + waitForCollectionCallback: true, + callback: (allReportActionsDrafts) => { + Onyx.disconnect(connectionID); + const expectedReportActionDraft1 = { + 1: 'a', + 2: 'b', + }; + const expectedReportActionDraft2 = { + 3: 'c', + 4: 'd', + }; + expect(allReportActionsDrafts[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_1`]).toBeUndefined(); + expect(allReportActionsDrafts[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_2`]).toBeUndefined(); + expect(allReportActionsDrafts[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2_4`]).toBeUndefined(); + expect(allReportActionsDrafts[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1`]).toMatchObject(expectedReportActionDraft1); + expect(allReportActionsDrafts[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2`]).toMatchObject(expectedReportActionDraft2); + }, + }); + })); + + it('Should skip if nothing to migrate', () => + Onyx.multiSet({ + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_1`]: null, + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_2`]: null, + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2`]: {}, + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2_4`]: null, + }) + .then(KeyReportActionsDraftByReportActionID) + .then(() => { + expect(LogSpy).toHaveBeenCalledWith('[Migrate Onyx] Skipped migration KeyReportActionsDraftByReportActionID because there are no actions drafts to migrate'); + const connectionID = Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS, + waitForCollectionCallback: true, + callback: (allReportActions) => { + Onyx.disconnect(connectionID); + const expectedReportActionDraft = {}; + expect(allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_1`]).toBeUndefined(); + expect(allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_2`]).toBeUndefined(); + expect(allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2_4`]).toBeUndefined(); + expect(allReportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}2`]).toMatchObject(expectedReportActionDraft); + }, + }); + })); + }); }); From 2bb124be228a679e7c5102d28db29fbe7ce0910e Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 20 Oct 2023 12:00:34 +0800 Subject: [PATCH 2/8] don't migrate empty draft --- src/libs/migrations/KeyReportActionsDraftByReportActionID.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js index 1a2b81d84891..fae2d1c453f3 100644 --- a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js +++ b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js @@ -26,10 +26,10 @@ export default function () { const newReportActionsDrafts = {}; _.each(allReportActionsDrafts, (reportActionDraft, onyxKey) => { - if (!_.isString(reportActionDraft)) { + newReportActionsDrafts[onyxKey] = null; + if (!_.isString(reportActionDraft) || _.isEmpty(reportActionDraft)) { return; } - newReportActionsDrafts[onyxKey] = null; const reportActionID = onyxKey.split('_').pop(); const newOnyxKey = onyxKey.replace(`_${reportActionID}`, ''); From 229605fd2ffb4b35ff8ae96cc7bd5685ab6c2550 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 20 Oct 2023 12:00:53 +0800 Subject: [PATCH 3/8] clear draft when workspace is deleted --- src/libs/actions/Policy.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js index 89324dd35485..30c124a54fa0 100644 --- a/src/libs/actions/Policy.js +++ b/src/libs/actions/Policy.js @@ -33,6 +33,7 @@ Onyx.connect({ _.each(policyReports, ({reportID}) => { cleanUpMergeQueries[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] = {hasDraft: false}; cleanUpSetQueries[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`] = null; + cleanUpSetQueries[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}`] = null; }); Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, cleanUpMergeQueries); Onyx.multiSet(cleanUpSetQueries); @@ -116,6 +117,12 @@ function deleteWorkspace(policyID, reports, policyName) { }, })), + ..._.map(reports, ({reportID}) => ({ + onyxMethod: Onyx.METHOD.SET, + key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}`, + value: null, + })), + // Add closed actions to all chat reports linked to this policy ..._.map(reports, ({reportID, ownerAccountID}) => { // Announce & admin chats have FAKE owners, but workspace chats w/ users do have owners. From 613e2545de7d04f491c48a805570a0c52bbc51cf Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 20 Oct 2023 12:05:04 +0800 Subject: [PATCH 4/8] Revert "don't migrate empty draft" This reverts commit 2bb124be228a679e7c5102d28db29fbe7ce0910e. --- src/libs/migrations/KeyReportActionsDraftByReportActionID.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js index fae2d1c453f3..1a2b81d84891 100644 --- a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js +++ b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js @@ -26,10 +26,10 @@ export default function () { const newReportActionsDrafts = {}; _.each(allReportActionsDrafts, (reportActionDraft, onyxKey) => { - newReportActionsDrafts[onyxKey] = null; - if (!_.isString(reportActionDraft) || _.isEmpty(reportActionDraft)) { + if (!_.isString(reportActionDraft)) { return; } + newReportActionsDrafts[onyxKey] = null; const reportActionID = onyxKey.split('_').pop(); const newOnyxKey = onyxKey.replace(`_${reportActionID}`, ''); From e9855c7d5e30aea51c4323ea825aa80c7b11604b Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 20 Oct 2023 12:12:03 +0800 Subject: [PATCH 5/8] don't migrate empty draft --- .../KeyReportActionsDraftByReportActionID.js | 4 ++++ tests/unit/MigrationTest.js | 17 +++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js index 1a2b81d84891..401064cc2880 100644 --- a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js +++ b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js @@ -31,6 +31,10 @@ export default function () { } newReportActionsDrafts[onyxKey] = null; + if (_.isEmpty(reportActionDraft)) { + return; + } + const reportActionID = onyxKey.split('_').pop(); const newOnyxKey = onyxKey.replace(`_${reportActionID}`, ''); newReportActionsDrafts[newOnyxKey] = { diff --git a/tests/unit/MigrationTest.js b/tests/unit/MigrationTest.js index 5c048fea5ac7..3139f7efc97f 100644 --- a/tests/unit/MigrationTest.js +++ b/tests/unit/MigrationTest.js @@ -684,5 +684,22 @@ describe('Migrations', () => { }, }); })); + + it("Shouldn't move empty individual draft to a draft collection of report", () => + Onyx.multiSet({ + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_1`]: '', + [`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1`]: {}, + }) + .then(KeyReportActionsDraftByReportActionID) + .then(() => { + const connectionID = Onyx.connect({ + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS, + waitForCollectionCallback: true, + callback: (allReportActionsDrafts) => { + Onyx.disconnect(connectionID); + expect(allReportActionsDrafts[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}1_1`]).toBeUndefined(); + }, + }); + })); }); }); From 2179169d17d91c65c7e8f1397279bf5bd6b2118b Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 25 Oct 2023 02:10:46 +0800 Subject: [PATCH 6/8] add comment --- src/libs/migrations/KeyReportActionsDraftByReportActionID.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js index 401064cc2880..22a2b9196589 100644 --- a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js +++ b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js @@ -38,7 +38,9 @@ export default function () { const reportActionID = onyxKey.split('_').pop(); const newOnyxKey = onyxKey.replace(`_${reportActionID}`, ''); newReportActionsDrafts[newOnyxKey] = { - ...(newReportActionsDrafts[newOnyxKey] || allReportActionsDrafts[newOnyxKey]), + ...(newReportActionsDrafts[newOnyxKey] + // Includes the existing migrated draft + || allReportActionsDrafts[newOnyxKey]), [reportActionID]: reportActionDraft, }; }); From c8286e811af147a2d846c226ad05ff64f8e1920c Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 25 Oct 2023 02:19:22 +0800 Subject: [PATCH 7/8] prettier --- src/libs/migrations/KeyReportActionsDraftByReportActionID.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js index 22a2b9196589..35f0fc4595df 100644 --- a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js +++ b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js @@ -38,9 +38,9 @@ export default function () { const reportActionID = onyxKey.split('_').pop(); const newOnyxKey = onyxKey.replace(`_${reportActionID}`, ''); newReportActionsDrafts[newOnyxKey] = { - ...(newReportActionsDrafts[newOnyxKey] + ...(newReportActionsDrafts[newOnyxKey] || // Includes the existing migrated draft - || allReportActionsDrafts[newOnyxKey]), + allReportActionsDrafts[newOnyxKey]), [reportActionID]: reportActionDraft, }; }); From 9edcd3bc0f16356e24e66df1ef19dc03fd3c6ad2 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 25 Oct 2023 12:23:41 +0800 Subject: [PATCH 8/8] make the code more readable --- .../migrations/KeyReportActionsDraftByReportActionID.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js index 35f0fc4595df..63282b8743dc 100644 --- a/src/libs/migrations/KeyReportActionsDraftByReportActionID.js +++ b/src/libs/migrations/KeyReportActionsDraftByReportActionID.js @@ -37,10 +37,11 @@ export default function () { const reportActionID = onyxKey.split('_').pop(); const newOnyxKey = onyxKey.replace(`_${reportActionID}`, ''); + + // If newReportActionsDrafts[newOnyxKey] isn't set, fall back on the migrated draft if there is one + const currentActionsDrafts = newReportActionsDrafts[newOnyxKey] || allReportActionsDrafts[newOnyxKey]; newReportActionsDrafts[newOnyxKey] = { - ...(newReportActionsDrafts[newOnyxKey] || - // Includes the existing migrated draft - allReportActionsDrafts[newOnyxKey]), + ...currentActionsDrafts, [reportActionID]: reportActionDraft, }; });