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

[HOLD E/E#294647] Update PersonalDetails migration of reports to remove email-based data #22811

Closed
wants to merge 8 commits into from
66 changes: 64 additions & 2 deletions src/libs/migrations/PersonalDetailsByAccountID.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,22 @@
});
}

/**
* @returns {Promise<Object>}
*/
function getReportsFromOnyx() {
return new Promise((resolve) => {
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (allReports) => {
Onyx.disconnect(connectionID);
return resolve(allReports);
},
});
});
}

/**
* We use the old personalDetails object becuase it is more efficient for this migration since it is keyed by email address.
* Also, if the old personalDetails object is not available, that likely means the migration has already run successfully before on this account.
Expand Down Expand Up @@ -71,12 +87,17 @@
* - whisperedTo -> whisperedToAccountIDs
* - childOldestFourEmails -> childOldestFourAccountIDs
* - originalMessage.participants -> originalMessage.participantAccountIDs
* - report_
* - ownerEmail -> ownerAccountID
* - managerEmail -> managerID
* - lastActorEmail -> lastActorAccountID
* - participants -> participantAccountIDs
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not doing such migration. We are just removing the email data and hoping the server would return the the IDs version.

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic here is slightly different than report actions. In reports we are blindly removing email data i.e. if a report has email data but not ids data, we will only remove the email data. The report data is now useless (no email data nor ids data). Should we delete the whole report in this case (similarly how it's done with report actions)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we delete the whole report in this case (similarly how it's done with report actions)?

We thought about this and decided not to, because then we'd also have to delete anything related to that report (ex: report actions) so we thought it would just be better to kill the keys we're no longer using

*
* @returns {Promise<void>}
*/
export default function () {
return Promise.all([getReportActionsFromOnyx(), getDeprecatedPersonalDetailsFromOnyx(), getDeprecatedPolicyMemberListFromOnyx()]).then(
([oldReportActions, oldPersonalDetails, oldPolicyMemberList]) => {
return Promise.all([getReportActionsFromOnyx(), getReportsFromOnyx(), getDeprecatedPersonalDetailsFromOnyx(), getDeprecatedPolicyMemberListFromOnyx()]).then(
([oldReportActions, oldReports, oldPersonalDetails, oldPolicyMemberList]) => {
const onyxData = {};

// The policyMemberList_ collection has been replaced by policyMembers_
Expand Down Expand Up @@ -225,6 +246,47 @@
}
});

// For the reports migration, we don't need to look up emails from accountIDs. Instead,
// we will just look for old email data and automatically remove it if it exists. The reason for
// this is that we already stopped sending email based data in reports, and from everywhere else
// in the app by this point (since this is the last data we migrated).
_.each(oldReports, (report, onyxKey) => {
// Delete report key if it's empty
if (_.isEmpty(report)) {
onyxData[onyxKey] = null;
return;
}

let reportWasModified = false;
if (lodashHas(report, ['ownerEmail'])) {
reportWasModified = true;
Log.info(`[Migrate Onyx] PersonalDetailsByAccountID migration: removing ownerEmail from report ${report.reportID}`);
delete report.ownerEmail;

Check failure on line 264 in src/libs/migrations/PersonalDetailsByAccountID.js

View workflow job for this annotation

GitHub Actions / lint

Assignment to property of function parameter 'report'
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
}

if (lodashHas(report, ['managerEmail'])) {
reportWasModified = true;
Log.info(`[Migrate Onyx] PersonalDetailsByAccountID migration: removing managerEmail from report ${report.reportID}`);
delete report.managerEmail;

Check failure on line 270 in src/libs/migrations/PersonalDetailsByAccountID.js

View workflow job for this annotation

GitHub Actions / lint

Assignment to property of function parameter 'report'
}

if (lodashHas(report, ['lastActorEmail'])) {
reportWasModified = true;
Log.info(`[Migrate Onyx] PersonalDetailsByAccountID migration: removing lastActorEmail from report ${report.reportID}`);
delete report.lastActorEmail;

Check failure on line 276 in src/libs/migrations/PersonalDetailsByAccountID.js

View workflow job for this annotation

GitHub Actions / lint

Assignment to property of function parameter 'report'
}

if (lodashHas(report, ['participants'])) {
reportWasModified = true;
Log.info(`[Migrate Onyx] PersonalDetailsByAccountID migration: removing participants from report ${report.reportID}`);
delete report.participants;

Check failure on line 282 in src/libs/migrations/PersonalDetailsByAccountID.js

View workflow job for this annotation

GitHub Actions / lint

Assignment to property of function parameter 'report'
}

if (reportWasModified) {
onyxData[onyxKey] = report;
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
}
})

// The personalDetails object has been replaced by personalDetailsList
// So if we find an instance of personalDetails we will clear it out
if (oldPersonalDetails) {
Expand Down
100 changes: 100 additions & 0 deletions tests/unit/MigrationTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -870,5 +870,105 @@ describe('Migrations', () => {
},
});
}));

