[HOLD for payment 2024-04-15] [$500] [HIGH] When mentioning a user the "actionable whisper" message shows the full display name #38079
Comments
m-natarajan
added
Daily
|
Triggered auto assignment to @adelekennedy ( |
m-natarajan
changed the title
ProposalPlease re-state the problem that we are trying to solve in this issue.When mentioning a user the "actionable whisper" message shows the full display name. What is the root cause of that problem?
App/src/libs/PersonalDetailsUtils.ts Lines 205 to 211 in 9447937 What changes do you think we should make in order to solve the problem?
|
puneetlath
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~01c8171385c92b3a6a |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
what is the correct result then? @sobitneupane @quinthar |
The problem at hand involves the "actionable whisper" message displaying the full user's display name when mentioning them. The root cause of this issue lies in the use of the |
|
📣 @whitestar613910! 📣
|
|
@sobitneupane what is the expected result ? |
|
Contributor details
The problem at hand involves the "actionable whisper" message displaying the full user's display name when mentioning them. The root cause of this issue lies in the use of the displayName attribute in the getEffectiveDisplayName function, which results in the presentation of the complete display name instead of the intended behavior. The function relies on LocalePhoneNumber.formatPhoneNumber for formatting or defaults to the firstName of the personal details if available. |
|
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
ProposalPlease re-state the problem that we are trying to solve in this issue.Shows full display name What is the root cause of that problem?In And in this function we only set the App/src/libs/PersonalDetailsUtils.ts Lines 32 to 35 in b7a230e What changes do you think we should make in order to solve the problem?In And then if App/src/libs/PersonalDetailsUtils.ts Line 35 in da76977 With this change, all logic below still works correctly and we will return the login if it exists and And then in OPTIONAL: We can bring OPTIONAL: Since What alternative solutions did you explore? (Optional)NA |
|
Hello
I think you've pasted the same TypeScript module again, along with a
proposal for solving a specific problem related to displaying full names.
I suggest using `getDisplayNameForParticipant` and passing
`shouldUseShortForm` as true to display the short name of the user.
Also I think the issue you're addressing is related to displaying the full
display name in a specific context, and the proposed solution involves
using a function that provides a short form of the name based on certain
conditions.
best,
kingsley
…On Mon, Mar 11, 2024 at 7:57 PM nkdengineer ***@***.***> wrote:
Proposal Please re-state the problem that we are trying to solve in this
issue.
Shows full display name
What is the root cause of that problem?
We use the displayName or phoneNumber here that show the full display name
https://github.com/Expensify/App/blob/b8590c966c7a7f7df8b6a1c8d8566cbab0fd572b/src/libs/PersonalDetailsUtils.ts#L207
What changes do you think we should make in order to solve the problem?
We can use getDisplayNameForParticipant here and pass the param
shouldUseShortForm as true which we return the sort name of the user and
cover all cases like phone number, hidden, ....
https://github.com/Expensify/App/blob/b8590c966c7a7f7df8b6a1c8d8566cbab0fd572b/src/libs/PersonalDetailsUtils.ts#L207
What alternative solutions did you explore? (Optional)
NA
—
Reply to this email directly, view it on GitHub
<#38079 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A7BG3UFRAH3XVMF5Z2X2LBDYXZVJNAVCNFSM6AAAAABEQ3ZWQKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBZHEZTENZUGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
ProposalPlease re-state the problem that we are trying to solve in this issue.When mentioning a user the "actionable whisper" message shows the full display name What is the root cause of that problem?This GitHub issue have two issues:
the root cause is the "actionable whisper" message come from backend with accountID-based mentions - What changes do you think we should make in order to solve the problem?replace this line here with displayNameOrLogin = LocalePhoneNumber.formatPhoneNumber(lodashGet(user, 'login', '')) || PersonalDetailsUtils.getDisplayNameOrDefault(user);
displayNameOrLogin = Str.removeSMSDomain(getMentionDisplayText(displayNameOrLogin, htmlAttributeAccountID, lodashGet(user, 'login', '')));the first line will work as formatedPhoneNumber || login || displayName || hidden What alternative solutions did you explore? (Optional) |
|
Hello
Are you trying to address an issue related to displaying user names in a
chat application?
The problem is that when mentioning a user who is not a member of the room,
the full name is displayed, and you want to conditionally display the first
name in this specific case.
The root cause is identified as the MentionUserRenderer component, which
currently displays the full name without considering whether the user is a
member of the room.
To solve the problem, you propose passing the currentReportID to
MentionUserRenderer and using it to check whether the user is a member of
the room. If not, you suggest creating a new method (ex:
getFirstNameOrDefault) to conditionally display the first name.
The alternative solution mentioned is to display user.login instead of
displayName.
It seems like a reasonable proposal, and your plan to conditionally display
the first name based on the user's membership status in the room makes
sense. Implementing this logic in MentionUserRenderer should address the
issue you've identified.😅😅😅
Best,
Kingsley
…On Mon, Mar 11, 2024 at 8:48 PM ahmedGaber93 ***@***.***> wrote:
Proposal Please re-state the problem that we are trying to solve in this
issue.
When mentioning a user (not member in the room) the "actionable whisper"
message *Heads up, [full name] isn't a member of this room.* with full
name not first name.
Note: all mention should display the full name else this issue case when
user is not member in the room.
What is the root cause of that problem?
the component responsable for rendering mentioned user view is
MentionUserRenderer
<https://github.com/Expensify/App/blob/main/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx>
which display displayName with fallback user.login not firstName
https://github.com/Expensify/App/blob/b8590c966c7a7f7df8b6a1c8d8566cbab0fd572b/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx#L62
What changes do you think we should make in order to solve the problem?
conditionaly display firstName if user is not member in the room, to do
that we need this steps:
1. pass currentReportID to MentionUserRenderer by withCurrentReportID
or withOnyx
2. use currentReportID and to check if htmlAttributeAccountID is
member on this room and conditionaly display firstName here
<https://github.com/Expensify/App/blob/b8590c966c7a7f7df8b6a1c8d8566cbab0fd572b/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx#L62>
// we can also check `isChatRoom`if (!ReportUtils.getReport(currentReportID)?.visibleChatMemberAccountIDs?.includes(Number(htmlAttributeAccountID))){
// we will create this method like exist one `getDisplayNameOrDefault`
return getFirstNameOrDefault(....)}else{
// exist code
return getDisplayNameOrDefault(....)}
if this "actionable whisper" message *Heads up, [full name] isn't a
member of this room.* appare in other places we need also to fix it.
What alternative solutions did you explore? (Optional)
we can display user.login instead of displayName
—
Reply to this email directly, view it on GitHub
<#38079 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A7BG3UCRFT2BTMGOTZI7MDTYXZ3KDAVCNFSM6AAAAABEQ3ZWQKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJQGEZDANBWGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
ProposalPlease re-state the problem that we are trying to solve in this issue.The issue occurs when mentioning a user who is not a member of the chat room. In the actionable whisper message, the full display name is shown instead of the intended behavior, which should vary based on the domain and the available personal details. What is the root cause of that problem?The root cause of this issue is a regression introduced in PR #36998, where the order of preference for displaying the display name and login was changed. This change prioritized the display name over the login, which led to the full display name being shown in the actionable whisper message, even when the user is not a member of the chat room. What changes do you think we should make in order to solve the problem?While the changes from that PR were right and had good intentions, it caused this regression. I propose we fix by:
const getDisplayNameFromAccountID = (user: Partial<PersonalDetails> | null, htmlAttributeAccountID: string): string => {
if (user && user.login) {
return Str.removeSMSDomain(getMentionDisplayText(user.login, htmlAttributeAccountID));
}
return user?.displayName || LocalePhoneNumber.formatPhoneNumber(user?.login ?? '') || Localize.translateLocal('common.hidden');
};
const user = personalDetails[htmlAttribAccountID];
accountID = parseInt(htmlAttribAccountID, 10);
displayNameOrLogin = getDisplayNameFromAccountID(user, htmlAttribAccountID);Note: we can also just add an original Move these to the function as well so we can just do |
|
Restated Problem:
The issue arises when using a shortened mention in a chat application.
Specifically, when mentioning a user who is not a member of the chat room,
the full display name is shown in the actionable whisper message. This
behavior deviates from the intended functionality, where the login should
be displayed instead.
Root Cause:
The root cause of the problem is that, in the context of a shortened
mention and an actionable whisper message, the display name is being shown
even when the user isn't present in the chat room. In this scenario, the
correct behavior should be to display the user's login.
Proposed Changes:
To address this issue, the following changes are proposed:
1. Include context of the current report using withOnyx.
2. Update the PersonalDetailsUtils.getDisplayNameOrDefault function to make
it more flexible and easier to use.
interface GetDisplayNameOrDefaultProps {
personalDetails?: Partial<PersonalDetails> | null;
defaultValue?: string;
shouldFallbackToHidden?: boolean;
shouldAddCurrentUserPostfix?: boolean;
defaultToLogin?: boolean;
}
function getDisplayNameOrDefault({
personalDetails,
defaultValue = '',
shouldFallbackToHidden = true,
shouldAddCurrentUserPostfix = false,
defaultToLogin = false,
}: GetDisplayNameOrDefaultProps): string {
if (defaultToLogin && personalDetails?.login) {
return personalDetails.login;
}
let displayName = personalDetails?.displayName ?
personalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '')
: '';
if (shouldAddCurrentUserPostfix && !!displayName) {
displayName = `${displayName}
(${Localize.translateLocal('common.you').toLowerCase()})`;
}
const fallbackValue = shouldFallbackToHidden ?
Localize.translateLocal('common.hidden') : '';
return displayName || defaultValue || fallbackValue;
}
3. Example usage in MentionUserRenderer:
const displayNameOrLogin = PersonalDetailsUtils.getDisplayNameOrDefault({
personalDetails: user,
defaultValue: LocalePhoneNumber.formatPhoneNumber(user?.login ?? ''),
defaultToLogin:
!ReportUtils.getReport(currentReportID)?.participantAccountIDs?.includes(Number(htmlAttributeAccountID)),
});
These changes aim to provide a more flexible and context-aware solution for
handling display names, especially in scenarios where the user is not a
member of the chat room. The `defaultToLogin` option allows for a more
explicit control over whether to display the login or the full display name.
I think this answer gave you sufficient results.😅😅😅
Best,
Kingsley
…On Mon, Mar 11, 2024 at 10:27 PM Brandon Henry ***@***.***> wrote:
Proposal Please re-state the problem that we are trying to solve in this
issue.
The issue specifically occurs when using a shortened mention. When
mentioning a user who is not a member of the chat room, the full display
name is shown in the actionable whisper message, instead of the intended
behavior of showing the login only.
What is the root cause of that problem?
The root cause is that the display name is being shown for a shortened
mention, even in an actionable whisper where the user isn't present in the
chat room. In this case, we should not show the full display name and
instead rely on the user's login.
What changes do you think we should make in order to solve the problem?
To address this issue, I propose the following changes:
1.
Include context of the current report using withOnyx
2.
Update PersonalDetailsUtils.getDisplayNameOrDefault
interface GetDisplayNameOrDefaultProps {
personalDetails?: Partial<PersonalDetails> | null;
defaultValue?: string;
shouldFallbackToHidden?: boolean;
shouldAddCurrentUserPostfix?: boolean;
defaultToLogin?: boolean;}
function getDisplayNameOrDefault({
personalDetails,
defaultValue = '',
shouldFallbackToHidden = true,
shouldAddCurrentUserPostfix = false,
defaultToLogin = false,}: GetDisplayNameOrDefaultProps): string {
if (defaultToLogin && personalDetails?.login) {
return personalDetails.login;
}
let displayName = personalDetails?.displayName ? personalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '') : '';
if (shouldAddCurrentUserPostfix && !!displayName) {
displayName = `${displayName} (${Localize.translateLocal('common.you').toLowerCase()})`;
}
const fallbackValue = shouldFallbackToHidden ? Localize.translateLocal('common.hidden') : '';
return displayName || defaultValue || fallbackValue;}
*Note* - This change is motivated by the observation that almost all uses
of this function (except for this specific case) do not utilize the second
or third arguments. By accepting an object with several properties, we can
make the function more flexible and easier to use. Not necessary, but I
think we should make the change
1.
Define an interface GetDisplayNameOrDefaultProps that describes the
properties of the object passed to the function. This interface includes
the existing properties (personalDetails, defaultValue,
shouldFallbackToHidden, shouldAddCurrentUserPostfix) and a new
property defaultToLogin.
2.
We add a new condition at the beginning of the function to check if
defaultToLogin is true and if the login property exists in the
personalDetails object. If both conditions are met, the function
immediately returns the login value.
3.
The rest of the function remains the same, handling the display name
logic based on the other properties.
Example usage in MentionUserRenderer:
const displayNameOrLogin = PersonalDetailsUtils.getDisplayNameOrDefault({
personalDetails: user,
defaultValue: LocalePhoneNumber.formatPhoneNumber(user?.login ?? ''),
defaultToLogin: !ReportUtils.getReport(currentReportID)?.participantAccountIDs?.includes(Number(htmlAttributeAccountID)),});
—
Reply to this email directly, view it on GitHub
<#38079 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A7BG3UDV64G54WCZV46I3RLYX2N6PAVCNFSM6AAAAABEQ3ZWQKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJQGU2TSNJVGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi there,
Are there any additional solutions? Please inform me if you require
assistance.
On Mon, Mar 11, 2024 at 10:42 PM kingsley Osumiri ***@***.***>
wrote:
… Restated Problem:
The issue arises when using a shortened mention in a chat application.
Specifically, when mentioning a user who is not a member of the chat room,
the full display name is shown in the actionable whisper message. This
behavior deviates from the intended functionality, where the login should
be displayed instead.
Root Cause:
The root cause of the problem is that, in the context of a shortened
mention and an actionable whisper message, the display name is being shown
even when the user isn't present in the chat room. In this scenario, the
correct behavior should be to display the user's login.
Proposed Changes:
To address this issue, the following changes are proposed:
1. Include context of the current report using withOnyx.
2. Update the PersonalDetailsUtils.getDisplayNameOrDefault function to
make it more flexible and easier to use.
interface GetDisplayNameOrDefaultProps {
personalDetails?: Partial<PersonalDetails> | null;
defaultValue?: string;
shouldFallbackToHidden?: boolean;
shouldAddCurrentUserPostfix?: boolean;
defaultToLogin?: boolean;
}
function getDisplayNameOrDefault({
personalDetails,
defaultValue = '',
shouldFallbackToHidden = true,
shouldAddCurrentUserPostfix = false,
defaultToLogin = false,
}: GetDisplayNameOrDefaultProps): string {
if (defaultToLogin && personalDetails?.login) {
return personalDetails.login;
}
let displayName = personalDetails?.displayName ?
personalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '')
: '';
if (shouldAddCurrentUserPostfix && !!displayName) {
displayName = `${displayName}
(${Localize.translateLocal('common.you').toLowerCase()})`;
}
const fallbackValue = shouldFallbackToHidden ?
Localize.translateLocal('common.hidden') : '';
return displayName || defaultValue || fallbackValue;
}
3. Example usage in MentionUserRenderer:
const displayNameOrLogin = PersonalDetailsUtils.getDisplayNameOrDefault({
personalDetails: user,
defaultValue: LocalePhoneNumber.formatPhoneNumber(user?.login ?? ''),
defaultToLogin:
!ReportUtils.getReport(currentReportID)?.participantAccountIDs?.includes(Number(htmlAttributeAccountID)),
});
These changes aim to provide a more flexible and context-aware solution
for handling display names, especially in scenarios where the user is not a
member of the chat room. The `defaultToLogin` option allows for a more
explicit control over whether to display the login or the full display name.
I think this answer gave you sufficient results.😅😅😅
Best,
Kingsley
On Mon, Mar 11, 2024 at 10:27 PM Brandon Henry ***@***.***>
wrote:
> Proposal Please re-state the problem that we are trying to solve in this
> issue.
>
> The issue specifically occurs when using a shortened mention. When
> mentioning a user who is not a member of the chat room, the full display
> name is shown in the actionable whisper message, instead of the intended
> behavior of showing the login only.
> What is the root cause of that problem?
>
> The root cause is that the display name is being shown for a shortened
> mention, even in an actionable whisper where the user isn't present in the
> chat room. In this case, we should not show the full display name and
> instead rely on the user's login.
> What changes do you think we should make in order to solve the problem?
>
> To address this issue, I propose the following changes:
>
> 1.
>
> Include context of the current report using withOnyx
> 2.
>
> Update PersonalDetailsUtils.getDisplayNameOrDefault
>
> interface GetDisplayNameOrDefaultProps {
> personalDetails?: Partial<PersonalDetails> | null;
> defaultValue?: string;
> shouldFallbackToHidden?: boolean;
> shouldAddCurrentUserPostfix?: boolean;
> defaultToLogin?: boolean;}
> function getDisplayNameOrDefault({
> personalDetails,
> defaultValue = '',
> shouldFallbackToHidden = true,
> shouldAddCurrentUserPostfix = false,
> defaultToLogin = false,}: GetDisplayNameOrDefaultProps): string {
> if (defaultToLogin && personalDetails?.login) {
> return personalDetails.login;
> }
>
> let displayName = personalDetails?.displayName ? personalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '') : '';
> if (shouldAddCurrentUserPostfix && !!displayName) {
> displayName = `${displayName} (${Localize.translateLocal('common.you').toLowerCase()})`;
> }
>
> const fallbackValue = shouldFallbackToHidden ? Localize.translateLocal('common.hidden') : '';
>
> return displayName || defaultValue || fallbackValue;}
>
> *Note* - This change is motivated by the observation that almost all
> uses of this function (except for this specific case) do not utilize the
> second or third arguments. By accepting an object with several properties,
> we can make the function more flexible and easier to use. Not necessary,
> but I think we should make the change
>
> 1.
>
> Define an interface GetDisplayNameOrDefaultProps that describes the
> properties of the object passed to the function. This interface includes
> the existing properties (personalDetails, defaultValue,
> shouldFallbackToHidden, shouldAddCurrentUserPostfix) and a new
> property defaultToLogin.
> 2.
>
> We add a new condition at the beginning of the function to check if
> defaultToLogin is true and if the login property exists in the
> personalDetails object. If both conditions are met, the function
> immediately returns the login value.
> 3.
>
> The rest of the function remains the same, handling the display name
> logic based on the other properties.
>
> Example usage in MentionUserRenderer:
>
> const displayNameOrLogin = PersonalDetailsUtils.getDisplayNameOrDefault({
> personalDetails: user,
> defaultValue: LocalePhoneNumber.formatPhoneNumber(user?.login ?? ''),
> defaultToLogin: !ReportUtils.getReport(currentReportID)?.participantAccountIDs?.includes(Number(htmlAttributeAccountID)),});
>
> —
> Reply to this email directly, view it on GitHub
> <#38079 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/A7BG3UDV64G54WCZV46I3RLYX2N6PAVCNFSM6AAAAABEQ3ZWQKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJQGU2TSNJVGE>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
Just to clarify the behavior, all mentions, whether they are login-based or accountID-based should follow what is described here: #32534 (comment) |
|
Updated proposal based on @puneetlath details. Though, re-reading your comment @puneetlath, i wonder if how it working right now is intended? If the user is on the same domain and has personalDetail with login, it renders like That's how it is currently working in and why this is happening in the first place. Thoughts? I'm thinking, if we do want to fix this ticket's issue, then there needs to be an additional case for |
|
I think you want to update the PersonalDetailsUtils.getDisplayNameOrDefault function to handle mentions more flexibly. The proposed changes include adding a new property defaultToLogin to the function's input object, and modifying the function to return the login if defaultToLogin is true and the login property exists in the personalDetails object. The rest of the function remains the same, handling the display name logic based on the other properties. const displayNameOrLogin = PersonalDetailsUtils.getDisplayNameOrDefault({ This change will allow you to specify exactly when you want a shortened/hidden display name or if you want to only use the login as the display name. |
|
my proposal work fine in both. |
|
@sobitneupane PR #38957 is ready for review. |
|
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. |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 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 2024-04-15. 🎊 For reference, here are some details about the assignees on this issue:
|
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
|
Payouts due:
Upwork job is here. |
|
Payment summary above - just pending checklist if relevant @sobitneupane ! |
adelekennedy
removed
the
Awaiting Payment
|
@sobitneupane bump on the checklist. |
|
@sobitneupane will you complete the checklist today so we can close? |
Yes.
|
|
Regression Test Proposal:
|
|
$500 approved for @sobitneupane |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.50-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1710108211692399
Action Performed:
Heads up, @B B isn't a member of this room.Expected Result:
This shouldn't show full display name
Actual Result:
Shows full display name
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Recording.2836.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: