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

test: test_classic_mailing_list: Check contact auth and display names #5294

Closed
wants to merge 1 commit into from

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Feb 28, 2024

No description provided.

@@ -601,7 +601,12 @@ pub async fn from_field_to_contact_id(
prevent_rename: bool,
) -> Result<Option<(ContactId, bool, Origin)>> {
let display_name = if prevent_rename {
Some("")
Some(
match Contact::lookup_id_by_addr(context, &from.addr, Origin::Unknown).await? {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not very good is that this adds a new db query, another option is to add a new Origin variant right before IncomingUnknownFrom so that the fallback display name doesn't overwrite the existing one

@iequidoo iequidoo marked this pull request as ready for review February 28, 2024 04:39
@adbenitez
Copy link
Member

adbenitez commented Feb 28, 2024

If we cannot use the display name from the "From" header for a new contact, e.g. for mailing list messages, use the "From" address, it's better than an empty string.

What solves this? 🤔 The situation is already that the from-address is displayed in the reactions details as fallback if contact doesn't have display name, the problem is that these addresses are random hashes for anonymous mailing lists, or even in normal mailing lists, the address is not enough, the display name is needed for more context, the issue is that the display name is not being saved if the contact/ sender of the message sent the message in a discussion mailing list

This used to work in the past, but was changed because for single-address ML like GitHub use a single/same address for all messages changing only the display name and then when going to the "notifications@github.com" profile it would have the last display name from the last user that commented in some issue you are subscribed to

@iequidoo iequidoo marked this pull request as draft February 28, 2024 16:35
@iequidoo
Copy link
Collaborator Author

The situation is already that the from-address is displayed in the reactions details as fallback if contact doesn't have display name

Indeed, i didn't notice that Contact::get_display_name() already returns &self.addr as a fallback. So, this PR doesn't actually change anything.

But do we have an idea how to tell single-address MLs from separate-address ones? Using smth like "Mailinglist User" isn't much better than the current situation and also needs translation.

@iequidoo
Copy link
Collaborator Author

@adbenitez Another option is to save the display name from the "From" header, but if it changes, create a new contact every time. But then we have other problems:

  • Actually the address can't be used to communicate to a contact, but there will be no warning, the contact isn't hidden etc.
  • If an ML user actually changes their name, the old contact will remain too.

@adbenitez
Copy link
Member

Using smth like "Mailinglist User" isn't much better than the current situation and also needs translation.

In fact it is actually worse

But do we have an idea how to tell single-address MLs from separate-address ones?

Maybe it is not that bad the tradeoffs after all and we should set the name for all

@iequidoo
Copy link
Collaborator Author

iequidoo commented Feb 28, 2024

Maybe it is not that bad the tradeoffs after all and we should set the name for all

Maybe still create new contacts for new display names in "From"? It doesn't look worse than a constantly changing display name for a contact. People don't change their display names very often i guess and also we can implement a garbage collection later. The only thing is that these contacts mustn't be shown in the list when you're going to create a new chat and in similar places. CC @r10s

@r10s
Copy link
Member

r10s commented Feb 29, 2024

not sure, that would mean address is no longer unique, one can easily imagine issues wrt keys and whatnot.

the effort would be mainly for showing "override sender name" as display name in eg. reaction list of some mailinglists. this is already a rare case, risking far lager and much worse issues, and throwing many resources at that seems not wise to me.

wont-fix is an option as well.

so, i would not change the whole contact concept because of this, at least not now and not only for that comparable weak reason

Using smth like "Mailinglist User" isn't much better than the current situation and also needs translation.

In fact it is actually worse

not sure, if it is worse. the random adresses are meaningless to the user as well, but additionally look more like a bug or as if one could make sense of it

also, that kind of underlines the anonymous feature of some mailing lists :)

anyways, as this PR does not change much, i think we should close this PR and go back to the original issue #5184, that has more context and is more on point for discussions

@iequidoo iequidoo force-pushed the iequidoo/fallback-display-name branch from dae55ee to e208a70 Compare February 29, 2024 17:25
@iequidoo iequidoo changed the title feat: Use "From" address instead of "" as a fallback contact display name (#5184) test: test_classic_mailing_list: Check contact auth and display names Feb 29, 2024
@iequidoo
Copy link
Collaborator Author

iequidoo commented Feb 29, 2024

So, i only left changes to the test, so that it also checks contact auth name.

EDIT: I'm wrong, auth name must be empty. Ok, let's close this PR

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

Successfully merging this pull request may close these issues.

None yet

3 participants