Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix report action draft still present after deleting the workspace #30049

Merged
merged 8 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions src/libs/actions/Policy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -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});
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/libs/migrateOnyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ 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();
Log.info('[Migrate Onyx] start');

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.
Expand Down
57 changes: 57 additions & 0 deletions src/libs/migrations/KeyReportActionsDraftByReportActionID.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
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;
}
Comment on lines +29 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Is there any non-string case?

Copy link
Contributor Author

@bernhardoj bernhardoj Oct 23, 2023

Choose a reason for hiding this comment

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

One case is when the action draft is already migrated to an object, so we skip it.

I don't know if it is possible that a user have both old and new structure of action draft, but just to be safe.

newReportActionsDrafts[onyxKey] = null;

if (_.isEmpty(reportActionDraft)) {
return;
}

const reportActionID = onyxKey.split('_').pop();
const newOnyxKey = onyxKey.replace(`_${reportActionID}`, '');
newReportActionsDrafts[newOnyxKey] = {
...(newReportActionsDrafts[newOnyxKey] || allReportActionsDrafts[newOnyxKey]),
Copy link
Contributor

Choose a reason for hiding this comment

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

What does || allReportActionsDrafts[newOnyxKey] do in here? do we ever set allReportActionsDrafts[newOnyxKey]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to cover a case where there is already an existing migrated draft of newOnyxKey.

the example is on the 2nd unit test

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment to make that obvious please? I see the test, and with one of the values for reportID 2 set to {} and one to null, but is that something that can happen, like having a migration run half-way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment.

but is that something that can happen, like having a migration run half-way?

I'm not sure actually, but I see the other existing tests and they include that kind of case.

[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);
},
});
});
}
4 changes: 2 additions & 2 deletions src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/ReportActionsDrafts.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
type ReportActionsDrafts = Record<string, string>;

export default ReportActionsDrafts;
2 changes: 2 additions & 0 deletions src/types/onyx/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -89,6 +90,7 @@ export type {
ReportMetadata,
ReportAction,
ReportActions,
ReportActionsDrafts,
ReportActionReactions,
SecurityGroup,
Transaction,
Expand Down
80 changes: 80 additions & 0 deletions tests/unit/MigrationTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -622,4 +623,83 @@ 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);
},
});
}));

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();
},
});
}));
});
});
Loading