fix: Sort system messages to the bottom of the chat

Fix #7435

For most messages, `calc_sort_timestamp()` makes sure that they are at the correct place; esp. that they are not above system messages or other noticed/seen messages.

Most callers of `add_info_msg()`, however, didn't call `calc_sort_timestamp()`, and just used `time()` or `smeared_time()` to get the sort timestamp. Because of this, system messages could sometimes wrongly be sorted above other messages.

This PR fixes this by making the sort timestamp optional in `add_info_msg*()`. If the sort timestamp isn't passed, then the message is sorted to the bottom of the chat. `sent_rcvd_timestamp` is not optional anymore, because we need _some_ timestamp that can be shown to the user (most callers just pass `time()` there).

deltachat-rpc-client/tests/test_securejoin.py

@@ -158,29 +158,29 @@ def test_qr_securejoin_broadcast(acfactory, all_devices_online):
         chat = get_broadcast(ac)
         chat_msgs = chat.get_messages()
 
+        encrypted_msg = chat_msgs.pop(0).get_snapshot()
+        assert encrypted_msg.text == "Messages are end-to-end encrypted."
+        assert encrypted_msg.is_info
+
         if please_wait_info_msg:
             first_msg = chat_msgs.pop(0).get_snapshot()
             assert first_msg.text == "Establishing guaranteed end-to-end encryption, please wait…"
             assert first_msg.is_info
 
-        encrypted_msg = chat_msgs[0].get_snapshot()
+        member_added_msg = chat_msgs.pop(0).get_snapshot()
-        assert encrypted_msg.text == "Messages are end-to-end encrypted."
-        assert encrypted_msg.is_info
-
-        member_added_msg = chat_msgs[1].get_snapshot()
         if inviter_side:
             assert member_added_msg.text == f"Member {contact_snapshot.display_name} added."
         else:
             assert member_added_msg.text == "You joined the channel."
         assert member_added_msg.is_info
 
-        hello_msg = chat_msgs[2].get_snapshot()
+        hello_msg = chat_msgs.pop(0).get_snapshot()
         assert hello_msg.text == "Hello everyone!"
         assert not hello_msg.is_info
         assert hello_msg.show_padlock
         assert hello_msg.error is None
 
-        assert len(chat_msgs) == 3
+        assert len(chat_msgs) == 0
 
         chat_snapshot = chat.get_full_snapshot()
         assert chat_snapshot.is_encrypted

deltachat-rpc-client/tests/test_something.py

@@ -867,15 +867,15 @@ def test_leave_broadcast(acfactory, all_devices_online):
         contact_snapshot = contact.get_snapshot()
         chat_msgs = chat.get_messages()
 
+        encrypted_msg = chat_msgs.pop(0).get_snapshot()
+        assert encrypted_msg.text == "Messages are end-to-end encrypted."
+        assert encrypted_msg.is_info
+
         if please_wait_info_msg:
             first_msg = chat_msgs.pop(0).get_snapshot()
             assert first_msg.text == "Establishing guaranteed end-to-end encryption, please wait…"
             assert first_msg.is_info
 
-        encrypted_msg = chat_msgs.pop(0).get_snapshot()
-        assert encrypted_msg.text == "Messages are end-to-end encrypted."
-        assert encrypted_msg.is_info
-
         member_added_msg = chat_msgs.pop(0).get_snapshot()
         if inviter_side:
             assert member_added_msg.text == f"Member {contact_snapshot.display_name} added."

src/chat.rs

@@ -462,19 +462,15 @@ impl ChatId {
     }
 
     /// Adds message "Messages are end-to-end encrypted".
