deltachat / deltachat-core-rust

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

Empty contact chat rename action is synced every time securejoin runs #5705

Closed link2xt closed 3 months ago

link2xt commented 3 months ago

publicbots@testrun.org bot "pings" other bots by (re)scanning their securejoin setup-contact QR codes in a loop.

While investigating #5703 I noticed that msgs table is full of sync messages similar to this:

This message is used to synchronize data between your devices.

👉 If you see this message in Delta Chat, please update your Delta Chat apps on all devices.||E={"items":[

{"timestamp":1718810737,"data":{"AlterChat":{"id":{"ContactAddr":"web2img@testrun.org"},"action":{"Rename":""}}}}

]}

This "contact rename" sync action generated over and over and looks like a bug. It seems to originate here: https://github.com/deltachat/deltachat-core-rust/blob/8ddc05923be71c06f3853bb91b1c3bb2777388de/src/contact.rs#L650-L657 We can at least check that name is not empty before sending a sync message, but maybe wait for #5697 first because in create_ex name is not sanitized yet.

iequidoo commented 3 months ago

I can't understand who calls Contact::create() then. It shouldn't be called internally. I see that only draft_self_report() calls it, but obviously it's not the reason. Still, maybe we should omit generating a sync message if nothing has changed, i.e. sth_modified == Modifier::None.

EDIT: I think we still should send a sync message even if the contact name isn't changed because Contact::create() is called as a result of an explicit user action. This allows the user to "sync again" the contact name if wasn't synced before for some reason. Maybe we better disable sync messages for bots? Seems that multi-device bots are not smth useful.

EDIT: chat::rename_ex() doesn't send a sync message if the name isn't changed. Let's do the same in Contact::create_ex() for consistency.

iequidoo commented 3 months ago

5fa7cff actually fixed this, let's close.