-
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
[HOLD for payment 2023-11-13] [HOLD for payment 2023-10-30] [$500] Update LHN/chat header to handle displayName not being set #27393
Comments
Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Update LHN/chat header to handle displayName not being set What is the root cause of that problem?We're still returning empty string incase we don't have the value for shortName and longName. Lines 1099 to 1107 in d68263a
What changes do you think we should make in order to solve the problem?We can add more condition if both of these values are empty, we should use if (_.isEmpty(longName) && _.isEmpty(shortName)) {
return personalDetails.login || Localize.translate("common.hidden");
}
return shouldUseShortForm ? shortName : longName; What alternative solutions did you explore? (Optional)N/A |
@puneetlath It's Lucas from Callstack. Happy to work on that. Just wanted to clarify one thing. By fallback to "Hidden" you mean displaying a text "Hidden" in the header and LHN? |
Yes, exactly. Let's have the text "Hidden" be the final fallback text whenever we use display names and aren't able to find either a display name or a login. |
@puneetlath How can I create an account with no display name, to test that change? |
We chatted in Slack, but this will only be testable by adding fake data in Onyx or removing data from an existing user. |
This issue was reported by @parasharrajat before @puneetlath created this issue. I've added the Slack thread where the issue was reported so that @parasharrajat can be paid out the reporting bonus. |
This issue has not been updated in over 15 days. @puneetlath, @lukemorawski eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
@puneetlath, @lukemorawski Whoops! This issue is 2 days overdue. Let's get this updated quick! |
This is missing an external label. |
Does that also handle adding back the previous changes that were reverted? |
Ah, dang it, they reverted themselves after I resolved a conflict against "main" branch! Such a noob thing to do 🤦♂️ Give me a moment while I'll bring 'em back. |
Haha all good! Git is fun 😅 |
@puneetlath updated the latest PR with all the changes! |
Great! |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.95-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-11-13. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($500) |
📣 @allroundexperts Please request via NewDot manual requests for the Contributor role ($500) |
Note to self: both @mananjadhav and @allroundexperts were involved in PR review here. Will need to figure out compensation. |
Payment Summary:
Please both request on NewDot. |
$500 payment approved for @mananjadhav based on summary above. |
$500 payment approved for @allroundexperts based on summary above. |
Looks like, we missed the reporting bonus for me here. @puneetlath Could you please add that to the payment summary? Thanks. |
Whoops, sorry about that. Updated @parasharrajat |
Payment requested as per #27393 (comment) |
$50 payment approved for @parasharrajat based on this comment. |
We are going to stop sending a display name from the back-end in certain scenarios. In those scenarios, we should fall back on using "Hidden" in the LHN and Chat Header (and anywhere else that relies on display name being set).
Let's update any place that expects a display name to fall back on the primary login. And if there is neither a displayName or a primary login, to fall back on "Hidden".
Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694550580877519
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: