-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
* - report_ | ||
* - ownerEmail -> ownerAccountID | ||
* - managerEmail -> managerID | ||
* - lastActorEmail -> lastActorAccountID | ||
* - participants -> participantAccountIDs |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
Co-authored-by: Abdelhafidh Belalia <16493223+s77rt@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 💪
Will this be off hold soon? 😄 Also just a heads up there's conflicts! |
Soonish hopefully, still working through migrating these emails 😅 thanks for the heads up about conflicts! |
I guess we can close this since some pieces have already been merged, and managerEmail / ownerEmail pieces are being handled in #31664 |
Details
Now that the back-end is no longer sending email-based data to reports, we update the migration to remove any email-based data that is found. We aren't removing entire reports from onyx, just the email-based data that is found.
Fixed Issues
$ #21454
Tests
The automated tests should mostly cover this since it's an Onyx migration. However you can also test the various cases manually by:
Onyx.set('report_123', {ownerEmail: 'hi', ownerAccountID: 1000});
{ownerAccountID: 1000}
Onyx.set('report_123', {ownerEmail: 'hi'});
Offline tests
Same tests work online or offline.
QA Steps
There is nothing in particular to QA for this PR as there are no user-facing changes. Regular regression testing that the app is working as normal should be enough.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android