Skip to content
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

fix: always show email address for suggestions from address collector #10683

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Feb 12, 2025

Have oc_mail_coll_addresses with:

user_id email display_name
admin [email protected] Bob
admin [email protected] Bob

B

Screenshot From 2025-02-12 12-43-43

A

image

@kesselb kesselb requested review from st3iny and GretaD February 12, 2025 11:47
@kesselb kesselb self-assigned this Feb 12, 2025
@kesselb kesselb added this to the v4.2.0 milestone Feb 12, 2025
@kesselb
Copy link
Contributor Author

kesselb commented Feb 12, 2025

/backport to stable4.2

@kesselb
Copy link
Contributor Author

kesselb commented Feb 12, 2025

/backport to stable4.1

@kesselb
Copy link
Contributor Author

kesselb commented Feb 12, 2025

/backport to stable3.7

@@ -37,9 +37,13 @@ public function findMatches(string $userId, string $term): array {

// Convert collected addresses into same format as CI creates
$recipientsFromCollector = array_map(static function (CollectedAddress $address) {
$label = ($address->getDisplayName())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to explicitly check for null or an empty string? what makes a truthy displayname?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, now it's really matching the implemention in contacts integration ;)

@kesselb kesselb force-pushed the bug/noid/always-show-email-address-for-recipients branch 2 times, most recently from ace208e to e77b98b Compare February 12, 2025 13:26
@ChristophWurst
Copy link
Member

Doesn't that mean we'll also send the email to a recipient Jane Doe ([email protected]) <[email protected]> instead of just Jane Doe <[email protected]>?

@kesselb
Copy link
Contributor Author

kesselb commented Feb 12, 2025

Doesn't that mean we'll also send the email to a recipient Jane Doe ([email protected]) [email protected] instead of just Jane Doe [email protected]?

Indeed, that's not ideal.

I initially did it in the frontend until I learned that we are using this format for the contacts' integration already. Let me move it back to the frontend and overload the options template for nc/vue select.

@kesselb
Copy link
Contributor Author

kesselb commented Feb 12, 2025

Good that we took a second look. It's already there, just using the wrong prop, apparently.

<ListItemIcon :no-margin="true"
:name="option.label"
:subtitle="option.email"
:icon-class="!option.id ? 'icon-user' : null"
:url="option.photo"
:avatar-size="24" />

subtitle => subname

@kesselb kesselb force-pushed the bug/noid/always-show-email-address-for-recipients branch from e77b98b to 0b50b3a Compare February 12, 2025 14:37
@kesselb
Copy link
Contributor Author

kesselb commented Feb 12, 2025

I've updated the patch to show the email address as subname again if label != email.

  1. It was not enough to just rename subtitle to subname because the subname is only shown when the avatar size is larger than 24. It's using the default of 32 now. I don't know if that plays well with the new smaller design. @nimishavijay @GretaD could you please check the screenshot's in the first post?

  2. @ChristophWurst the issue, that the sender name is weird, is still there for recipients from the contacts integration service. Please let me know if you want to address this in this pr or do a follow-up. The purpose for this change was to improve the usability for the recipients from the address collector.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I like the sublinename approach

It's fine to do the follow up. This can be put into a ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants