[HOLD for payment 2024-02-14] [HOLD for payment 2024-01-18] Consolidate GetDislplayName methods

[puneetlath]

We have a bunch of different methods that are used to get the display name:

• PersonalDetails.getDisplayNameForTypingIndicator
• PersonalDetails.getDisplayName
• ReportUtils.getDisplayNamesStringFromTooltips
• ReportUtils.getDisplayNameForParticipant
• ReportUtils.getDisplayNamesWithTooltips
• PersonalDetailsUtils.getDisplayNameOrDefault

I would like to do two things:

1. Conslidate these as much as possible. They probably can't all be consolidated, but I would think at least PersonalDetailsUtils.getDisplayNameOrDefault and PersonalDetails.getDisplayName and ReportUtils.getDisplayNameForParticipant could be
2. Make sure that we are falling back on the login if we have it. Right now ReportUtils.getDisplayNameForParticipant doesn't fall back on the login, even if it is available, and I would like to update it to do so

[melvin-bot]

Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new.

[koko57]

ready to work on this issue

[koko57]

@puneetlath I've analyzed the methods you mentioned and looking for another parts of code that have to do something with creating a displayName. For now leaving my findings for each method:

• PersonalDetails.getDisplayNameForTypingIndicator
  only used in:

  App/src/pages/home/report/ReportTypingIndicator.js

  Lines 45 to 46 in 47c7819

  	<TextWithEllipsis
  	leadingText={PersonalDetails.getDisplayNameForTypingIndicator(usersTyping[0], props.translate('common.someone'))}

  - we can try to use getDisplayName instead
• PersonalDetails.getDisplayName
  it could be possibly merged with getDisplayNameOrDefault and getDisplayNameForParticipant
• ReportUtils.getDisplayNamesStringFromTooltips
  it's only used here:

  App/src/libs/GroupChatUtils.ts

  Lines 16 to 22 in 47c7819

  	function getGroupChatName(report: Report): string {
  	const participants = report.participantAccountIDs ?? [];
  	const isMultipleParticipantReport = participants.length > 1;
  	const participantPersonalDetails = OptionsListUtils.getPersonalDetailsForAccountIDs(participants, allPersonalDetails ?? {});
  	const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(participantPersonalDetails, isMultipleParticipantReport);
  	return ReportUtils.getDisplayNamesStringFromTooltips(displayNamesWithTooltips);
  	}

  
  and it uses getDisplayNamesWithTooltips. So we're creating a list of objects just to extract a display name from them 🤔. I think this logic should be simplified not to iterate over a list twice
• ReportUtils.getDisplayNameForParticipant
  maybe we could replace this method, getDisplayName and getDisplayNameOrDefault and create one that will be used instead of these 3
• ReportUtils.getDisplayNamesWithTooltips
  iterates over personalDetailsList array, uses getDisplayNameForParticipant
• PersonalDetailsUtils.getDisplayNameOrDefault
  same as in getDisplayNameForParticipant

[koko57]

for your PR - I think we can merge it and then I will work on mine. I would rather split the whole refactor into several smaller PRs. It would be easier to test for regressions.

[puneetlath]

Ok sounds good. I'll get it ready for review.

[puneetlath]

@koko57 my PR was merged.

[situchan]

If dedicated C+ is needed to review all sub-PRs for this issue, I am happy to help.
@puneetlath If you agree, you can assign me on this issue.

[koko57]

@puneetlath I've opened the first draft PR. I removed the getDisplayNameForTypingIndicator method. I actually wonder if we need to check if the we have user's login or ID in reportUserIsTyping object. I tried to create a new user in OldDot and then was typing a message in NewDot it looked like we have the ID. But maybe it's for the old users? So leaving this logic now, let me know if it's still necessary.

Another thing: while working with ReportTypingIndicator (the only component that used that removed method) I noticed that we don't really need this other component

App/src/pages/home/report/ReportTypingIndicator.js

Lines 46 to 52 in 8c9925d

	<TextWithEllipsis
	leadingText={PersonalDetails.getDisplayNameForTypingIndicator(usersTyping[0], props.translate('common.someone'))}
	trailingText={` ${props.translate('reportTypingIndicator.isTyping')}`}
	textStyle={[styles.chatItemComposeSecondaryRowSubText]}
	wrapperStyle={[styles.chatItemComposeSecondaryRow, styles.flex1]}
	leadingTextParentStyle={styles.chatItemComposeSecondaryRowOffset}
	/>

- we could use the same approach as in this other case:

App/src/pages/home/report/ReportTypingIndicator.js

Lines 57 to 63 in 8c9925d

	<Text
	style={[styles.chatItemComposeSecondaryRowSubText, styles.chatItemComposeSecondaryRowOffset]}
	numberOfLines={1}
	>
	{props.translate('reportTypingIndicator.multipleUsers')}
	{` ${props.translate('reportTypingIndicator.areTyping')}`}
	</Text>

and it actually looks the same (screenshots - I changed for the red color the implementation with Text instead of TextWithEllipsis)

