-
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
Migrate tasks to use childManagerAccountID
#21781
Conversation
childManagerAccountID
childManagerAccountID
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.
Noticed this needed to be updated, maybe it belongs in a different PR? 🤷
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.
What's this file change for? I don't see any lint issue in latest main
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.
You can see I'm just including extra prop types / default prop types that were missing. We used withCurrentUserPersonalDetails
already, but didn't declare the prop types.
Updated test steps & added screenshots - I thinkkkkk we could get a C+ to help test this, once the web-e PR is on staging :D |
childManagerAccountID
childManagerAccountID
…ChildManagerAccountID
No longer on hold since https://github.com/Expensify/Web-Expensify/pull/37963 is on prod! |
feel free to get a C+ to help review if you'd like! also cc @puneetlath in case you're free to review |
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.
does this instance need to be removed as well?
Line 1263 in 8b7a65d
reportAction.reportAction.childManagerEmail = taskAssignee; |
Thanks @bondydaa , good find 🤦 ready again for review |
asked in contributor plus to get someone to test and do the checklist for us. |
…ChildManagerAccountID
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.
Bug (regression): task assignee not displaying in TaskPreview
Repro step:
Assign task to any random user (not in contact list) in any DM chat
Before:
before.mov
After:
after.mov
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.
What's this file change for? I don't see any lint issue in latest main
@@ -59,7 +65,8 @@ function TaskPreview(props) { | |||
? props.taskReport.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.taskReport.statusNum === CONST.REPORT.STATUS.APPROVED | |||
: props.action.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.action.childStatusNum === CONST.REPORT.STATUS.APPROVED; | |||
const taskTitle = props.taskReport.reportName || props.action.childReportName; | |||
const taskAssignee = props.taskReport.managerEmail || props.action.childManagerEmail; | |||
const taskAssigneeAccountID = TaskUtils.getTaskAssigneeAccountID(props.taskReport); |
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.
props.action
already has childManagerAccountID
. Is there any problem using it directly?
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 don't think so - but since we were defaulting to managerEmail
first, I thought it would be best if we default to managerID
first as well, THEN use childManagerAccountID
- and that logic exists in this function so I thought it would be better to reuse it, do you disagree?
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.
ok agree. But as I reported, we should fallback to original logic if report.managerID or action.childManagerAccountID doesn't exist.
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.
"original logic" being props.taskReport.managerEmail || props.action.childManagerEmail;
? We're trying to get rid of those :D
Checking that regression you mentioned @0xmiroslav 👍 |
Hm yeah that's interesting. We're going to migrate mentions to be accountID-based but it's just not quite done yet. So whatever we do for now will be temporary. I think we can just do whatever is easiest. |
Mmmm nevermind I think it makes the most sense to keep defaulting to email ( |
@0xmiroslav ready for re-review please 🙏 |
What happens if both don't exist? as we faced the crash yesterday? |
@0xmiroslav if neither exist then The crash you mentioned was what, the |
correct. Just to confirm that this is temporary and won't be considered as regression. As production app shows email correctly.
correct. It's not exact same case but similar. I just provided example. |
Correct, it's not a regression, it's more of a "new status quo" - we expect you to not have the user's
Ok cool, that issue was due to not testing after merging & there being a bug introduced - this is a different situation. If it tests well now (including the new expected state mentioned above), we're good to go |
Oh I meant this crash - #21874 (comment) |
Aah right that one!! This PR should default to |
Reviewer Checklist
Screenshots/VideosMobile Web - ChromeMobile Web - SafariDesktopScreen.Recording.2023-07-06.at.4.01.28.PM.moviOSAndroid |
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!
@yuwenmemon 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] |
@@ -65,7 +66,7 @@ function TaskPreview(props) { | |||
: props.action.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.action.childStatusNum === CONST.REPORT.STATUS.APPROVED; | |||
const taskTitle = props.taskReport.reportName || props.action.childReportName; | |||
const taskAssigneeAccountID = TaskUtils.getTaskAssigneeAccountID(props.taskReport); | |||
const taskAssignee = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], ''); | |||
const taskAssignee = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'displayName'], '')); |
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.
personally not crazy about this nested lodash, would rather see something like:
const taskAssignee = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'displayName'], '')); | |
const assigneeLogin = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'login'], ''); | |
const assigneeDisplayName = lodashGet(props.personalDetailsList, [taskAssigneeAccountID, 'displayName'], ''); | |
const taskAssignee = assigneeLogin || assigneeDisplayName; |
as it's more clear to me that displayName
is a fall back if we don't have a login. instead of trying to reason how the 2nd lodashGet might get called and be a fallback for the first call.
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.
Ooh that's totally fair - I'll try to remember to clean this up later - i can see it being useful but not highest priority at the moment
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.
Cleanup PR here: #22439
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 from me but if you want to clean it up then bonus points 😂
Going to go ahead and merge given the urgency and since Alex is off for the day. Feel free to handle that in follow-up if you're so-inclined @Beamanator! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.38-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.38-7 🚀
|
@@ -59,7 +65,8 @@ function TaskPreview(props) { | |||
? props.taskReport.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.taskReport.statusNum === CONST.REPORT.STATUS.APPROVED | |||
: props.action.childStateNum === CONST.REPORT.STATE_NUM.SUBMITTED && props.action.childStatusNum === CONST.REPORT.STATUS.APPROVED; | |||
const taskTitle = props.taskReport.reportName || props.action.childReportName; | |||
const taskAssignee = props.taskReport.managerEmail || props.action.childManagerEmail; | |||
const taskAssigneeAccountID = TaskUtils.getTaskAssigneeAccountID(props.taskReport); |
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 didn't consider the case where the taskReport
could be empty and hence it caused a regression here, because the task assignment didn't show an email address.
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 if task report could be empty, would this have crashed before too? 🤔 (since we used to directly access props.taskReport.managerEmail
)
currently holding on https://github.com/Expensify/Web-Expensify/pull/37963Details
Just starting to use the new accountID property instead of email, in order to limit the emails we're passing around the app. This has been available for a few weeks but forgot to migrate this.
Fixed Issues
$ Part of #19007 (wasn't noticed before)
Tests
Tests related to #19990:
Tests for #19089
Tests for #19959
Tests for #19967
Make sure this still works (from this recent PR):
Offline tests
Same as above
QA Steps
Same as above
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