it('Should remove any instances of ownerEmail found in a report', () =>
Onyx.multiSet({
[`${ONYXKEYS.REPORT}1`]: {
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
reportID: 1,
ownerEmail: 'fake@test.com',
ownerAccountID: 5,
}
})
.then(PersonalDetailsByAccountID)
.then(() => {
expect(LogSpy).toHaveBeenCalledWith('[Migrate Onyx] PersonalDetailsByAccountID migration: removing ownerEmail from report 1');
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (allReports) => {
Onyx.disconnect(connectionID);
const expectedReport = {
reportID: 1,
ownerAccountID: 5,
};
expect(allReports[`${ONYXKEYS.COLLECTION.REPORT}1`][1]).toMatchObject(expectedReport);
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
},
});
}));

it('Should remove any instances of managerEmail found in a report', () =>
Onyx.multiSet({
[`${ONYXKEYS.REPORT}1`]: {
reportID: 1,
managerEmail: 'fake@test.com',
managerID: 5,
}
})
.then(PersonalDetailsByAccountID)
.then(() => {
expect(LogSpy).toHaveBeenCalledWith('[Migrate Onyx] PersonalDetailsByAccountID migration: removing managerEmail from report 1');
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (allReports) => {
Onyx.disconnect(connectionID);
const expectedReport = {
reportID: 1,
managerID: 5,
};
expect(allReports[`${ONYXKEYS.COLLECTION.REPORT}1`][1]).toMatchObject(expectedReport);
},
});
}));

it('Should remove any instances of lastActorEmail found in a report', () =>
Onyx.multiSet({
[`${ONYXKEYS.REPORT}1`]: {
reportID: 1,
lastActorEmail: 'fake@test.com',
lastActorAccountID: 5,
}
})
.then(PersonalDetailsByAccountID)
.then(() => {
expect(LogSpy).toHaveBeenCalledWith('[Migrate Onyx] PersonalDetailsByAccountID migration: removing lastActorEmail from report 1');
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (allReports) => {
Onyx.disconnect(connectionID);
const expectedReport = {
reportID: 1,
lastActorAccountID: 5,
};
expect(allReports[`${ONYXKEYS.COLLECTION.REPORT}1`][1]).toMatchObject(expectedReport);
},
});
}));

it('Should remove any instances of participants found in a report', () =>
Onyx.multiSet({
[`${ONYXKEYS.REPORT}1`]: {
reportID: 1,
participants: ['fake@test.com'],
participantAccountIDs: 5,
}
})
.then(PersonalDetailsByAccountID)
.then(() => {
expect(LogSpy).toHaveBeenCalledWith('[Migrate Onyx] PersonalDetailsByAccountID migration: removing participants from report 1');
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (allReports) => {
Onyx.disconnect(connectionID);
const expectedReport = {
reportID: 1,
participantAccountIDs: 5,
};
expect(allReports[`${ONYXKEYS.COLLECTION.REPORT}1`][1]).toMatchObject(expectedReport);
},
});
}));
});
});
Loading