Update LHN/chat header to handle displayName not being set #27547
Conversation
melvin-bot
bot
requested review from
mananjadhav
and removed request for
a team
|
@mananjadhav 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] |
|
@mananjadhav hi! Do you have an ETA for reviewing? |
|
Will be reviewing this today within 2-3 hours. |
|
@puneetlath @lukemorawski The Steps to reproduce:
|
|
Ah interesting. Yeah on the search page, let's just show the email address as the display name like we do today. Would you be able to make that update @lukemorawski? |
|
@lukemorawski think you'll be able to make that update today? |
|
@puneetlath yep! |
|
@puneetlath Added some changes. Please check :) |
|
Thanks! @mananjadhav mind giving it a quick look first? |
|
@lukemorawski Now for the valid ones too, we're seeing a blank.
web-hidden.mov@puneetlath I think we should be able to differentiate between the new chat or existing hidden records. Can't backend send some flag from the backend, where we want to show Hidden? |
It's not really so simple. The back-end will send the data that the user should have access to. Which sometimes will be just accountID. Sometimes will be accountID and displayName. Sometimes will be accountID, displayName, and login. Can we not do this based on what information we have locally? |
|
@lukemorawski what are your thoughts? Would love to get this wrapped up this week! |
|
@puneetlath Sorry, had some more urgent tickets. Will check this out today! |
|
@puneetlath @mananjadhav tricky thing, but there might be a way to deal with that locally |
|
@puneetlath @mananjadhav OK, made some fixes. Looks good for me, but please do check it out! |
|
@mananjadhav thoughts on the approach? |
src/libs/ReportUtils.js
Outdated
| /** | ||
| * Get the displayName for a single report participant. | ||
| * | ||
| * @param {Number} accountID | ||
| * @param {Boolean} [shouldUseShortForm] | ||
| * @param {Boolean} shouldNotFallbackToHidden |
There was a problem hiding this comment.
Prefer using no negated condition https://eslint.org/docs/latest/rules/no-negated-condition
We have this as an eslint rule, but it didn't catch this because it's a function param.
| * @param {Boolean} shouldNotFallbackToHidden | |
| * @param {Boolean} shouldFallbackToHidden |
There was a problem hiding this comment.
Agreed with @rushatgabhane here.
There was a problem hiding this comment.
ok, though it made more sense like that, as falling back to hidden is the default :)
src/libs/ReportUtils.js
Outdated
| if (!accountID) { | ||
| return ''; | ||
| } | ||
| const personalDetails = getPersonalDetailsForAccountID(accountID); | ||
| // check if it's invite account | ||
| if (hasOnlyAvatarField(personalDetails)) { | ||
| return ''; |
There was a problem hiding this comment.
question: could you please help me understand this. In which cases a user will have only Avatar field?
There was a problem hiding this comment.
Sure! Say a user is entering an email address into the search field, and that address has not yet been registered as Expensify user, it will, in that scenario have no other details than automatically created avatar. This is how I detect a "new invite account", because the can't fallback to "Hidden" displayName value.
There was a problem hiding this comment.
Thanks for the explanation @lukemorawski. Let's add a comment as a JSDoc for the method.
src/libs/ReportUtils.js
Outdated
| @@ -1089,32 +1089,46 @@ function getPersonalDetailsForAccountID(accountID) { | |||
| ); | |||
| } | |||
|
|
|||
| function hasOnlyAvatarField(obj) { | |||
| const keys = _.keys(obj); | |||
| return keys.length === 1 && keys[0] === 'avatar'; | |||
There was a problem hiding this comment.
should we use lodash get to access keys[0]?
There was a problem hiding this comment.
not sure why. This is safe and very readable. Lodash would be something like this
_.isEqual(_.keys(obj), ['avatar']);
There was a problem hiding this comment.
What happens to this personalDetails object post-invite once the API responds. Does it get cleared out? If so, perhaps we can add an explicit isOptimistc key to it instead of having to infer it.
There was a problem hiding this comment.
Not sure what you mean by cleared out? But when we search for a non-existing account we see this.
Once we receive the object again post-invite, it includes all the keys. For example,
{
"accountID": 15820244,
"avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_4.png",
"displayName": "[email protected]",
"firstName": "",
"lastName": "",
"status": null
}
There was a problem hiding this comment.
Sorry, I think I'm confused by what an "invite" account is.
From what I can see, when I invite a new user, we create an optimistic personalDetails object for that user that looks like this:
Then when the API responds, the optimistic personalDetail gets removed and the real personalDetail with the real accountID for that user is added:
So I'm just trying to understand what this "invite" account that has only the avatar field is and when it would exist. Sorry if I'm missing something obvious!
src/libs/ReportUtils.js
Outdated
| @@ -1089,32 +1089,46 @@ function getPersonalDetailsForAccountID(accountID) { | |||
| ); | |||
| } | |||
|
|
|||
| function hasOnlyAvatarField(obj) { | |||
There was a problem hiding this comment.
Can you explain how this method helps us?
There was a problem hiding this comment.
Let's add a comment as a JSDoc for the method.
src/libs/ReportUtils.js
Outdated
| @@ -1089,32 +1089,46 @@ function getPersonalDetailsForAccountID(accountID) { | |||
| ); | |||
| } | |||
|
|
|||
| function hasOnlyAvatarField(obj) { | |||
There was a problem hiding this comment.
Can we rename obj to something more meaningful?
|
@lukemorawski quick bump on the changes requested. |
There was a problem hiding this comment.
Looks good to me!
One small comment and @mananjadhav it'd be great to do a quick re-test.
|
I'll retest this by eod. |
|
@lukemorawski Thanks for the changes. @puneetlath I retested this and it looks to be working as expected |
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOSNAAndroidNA |
|
Great, good with me too! @lukemorawski looks like you have a Jest test failing though. Not sure if that is because of your changes or if you just need to merge main. Once you fix that, we should be good to merge. |
|
Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
This PR caused serious regression - Not able to submit distance request at all. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.88-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.88-11 🚀
|
| _.map(props.iou.participants, (participant) => { | ||
| const isPolicyExpenseChat = lodashGet(participant, 'isPolicyExpenseChat', false); | ||
| return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, props.personalDetails); | ||
| }), | ||
| _.chain(props.iou.participants) | ||
| .map((participant) => { | ||
| const isPolicyExpenseChat = lodashGet(participant, 'isPolicyExpenseChat', false); | ||
| return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, props.personalDetails); | ||
| }) | ||
| .filter((participant) => !!participant.login) | ||
| .value(), |
There was a problem hiding this comment.
@lukemorawski Can you please clarify why we need to filter participants on login? cc @mananjadhav
(Coming from #31792)
There was a problem hiding this comment.
@s77rt You probably meant filtering "by login". It' there to filter out user without login so the would not appear in the money request as "Hidden". Also the process of creating a money request requires that field and allowing "Hidden" users to make a money request caused an error.
There was a problem hiding this comment.
@lukemorawski Thanks for your reply. It looks like making money requests to "Hidden" accounts works and it's actually a wanted functionality. Since this filter was only done to avoid an error that no longer occurs I think it's safe to remove it. Let us know if disagree.
Details
Fixed Issues
$ #27393
PROPOSAL: No proposal
Tests
Offline tests
QA Steps
personalDetailsListkey, find the object that represents personal details of a contact you want to make hiddenPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android