deltachat / deltachat-core-rust

Delta Chat Rust Core library, used by Android/iOS/desktop apps, bindings and bots 📧
https://delta.chat/en/contribute
Other
656 stars 83 forks source link

no contact names for contact-IDs used in mailinglists' reaction details #5184

Open r10s opened 7 months ago

r10s commented 7 months ago

the reaction details in mailinglists seems not to use getSenderName() as in similar places (group bubbles etc.); i've seen this on the screen of @hpk42

@adbenitez i do not have and android with such mailinglists, can you confirm and fix?

adbenitez commented 7 months ago

this is a core issue, you can't use "getSenderName" since the reactions API what gives you is contact IDs and that contacts don't have the proper display name since in mailing lists these display names are treated as overriden display names (~) so if you open the profile of any user in the maling list you will see that the name is not displayed in the profile either, because core has no saved the display name for that contacts, so it is not affecting only reactions but profiles, search results etc.

r10s commented 7 months ago

i see. getSenderName() requires sender-message-information which we do not have for mailinglists

r10s commented 7 months ago

do we know internally if a given contact-ID was created for a mailing lists and the "real" name is available only when using getSenderName() for an explicit message?

if so, we could alter getDisplayname() so that it returns just "Mailinglist User" in that case, this seems to be good enough and might avoid larger refactoring when many ppl potentially share the same address and we cannot attach a name there

iequidoo commented 7 months ago

I see that in receive_imf.rs mailing list contacts are created just with Origin::Hidden:

let (contact_id, _) = Contact::add_or_lookup(context, "", &list_post, Origin::Hidden).await?;

But when you call Contact::get_all_blocked() the first time they turn into Origin::MailinglistAddress. It's a question for me what happens if another message is received then because it triggers Contact::add_or_lookup(... Origin::Hidden) again. I think a Rust test should be written for this (at least to record the current behaviour). Anyway we can determine if a contact was created for a mailing list, but it's a bit complicated now (see Contact::update_blocked_mailinglist_contacts() for the reference), maybe we should add a db migration instead marking such contacts somehow.

Also it's a question how to make this "Mailinglist User" displayname translatable.

iequidoo commented 6 months ago

I was looking at the wrong place, that is the code creating a contact for a mailing list itself, not for mailing list users.

do we know internally if a given contact-ID was created for a mailing lists and the "real" name is available only when using getSenderName() for an explicit message?

So, for already existing contacts we don't know that. But we can create new contacts using "Mailinglist User" as a display name, but i'd suggest to use the From address instead, this way display names would differ and also we don't need translation for this string. I made a draft PR, if you want, you can try it out.

iequidoo commented 6 months ago

There's also some discussion about creating a new contact if a display name changes in the "From" header: https://github.com/deltachat/deltachat-core-rust/pull/5294#issuecomment-1969492736

iequidoo commented 5 months ago

So, for now we don't have any idea how to differ single-address mailing lists from separate-addresses ones which is the source of the problem. We can name all ML users "Mailinglist User", but i'd suggest some emoji like 👤 to avoid translation.

r10s commented 5 months ago

translation is not an issue, we do that for far less visible aspects

adbenitez commented 5 months ago

I still don't understand why we keep considering "mailing list user" as a possible solution, it is better to leave it without any name then and at least have the address as a way to differentiate users, otherwise a bunch of "mailing list user" in the reactions overview is WAY worse

iequidoo commented 5 months ago

@adbenitez But the address is displayed right under the username, checked on Android and Desktop, and i guess it's so everywhere, not only in the reactions overview.

But we can also extend the reactions API to store and return usernames to truly solve the problem. Looks like we even don't need to deprecate the current API, only add smth like dc_msg_get_override_sender_name().

iequidoo commented 5 months ago

Checked the code, now the overriden name isn't saved to the db at all for trashed messages (what reactions are), see receive_imf::add_parts():

                    if trash {
                        "".to_string()
                    } else {
                        param.to_string()                                                                                                                                                                            
                    },

So, the username can't even be restored. Not sure how useful this information is in the single-address MLs, but maybe it's not so bad to save it for reactions.

adbenitez commented 5 months ago

@adbenitez But the address is displayed right under the username, checked on Android and Desktop, and i guess it's so everywhere, not only in the reactions overview.

Good point, but even then the "mailing list user" thing is pointless and misleading ex. for GitHub it would be weird to see "mailing list user" as display name because it is not exactly a discussion mailing list, I guess this could also be the case for other mailing lists, putting a dummy display name is not useful IMHO

iequidoo commented 5 months ago

Btw, currently reactions are broken at all for single-address MLs because all users are mapped to a signle contact and reactions from the most recent user override reactions from the previous one. Apparently we indeed need separate contacts for users having different display names. There is no limitation currently on the number of contacts having the same address, but of course such a change should be made carefully.

r10s commented 5 months ago

if all users use the same address, assigning reactions by name only, seems weak and error prone as well (can one really reply to these ml in general?)

compared to other issues, it seems questionable if single-user ml as github are worth the effort of large contact-/peer-handling refactoring/logic-changing and risking by far more worse bugs

adbenitez commented 5 months ago

I don't think the single address mailing list is a common scenario, but saving reaction per overridden sender name in fact would be a great improvement since it would also allow bridge bots to properly map reactions from other platforms, ex. from matrix or telegram, to delta chat, currently it is not possible to properly bridge reactions because of this, oth only using overridden sender as id for each reaction sounds a bit of a hack

iequidoo commented 5 months ago

I don't think the single address mailing list is a common scenario, but saving reaction per overridden sender name in fact would be a great improvement since it would also allow bridge bots to properly map reactions from other platforms, ex. from matrix or telegram, to delta chat, currently it is not possible to properly bridge reactions because of this, oth only using overridden sender as id for each reaction sounds a bit of a hack

By default we shouldn't use the overriden sender name as an id (or to create separate contacts for different names), but only in some special cases. So, we need some flag that a message is bridged. Or maybe better that a contact (mailing list or bot) is a "collector" for users on some other service. I don't think it's a very bad hack, it's responsibility of that bot to assign unique names to users, otherwise it would be impossible to differ them anyway. But of course better to add some message header to identify users.

gerryfrancis commented 3 months ago

@link2xt Do you think a similar fix like https://github.com/deltachat/deltachat-core-rust/pull/5375 could be applied here as well?

iequidoo commented 3 months ago

@link2xt Do you think a similar fix like #5375 could be applied here as well?

The problem is that for reactions overriden sender names aren't even saved to the db. But the bigger problem is that we don't know how to differ contacts from each other in a single-address ML. In this case we handle reactions as if they are from the same contact, so reactions are broken completely. Probably we need to create separate contacts for different overriden sender names, this doesn't change the db structure, but anyway this may be a dangerous change as r10s noted.

link2xt commented 3 months ago

To summarize, there are multiple use cases:

Bridge bot is the most complicated scenario to think about, because there may be multiple users behind the bridge that want to put the same thumbs-up reaction. In this case we need to display multiple reactions with different display name coming from the same email address.