fix: Add Chat-Group-Name-Timestamp header and use it to update group names (#6412)

Add "Chat-Group-Name-Timestamp" message header and use the last-write-wins logic when updating group
names (similar to group member timestamps). Note that if the "Chat-Group-Name-Changed" header is
absent though, we don't add a system message (`MsgGrpNameChangedBy`) because we don't want to blame
anyone.

Author: iequidoo

src/headerdef.rs

@@ -57,6 +57,7 @@ pub enum HeaderDef {
     ChatGroupId,
     ChatGroupName,
     ChatGroupNameChanged,
+    ChatGroupNameTimestamp,
     ChatVerified,
     ChatGroupAvatar,
     ChatUserAvatar,

src/mimefactory.rs

@@ -1149,7 +1149,7 @@ impl MimeFactory {
         let Loaded::Message { chat, msg } = &self.loaded else {
             bail!("Attempt to render MDN as a message");
         };
-        let chat = chat.clone();
+        let mut chat = chat.clone();
         let msg = msg.clone();
         let command = msg.param.get_cmd();
         let mut placeholdertext = None;
@@ -1181,6 +1181,23 @@ impl MimeFactory {
                 "Chat-Group-Name",
                 mail_builder::headers::text::Text::new(chat.name.to_string()).into(),
             ));
+            if chat.param.get_i64(Param::GroupNameTimestamp).is_none()
+                || command == SystemMessage::GroupNameChanged
+            {
+                chat.param
+                    .set_i64(Param::GroupNameTimestamp, self.timestamp);
+                chat.update_param(context).await?;
+            }
+            headers.push((
+                "Chat-Group-Name-Timestamp",
+                mail_builder::headers::text::Text::new(
+                    chat.param
+                        .get_i64(Param::GroupNameTimestamp)
+                        .unwrap_or(0)
+                        .to_string(),
+                )
+                .into(),
+            ));
 
             match command {
                 SystemMessage::MemberRemovedFromGroup => {

src/mimeparser.rs

@@ -445,6 +445,7 @@ impl MimeMessage {
                     HeaderDef::ChatGroupId,
                     HeaderDef::ChatGroupName,
                     HeaderDef::ChatGroupNameChanged,
+                    HeaderDef::ChatGroupNameTimestamp,
                     HeaderDef::ChatGroupAvatar,
                     HeaderDef::ChatGroupMemberRemoved,
                     HeaderDef::ChatGroupMemberAdded,

src/receive_imf.rs

@@ -2104,14 +2104,16 @@ async fn create_group(
             // serialization/deserialization #3650" issue. DC itself never creates group names with
             // leading/trailing whitespace.
             .trim();
+        let mut param = Params::new();
+        param.set_i64(Param::GroupNameTimestamp, mime_parser.timestamp_sent);
         let new_chat_id = ChatId::create_multiuser_record(
             context,
             Chattype::Group,
             grpid,
             grpname,
             create_blocked,
             create_protected,
-            None,
+            Some(param.to_string()),
             mime_parser.timestamp_sent,
         )
         .await
@@ -2332,9 +2334,18 @@ async fn apply_group_changes(
         }
 
         better_msg = Some(stock_str::msg_add_member_local(context, added_addr, from_id).await);
-    } else if let Some(old_name) = mime_parser
+    }
+
+    let group_name_timestamp = mime_parser
+        .get_header(HeaderDef::ChatGroupNameTimestamp)
+        .and_then(|s| s.parse::<i64>().ok());
+    if let Some(old_name) = mime_parser
         .get_header(HeaderDef::ChatGroupNameChanged)
         .map(|s| s.trim())
+        .or(match group_name_timestamp {
+            Some(_) => Some(chat.name.as_str()),
+            None => None,
+        })
     {
         if let Some(grpname) = mime_parser
             .get_header(HeaderDef::ChatGroupName)
@@ -2343,13 +2354,17 @@ async fn apply_group_changes(
         {
             let grpname = &sanitize_single_line(grpname);
             let old_name = &sanitize_single_line(old_name);
-            if chat_id
-                .update_timestamp(
-                    context,
-                    Param::GroupNameTimestamp,
-                    mime_parser.timestamp_sent,
-                )
-                .await?
+
+            let chat_group_name_timestamp = chat.param.get_i64(Param::GroupNameTimestamp);
+            let group_name_timestamp = group_name_timestamp.unwrap_or(mime_parser.timestamp_sent);
+            // NB: For old chats (created before "Chat-Group-Name-Timestamp" header introduction)
+            // this may cause an extra name update and `ChatModified` event, but that's fine.
+            //
+            // To provide group name consistency, compare names if timestamps are equal.
+            if (chat_group_name_timestamp, grpname) < (Some(group_name_timestamp), old_name)
+                && chat_id
+                    .update_timestamp(context, Param::GroupNameTimestamp, group_name_timestamp)
+                    .await?
             {
                 info!(context, "Updating grpname for chat {chat_id}.");
                 context
@@ -2358,10 +2373,18 @@ async fn apply_group_changes(
                     .await?;
                 send_event_chat_modified = true;
             }
-
-            better_msg = Some(stock_str::msg_grp_name(context, old_name, grpname, from_id).await);
+            if mime_parser
+                .get_header(HeaderDef::ChatGroupNameChanged)
+                .is_some()
+            {
+                better_msg.get_or_insert(
+                    stock_str::msg_grp_name(context, old_name, grpname, from_id).await,
+                );
+            }
         }
-    } else if let Some(value) = mime_parser.get_header(HeaderDef::ChatContent) {
+    }
+
+    if let (Some(value), None) = (mime_parser.get_header(HeaderDef::ChatContent), &better_msg) {
         if value == "group-avatar-changed" {
             if let Some(avatar_action) = &mime_parser.group_avatar {
                 // this is just an explicit message containing the group-avatar,
@@ -2837,14 +2860,16 @@ async fn create_adhoc_group(
         return Ok(None);
     }
 
+    let mut param = Params::new();
+    param.set_i64(Param::GroupNameTimestamp, mime_parser.timestamp_sent);
     let new_chat_id: ChatId = ChatId::create_multiuser_record(
         context,
         Chattype::Group,
         "", // Ad hoc groups have no ID.
         grpname,
         create_blocked,
         ProtectionStatus::Unprotected,
-        None,
+        Some(param.to_string()),
         mime_parser.timestamp_sent,
     )
     .await?;

src/receive_imf/receive_imf_tests.rs

@@ -5425,6 +5425,43 @@ Hello!"
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_rename_chat_on_missing_message() -> Result<()> {
+    let alice = TestContext::new_alice().await;
+    let bob = TestContext::new_bob().await;
+    let chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?;
+    add_to_chat_contacts_table(
+        &alice,
+        time(),
+        chat_id,
+        &[Contact::create(&alice, "bob", "[email protected]").await?],
+    )
+    .await?;
+    send_text_msg(&alice, chat_id, "populate".to_string()).await?;
+    let bob_chat_id = bob.recv_msg(&alice.pop_sent_msg().await).await.chat_id;
+    bob_chat_id.accept(&bob).await?;
+
+    // Bob changes the group name.
+    // TODO: If Bob does this too fast, it's not guaranteed that his group name wins because "Date"
+    // of his message may be less than one of Alice's message, and setting "Group-Name-Timestamp" to
+    // a value greater than "Date" doesn't look as a good solution. This should be fixed by
+    // adjusting "Date" on the replier side (limited to a few seconds).
+    SystemTime::shift(Duration::from_secs(3600));
+    chat::set_chat_name(&bob, bob_chat_id, "Renamed").await?;
+    bob.pop_sent_msg().await;
+
+    // Bob adds a new member.
+    let bob_orange = Contact::create(&bob, "orange", "[email protected]").await?;
+    add_contact_to_chat(&bob, bob_chat_id, bob_orange).await?;
+    let add_msg = bob.pop_sent_msg().await;
+
+    // Alice only receives the member addition.
+    alice.recv_msg(&add_msg).await;
+    let chat = Chat::load_from_db(&alice, chat_id).await?;
+    assert_eq!(chat.get_name(), "Renamed");
+    Ok(())
+}
+
 /// Tests that creating a group
 /// is preferred over assigning message to existing
 /// chat based on `In-Reply-To` and `References`.

src/update_helper.rs

@@ -213,6 +213,44 @@ mod tests {
         // Assert that the \n was correctly removed from the group name also in the system message
         assert_eq!(msg.text.contains('\n'), false);
 
+        // This doesn't update the name because Date is the same and name is greater.
+        receive_imf(
+            &t,
+            b"From: Bob Authname <[email protected]>\n\
+                 To: [email protected]\n\
+                 Message-ID: <[email protected]>\n\
+                 Chat-Version: 1.0\n\
+                 Chat-Group-ID: abcde123456\n\
+                 Chat-Group-Name: another name update 4\n\
+                 Chat-Group-Name-Changed: another name update\n\
+                 Date: Sun, 22 Mar 2021 03:00:00 +0000\n\
+                 \n\
+                 4th message\n",
+            false,
+        )
+        .await?;
+        let chat = Chat::load_from_db(&t, chat.id).await?;
+        assert_eq!(chat.name, "another name update");
+
+        // This updates the name because Date is the same and name is lower.
+        receive_imf(
+            &t,
+            b"From: Bob Authname <[email protected]>\n\
+                 To: [email protected]\n\
+                 Message-ID: <[email protected]>\n\
+                 Chat-Version: 1.0\n\
+                 Chat-Group-ID: abcde123456\n\
+                 Chat-Group-Name: another name updat\n\
+                 Chat-Group-Name-Changed: another name update\n\
+                 Date: Sun, 22 Mar 2021 03:00:00 +0000\n\
+                 \n\
+                 5th message\n",
+            false,
+        )
+        .await?;
+        let chat = Chat::load_from_db(&t, chat.id).await?;
+        assert_eq!(chat.name, "another name updat");
+
         Ok(())
     }
 }