I'm trying to find the reason why now we use two different components for displaying this text. Maybe I'm missing something? If it would be ok to use the Text only for both cases, we could remove TextWithEllipsis (it's only used in ReportTypingIndicator). What do you think? 🙂

[koko57]

@puneetlath and regarding the issue and the next steps:

• what do you think about moving getDisplayName and extractFirstAndLastNameFromAvailableDetails and getCountryISO to PersonalDetailsUtils? They are now in actions/PersonalDetails. Should we leave in the actions only the functions that write to API?
• same with getDisplayNameForParticipant and other methods that deal only with allPersonalDetails object - should we move them to PersonalDetailsUtils?
• for now, I think that we can leave only getDisplayNameForParticipant and getDisplayNamesWithTooltips methods. The latter one iterates over an array uses getDisplayNameForParticipant and it's used in several places. I would remove getDisplayNamesStringFromTooltips

[puneetlath]

If dedicated C+ is needed to review all sub-PRs for this issue, I am happy to help.

Sounds good.

what do you think about moving getDisplayName and extractFirstAndLastNameFromAvailableDetails and getCountryISO to PersonalDetailsUtils? They are now in actions/PersonalDetails. Should we leave in the actions only the functions that write to API?

Yes, I agree with this. Do you @situchan?

same with getDisplayNameForParticipant and other methods that deal only with allPersonalDetails object - should we move them to PersonalDetailsUtils?

This makes sense to me also.

for now, I think that we can leave only getDisplayNameForParticipant and getDisplayNamesWithTooltips methods. The latter one iterates over an array uses getDisplayNameForParticipant and it's used in several places. I would remove getDisplayNamesStringFromTooltips

Great!

[puneetlath]

I actually wonder if we need to check if the we have user's login or ID in reportUserIsTyping object. I tried to create a new user in OldDot and then was typing a message in NewDot it looked like we have the ID. But maybe it's for the old users? So leaving this logic now, let me know if it's still necessary.

Don't we fall back on "someone is typing..." if we don't have the info?

Another thing: while working with ReportTypingIndicator (the only component that used that removed method) I noticed that we don't really need this other component

Nice find, I think consolidation makes sense if it isn't needed. But maybe @situchan can take a deeper look and weigh in.

[koko57]

I actually wonder if we need to check if the we have user's login or ID in reportUserIsTyping object. I tried to create a new user in OldDot and then was typing a message in NewDot it looked like we have the ID. But maybe it's for the old users? So leaving this logic now, let me know if it's still necessary.

Don't we fall back on "someone is typing..." if we don't have the info?

Yes, we do, but I meant checking if the key is a user ID or email login. For newly created account in the OD we do have an ID for the user, but maybe for the old accounts we only have an email. But I think we can revisit this logic later on.

For the removing TextWithEllipsis - as you asked @situchan - I'm waiting for the feedback before I will refactor this one.

For moving the methods - I would do this in a separate PR - a PR only for moving these methods.

[situchan]

I'm trying to find the reason why now we use two different components for displaying this text. Maybe I'm missing something? If it would be ok to use the Text only for both cases, we could remove TextWithEllipsis (it's only used in ReportTypingIndicator). What do you think? 🙂

It's intentional. If display name is very long, it should be truncated at name, not on typing...

Original: Very long display name is typing...
Bad: Very long display name is ... (using Text)
Good: Very long display ... is typing (using TextWithEllipsis)

But this isn't necessary when multiple users are typing because "Multiple users are typing..." text is short enough to display even in small screen width.

[koko57]

@situchan great, thanks a lot for the explanation! So leaving it as it is.

[situchan]

• what do you think about moving getDisplayName and extractFirstAndLastNameFromAvailableDetails and getCountryISO to PersonalDetailsUtils? They are now in actions/PersonalDetails. Should we leave in the actions only the functions that write to API?
• same with getDisplayNameForParticipant and other methods that deal only with allPersonalDetails object - should we move them to PersonalDetailsUtils?

Agree as long as it doesn't cause cyclic dependencies issue

• for now, I think that we can leave only getDisplayNameForParticipant and getDisplayNamesWithTooltips methods. The latter one iterates over an array uses getDisplayNameForParticipant and it's used in several places. I would remove getDisplayNamesStringFromTooltips

👍

[koko57]

PR opened for review #31495

[koko57]

opened a draft PR for moving the methods: #34657
I will update the test steps and the checklist shortly

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @koko57 is from an agency-contributor and not due payment
• ROLE: @situchan paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@puneetlath)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[situchan]

Sounds good to me too. Thanks

[koko57]

Great! Thanks! So the first part is opened: #34657

[puneetlath]

@situchan how's this going?

[situchan]

review is in progress. will complete today

[puneetlath]

What's left here?

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.37-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• refactor: move methods to personalDetailsUtils, remove unused one #34657

If no regressions arise, payment will be issued on 2024-02-14. 🎊

For reference, here are some details about the assignees on this issue:

• @koko57 does not require payment (Contractor)
• @situchan requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@situchan] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[koko57]

@puneetlath I suggested to merge getDisplayNameForParticipant with getDisplayNameOrDefault. It's a big change, I'm still looking if there won't be any edge cases and we can use only one method everywhere.

[puneetlath]

Ok, sounds good. Thanks for keeping me in the loop.

[koko57]

@puneetlath After analyzing the usages of both methods I'm not convinced that we should go with merging them - they are slightly different and I would need to introduce an additional layer of complexity to cover all the cases. For now, I think that both methods serve their purpose in the places they are used and are easy to use.
If you agree, we can close this issue.

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @koko57 is from an agency-contributor and not due payment
• ROLE: @situchan paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@puneetlath)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[puneetlath]

Ok sounds good to me. @situchan do you agree?

Thanks for all your effort on this @koko57!

[situchan]

Agree. Thanks

[puneetlath]

Ok sounds good. @situchan I see two "payment summary" posts in the issue. Is that correct? I feel like there were more PRs than that 😅

[situchan]

• fix: remove getDisplayNameForTypingIndicator #31495
• Refactor/31312 consolidate getdisplayname methods pt2 #33930
• refactor: move methods to personalDetailsUtils, remove unused one #34657

[puneetlath]

Great, thanks. Offer sent: https://www.upwork.com/nx/wm/offer/100915132

Please ping me here when you've accepted.

[situchan]

Accepted thanks

[puneetlath]

Paid. Thanks y'all!