-    pub(crate) async fn add_encrypted_msg(
+    pub(crate) async fn add_encrypted_msg(self, context: &Context, timestamp: i64) -> Result<()> {
-        self,
-        context: &Context,
-        timestamp_sort: i64,
-    ) -> Result<()> {
         let text = stock_str::messages_e2e_encrypted(context).await;
         add_info_msg_with_cmd(
             context,
             self,
             &text,
             SystemMessage::ChatE2ee,
-            timestamp_sort,
+            Some(timestamp),
-            None,
+            timestamp,
             None,
             None,
             None,
@@ -3465,7 +3461,7 @@ pub(crate) async fn create_group_ex(
             // Add "Messages in this chat use classic email and are not encrypted." message.
             stock_str::chat_unencrypted_explanation(context).await
         };
-        add_info_msg(context, chat_id, &text, create_smeared_timestamp(context)).await?;
+        add_info_msg(context, chat_id, &text).await?;
     }
     if let (true, true) = (sync.into(), !grpid.is_empty()) {
         let id = SyncId::Grpid(grpid);
@@ -4616,9 +4612,11 @@ pub(crate) async fn add_info_msg_with_cmd(
     chat_id: ChatId,
     text: &str,
     cmd: SystemMessage,
-    timestamp_sort: i64,
+    // Timestamp where in the chat the message will be sorted.
-    // Timestamp to show to the user (if this is None, `timestamp_sort` will be shown to the user)
+    // If this is None, the message will be sorted to the bottom.
-    timestamp_sent_rcvd: Option<i64>,
+    timestamp_sort: Option<i64>,
+    // Timestamp to show to the user
+    timestamp_sent_rcvd: i64,
     parent: Option<&Message>,
     from_id: Option<ContactId>,
     added_removed_id: Option<ContactId>,
@@ -4634,6 +4632,22 @@ pub(crate) async fn add_info_msg_with_cmd(
         param.set(Param::ContactAddedRemoved, contact_id.to_u32().to_string());
     }
 
+    let timestamp_sort = if let Some(ts) = timestamp_sort {
+        ts
+    } else {
+        let sort_to_bottom = true;
+        let (received, incoming) = (false, false);
+        chat_id
+            .calc_sort_timestamp(
+                context,
+                smeared_time(context),
+                sort_to_bottom,
+                received,
+                incoming,
+            )
+            .await?
+    };
+
     let row_id =
     context.sql.insert(
         "INSERT INTO msgs (chat_id,from_id,to_id,timestamp,timestamp_sent,timestamp_rcvd,type,state,txt,txt_normalized,rfc724_mid,ephemeral_timer,param,mime_in_reply_to)
@@ -4643,8 +4657,8 @@ pub(crate) async fn add_info_msg_with_cmd(
             from_id.unwrap_or(ContactId::INFO),
             ContactId::INFO,
             timestamp_sort,
-            timestamp_sent_rcvd.unwrap_or(0),
+            timestamp_sent_rcvd,
-            timestamp_sent_rcvd.unwrap_or(0),
+            timestamp_sent_rcvd,
             Viewtype::Text,
             MessageState::InNoticed,
             text,
@@ -4664,19 +4678,14 @@ pub(crate) async fn add_info_msg_with_cmd(
 }
 
 /// Adds info message with a given text and `timestamp` to the chat.
-pub(crate) async fn add_info_msg(
+pub(crate) async fn add_info_msg(context: &Context, chat_id: ChatId, text: &str) -> Result<MsgId> {
-    context: &Context,
-    chat_id: ChatId,
-    text: &str,
-    timestamp: i64,
-) -> Result<MsgId> {
     add_info_msg_with_cmd(
         context,
         chat_id,
         text,
         SystemMessage::Unknown,
-        timestamp,
         None,
+        time(),
         None,
         None,
         None,

src/chat/chat_tests.rs

@@ -1238,7 +1238,7 @@ async fn test_unarchive_if_muted() -> Result<()> {
     chat_id.set_visibility(&t, ChatVisibility::Archived).await?;
     set_muted(&t, chat_id, MuteDuration::Forever).await?;
     send_text_msg(&t, chat_id, "out".to_string()).await?;
-    add_info_msg(&t, chat_id, "info", time()).await?;
+    add_info_msg(&t, chat_id, "info").await?;
     assert_eq!(get_archived_cnt(&t).await?, 1);
 
     // finally, unarchive on sending to not muted chat
@@ -1637,7 +1637,7 @@ async fn test_set_mute_duration() {
 async fn test_add_info_msg() -> Result<()> {
     let t = TestContext::new().await;
     let chat_id = create_group(&t, "foo").await?;
-    add_info_msg(&t, chat_id, "foo info", time()).await?;
+    add_info_msg(&t, chat_id, "foo info").await?;
 
     let msg = t.get_last_msg_in(chat_id).await;
     assert_eq!(msg.get_chat_id(), chat_id);
@@ -1659,8 +1659,8 @@ async fn test_add_info_msg_with_cmd() -> Result<()> {
         chat_id,
         "foo bar info",
         SystemMessage::EphemeralTimerChanged,
-        time(),
         None,
+        time(),
         None,
         None,
         None,
@@ -4507,7 +4507,7 @@ async fn test_info_not_referenced() -> Result<()> {
 
     let bob_received_message = tcm.send_recv_accept(alice, bob, "Hi!").await;
     let bob_chat_id = bob_received_message.chat_id;
-    add_info_msg(bob, bob_chat_id, "Some info", create_smeared_timestamp(bob)).await?;
+    add_info_msg(bob, bob_chat_id, "Some info").await?;
 
     // Bob sends a message.
     // This message should reference Alice's "Hi!" message and not the info message.

src/location.rs

@@ -294,7 +294,7 @@ pub async fn send_locations_to_chat(
             .unwrap_or_default();
     } else if 0 == seconds && is_sending_locations_before {
         let stock_str = stock_str::msg_location_disabled(context).await;
-        chat::add_info_msg(context, chat_id, &stock_str, now).await?;
+        chat::add_info_msg(context, chat_id, &stock_str).await?;
     }
     context.emit_event(EventType::ChatModified(chat_id));
     chatlist_events::emit_chatlist_item_changed(context, chat_id);
@@ -849,7 +849,7 @@ async fn maybe_send_locations(context: &Context) -> Result<Option<u64>> {
                 .context("failed to disable location streaming")?;
 
             let stock_str = stock_str::msg_location_disabled(context).await;
-            chat::add_info_msg(context, chat_id, &stock_str, now).await?;
+            chat::add_info_msg(context, chat_id, &stock_str).await?;
             context.emit_event(EventType::ChatModified(chat_id));
             chatlist_events::emit_chatlist_item_changed(context, chat_id);
         }

src/mimefactory.rs

@@ -1239,7 +1239,7 @@ impl MimeFactory {
                         // created before we had symmetric encryption,
                         // we show an error message.
                         let text = BROADCAST_INCOMPATIBILITY_MSG;
-                        chat::add_info_msg(context, chat.id, text, time()).await?;
+                        chat::add_info_msg(context, chat.id, text).await?;
                         bail!(text);
                     }
                     secret

src/receive_imf.rs

@@ -1777,11 +1777,16 @@ async fn add_parts(
                     "Updated ephemeral timer to {ephemeral_timer:?} for chat {chat_id}."
                 );
                 if mime_parser.is_system_message != SystemMessage::EphemeralTimerChanged {
-                    chat::add_info_msg(
+                    chat::add_info_msg_with_cmd(
                         context,
                         chat_id,
                         &stock_ephemeral_timer_changed(context, ephemeral_timer, from_id).await,
-                        sort_timestamp,
+                        SystemMessage::Unknown,
+                        Some(sort_timestamp),
+                        mime_parser.timestamp_sent,
+                        None,
+                        None,
+                        None,
                     )
                     .await?;
                 }
@@ -1889,8 +1894,8 @@ async fn add_parts(
             chat_id,
             &group_changes_msg,
             cmd,
-            sort_timestamp,
+            Some(sort_timestamp),
-            None,
+            mime_parser.timestamp_sent,
             None,
             None,
             added_removed_id,

src/securejoin.rs

@@ -118,7 +118,7 @@ pub async fn get_securejoin_qr(context: &Context, chat: Option<ChatId>) -> Resul
                         chat.id,
                     );
                     let text = BROADCAST_INCOMPATIBILITY_MSG;
-                    add_info_msg(context, chat.id, text, time()).await?;
+                    add_info_msg(context, chat.id, text).await?;
                     bail!(text.to_string());
                 }
             }

src/securejoin/bob.rs

@@ -127,7 +127,7 @@ pub(super) async fn start_protocol(context: &Context, invite: QrInvite) -> Resul
         QrInvite::Group { .. } => {
             let joining_chat_id = joining_chat_id(context, &invite, private_chat_id).await?;
             let msg = stock_str::secure_join_started(context, invite.contact_id()).await;
-            chat::add_info_msg(context, joining_chat_id, &msg, time()).await?;
+            chat::add_info_msg(context, joining_chat_id, &msg).await?;
             Ok(joining_chat_id)
         }
         QrInvite::Broadcast { .. } => {
@@ -148,28 +148,20 @@ pub(super) async fn start_protocol(context: &Context, invite: QrInvite) -> Resul
             // use the generic `Establishing guaranteed end-to-end encryption, please wait…`
             if !is_contact_in_chat(context, joining_chat_id, ContactId::SELF).await? {
                 let msg = stock_str::securejoin_wait(context).await;
-                chat::add_info_msg(context, joining_chat_id, &msg, time()).await?;
+                chat::add_info_msg(context, joining_chat_id, &msg).await?;
             }
             Ok(joining_chat_id)
         }
         QrInvite::Contact { .. } => {
             // For setup-contact the BobState already ensured the 1:1 chat exists because it
             // uses it to send the handshake messages.
-            // Calculate the sort timestamp before checking the chat protection status so that if we
-            // race with its change, we don't add our message below the protection message.
-            let sort_to_bottom = true;
-            let (received, incoming) = (false, false);
-            let ts_sort = private_chat_id
-                .calc_sort_timestamp(context, 0, sort_to_bottom, received, incoming)
-                .await?;
-            let ts_start = time();
             chat::add_info_msg_with_cmd(
                 context,
                 private_chat_id,
                 &stock_str::securejoin_wait(context).await,
                 SystemMessage::SecurejoinWait,
-                ts_sort,
+                None,
-                Some(ts_start),
+                time(),
                 None,
                 None,
                 None,
@@ -243,7 +235,7 @@ pub(super) async fn handle_auth_required(
                 let contact_id = invite.contact_id();
                 let msg = stock_str::secure_join_replies(context, contact_id).await;
                 let chat_id = joining_chat_id(context, &invite, chat_id).await?;
-                chat::add_info_msg(context, chat_id, &msg, time()).await?;
+                chat::add_info_msg(context, chat_id, &msg).await?;
             }
         }
 

src/smtp.rs

@@ -443,11 +443,11 @@ pub(crate) async fn send_msg_to_smtp(
                         chat_id,
                         &text,
                         crate::mimeparser::SystemMessage::InvalidUnencryptedMail,
+                        Some(timestamp_sort),
                         timestamp_sort,
                         None,
                         None,
                         None,
-                        None,
                     )
                     .await?;
                 };

src/webxdc.rs

@@ -379,8 +379,8 @@ impl Context {
                     instance.chat_id,
                     info.as_str(),
                     SystemMessage::WebxdcInfoMessage,
+                    Some(timestamp),
                     timestamp,
-                    None,
                     Some(&instance),
                     Some(from_id),
                     None,

test-data/golden/test_broadcast_joining_golden_bob

@@ -1,6 +1,6 @@
 InBroadcast#Chat#2002: My Channel [2 member(s)] Icon: e9b6c7a78aa2e4f415644f55a553e73.png
 --------------------------------------------------------------------------------
-Msg#2004: info (Contact#Contact#Info): Establishing guaranteed end-to-end encryption, please wait… [NOTICED][INFO]
 Msg#2003: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
+Msg#2004: info (Contact#Contact#Info): Establishing guaranteed end-to-end encryption, please wait… [NOTICED][INFO]
 Msg#2007🔒:  (Contact#Contact#2001): You joined the channel. [FRESH][INFO]
 --------------------------------------------------------------------------------

test-data/golden/test_sync_broadcast_bob

@@ -1,7 +1,7 @@
 InBroadcast#Chat#2002: Channel [1 member(s)]
 --------------------------------------------------------------------------------
-Msg#2004: info (Contact#Contact#Info): Establishing guaranteed end-to-end encryption, please wait… [NOTICED][INFO]
 Msg#2003: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
+Msg#2004: info (Contact#Contact#Info): Establishing guaranteed end-to-end encryption, please wait… [NOTICED][INFO]
 Msg#2008🔒:  (Contact#Contact#2001): You joined the channel. [FRESH][INFO]
 Msg#2010🔒:  (Contact#Contact#2001): hi [FRESH]
 Msg#2011🔒:  (Contact#Contact#2001): Member Me removed by alice@example.org. [FRESH][INFO]

test-data/golden/two_group_securejoins

@@ -1,9 +1,9 @@
 Group#Chat#6002: Group [3 member(s)]
 --------------------------------------------------------------------------------
+Msg#6003: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
 Msg#6004: info (Contact#Contact#Info): alice@example.org invited you to join this group.
 
 Waiting for the device of alice@example.org to reply… [NOTICED][INFO]
 Msg#6006: info (Contact#Contact#Info): alice@example.org replied, waiting for being added to the group… [NOTICED][INFO]
-Msg#6003: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
 Msg#6008🔒:  (Contact#Contact#6001): Member Me added by alice@example.org. [FRESH][INFO]
 --------------------------------------------------------------------------------

test-data/golden/verified_chats_editor_reordering

@@ -1,10 +1,10 @@
 Group#Chat#3002: Group [3 member(s)]
 --------------------------------------------------------------------------------
+Msg#3003: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
 Msg#3004: info (Contact#Contact#Info): alice@example.org invited you to join this group.
 
 Waiting for the device of alice@example.org to reply… [NOTICED][INFO]
 Msg#3006: info (Contact#Contact#Info): alice@example.org replied, waiting for being added to the group… [NOTICED][INFO]
-Msg#3003: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO]
 Msg#3008🔒:  (Contact#Contact#3002):  [FRESH]
 Msg#3009: info (Contact#Contact#Info): Member bob@example.net added. [NOTICED][INFO]
 Msg#3010🔒:  (Contact#Contact#3001): Member Me added by alice@example.org. [FRESH][INFO]