-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Continue personalDetails
-> personalDetailsList
migration
#20328
Continue personalDetails
-> personalDetailsList
migration
#20328
Conversation
@madmax330 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Lmk when this is ready, let's also get a C+ on the review since it's a large PR. |
Yo @madmax330 - I thinkkkk we don't need a C+ to test this specific one since we're merging into a different branch. I'm planning to get a lot of changes into the branch |
Tests fixed for "beaman-continueMigration" branch
fix: fixing failing tests for `OptionsListUtils` and `ReportUtils` suites
…nto beaman-continueMigration
#20371 got merged into this one & I added a few commits, but there's a "GH incident" affecting PRs, so not sure if we'll have to do some cleanup eventually 🙃 |
Policy members keyed by accountID
…App into beaman-continueMigration
…nto beaman-continueMigration
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
QA: Number(lodashGet(Config, 'EXPENSIFY_ACCOUNT_ID_QA', 3126513)), | ||
QA_TRAVIS: Number(lodashGet(Config, 'EXPENSIFY_ACCOUNT_ID_QA_TRAVIS', 8595733)), | ||
RECEIPTS: Number(lodashGet(Config, 'EXPENSIFY_ACCOUNT_ID_RECEIPTS', -1)), | ||
REWARDS: Number(lodashGet(Config, 'EXPENSIFY_ACCOUNT_ID_REWARDS', 11023767)), // [email protected] |
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.
Not sure if we need this? I think yes, but it's not in CONST.EMAIL
at the moment 🤷
@@ -114,6 +111,7 @@ export default { | |||
DOWNLOAD: 'download_', | |||
POLICY: 'policy_', | |||
POLICY_MEMBER_LIST: 'policyMemberList_', |
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.
I wonder if we can remove this now?
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.
I'm not quite done migrating to the new key so IMO we should leave it until that's done.
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.
Giggidy 👍
@puneetlath @neil-marcellini do y'all want to do a mini review before we merge this? |
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.
I'm blindly approving so we can start doing bug fixes on the main feature branch.
Reviewer ChecklistN/A but here it is
Screenshots/VideosN/A WebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.29-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.29-11 🚀
|
* @returns {String} | ||
*/ | ||
function getDefaultAvatar(login = '') { | ||
if (!login) { | ||
function getDefaultAvatar(accountID = -1) { |
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.
Getting default avatar based on accountID introduced #22085 since accountID always changes for new users.
#22659 issue was caused by this PR. 'Hidden' was not translated to Spanish language. We should have used translation key instead of constant for 'Hidden'. |
const sessionAccountID = lodashGet(props.session, 'accountID', null); | ||
const managerID = props.iouReport.managerID || ''; | ||
const ownerAccountID = props.iouReport.ownerAccountID || ''; | ||
const participantAccountIDs = props.isBillSplit ? lodashGet(props.action, 'originalMessage.participantAccountIDs', []) : [managerID, ownerAccountID]; |
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 should have sorted the account ids. Not doing so resulted in #31732
* @returns {String} | ||
*/ | ||
function getDefaultAvatar(login = '') { | ||
if (!login) { | ||
function getDefaultAvatar(accountID = -1) { |
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.
Switching to using the accountID
to get a default avatar caused #28518 as we use an optimistic accountID when chatting with new users.
Details
Removing old key so that we're FORCED to start fixing issues as they come up. It's fine to break this branch, we'll come back to fix everything once we're much closer to the migration being complete
Fixed Issues
$ Related to #19007
Tests
N/A - This is definitely going to break things, we'll fix them as they come up
Offline tests
N/A - This is definitely going to break things, we'll fix them as they come up
QA Steps
N/A - This is definitely going to break things, we'll fix them as they come up
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