-
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
Remove all uses of actorEmail
as part of personalDetails migration
#21874
Changes from 11 commits
2162be6
c3ff82f
e60c318
f7d6e79
a4ac931
d11464d
36c3879
2c7b104
312b4ad
bba1d48
d1b3e65
0a557bc
3ec735c
50c362a
9e0cabf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,7 +67,6 @@ export default () => ({ | |
text: '123 Ios', | ||
}, | ||
], | ||
actorEmail: '[email protected]', | ||
actorAccountID: 10773236, | ||
message: [ | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,7 +74,7 @@ function ReportActionItemSingle(props) { | |
const actorAccountID = props.action.actorAccountID; | ||
let {displayName} = props.personalDetailsList[actorAccountID] || {}; | ||
const {avatar, pendingFields} = props.personalDetailsList[actorAccountID] || {}; | ||
let actorHint = lodashGet(props.action, 'actorEmail', '').replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, ''); | ||
let actorHint = displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we get email here from personal details list and if email is not present then fallback to login? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Juuuust to be clear @MonilBhavsar do you mean, we should check the If that's what you meant, then yes I agree :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes correct! Exactly what I mean :D
Yes, in case display name is set it won't be login. And in that line we specifically want to process login(email). So we can first check if login exists, if not we can fallback to displayName. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated!
Technically no, not always - for example in |
||
const isWorkspaceActor = ReportUtils.isPolicyExpenseChat(props.report) && !actorAccountID; | ||
let avatarSource = UserUtils.getAvatar(avatar, actorAccountID); | ||
|
||
|
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.
NAB, I think we should still keep it as
reopenedTaskReportAction.actorEmail
In case we change
ReportUtils::buildOptimisticTaskReportAction()
in future andreopenedTaskReportAction
get's updated, this direct reference might introduce some bugs.Same below
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.
Hmm I'm keen to completely get rid of
actorEmail
AND we're getting rid oflastActorEmail
in this PR: #22008So I think we'll keep it like this for now 🙏