diff --git a/src/chat.rs b/src/chat.rs index e98385c06b..7113f97be7 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -477,12 +477,17 @@ impl ChatId { /// Adds message "Messages are end-to-end encrypted". pub(crate) async fn add_e2ee_notice(self, context: &Context, timestamp: i64) -> Result<()> { let text = stock_str::messages_e2ee_info_msg(context); + + // Sort this notice to the very beginning of the chat. + // We don't want any message to appear before this notice + // which is normally added when encrypted chat is created. + let sort_timestamp = 0; add_info_msg_with_cmd( context, self, &text, SystemMessage::ChatE2ee, - Some(timestamp), + Some(sort_timestamp), timestamp, None, None, @@ -925,6 +930,17 @@ SELECT id, rfc724_mid, pre_rfc724_mid, timestamp, ?, 1 FROM msgs WHERE chat_id=? .unwrap_or(0)) } + /// Returns timestamp of us joining the chat if we are the member of the chat. + pub(crate) async fn join_timestamp(self, context: &Context) -> Result> { + context + .sql + .query_get_value( + "SELECT add_timestamp FROM chats_contacts WHERE chat_id=? AND contact_id=?", + (self, ContactId::SELF), + ) + .await + } + /// Returns timestamp of the latest message in the chat, /// including hidden messages or a draft if there is one. pub(crate) async fn get_timestamp(self, context: &Context) -> Result> { @@ -1212,15 +1228,11 @@ SELECT id, rfc724_mid, pre_rfc724_mid, timestamp, ?, 1 FROM msgs WHERE chat_id=? /// corresponding event in case of a system message (usually the current system time). /// `always_sort_to_bottom` makes this adjust the returned timestamp up so that the message goes /// to the chat bottom. - /// `received` -- whether the message is received. Otherwise being sent. - /// `incoming` -- whether the message is incoming. pub(crate) async fn calc_sort_timestamp( self, context: &Context, message_timestamp: i64, always_sort_to_bottom: bool, - received: bool, - incoming: bool, ) -> Result { let mut sort_timestamp = cmp::min(message_timestamp, smeared_time(context)); @@ -1240,38 +1252,6 @@ SELECT id, rfc724_mid, pre_rfc724_mid, timestamp, ?, 1 FROM msgs WHERE chat_id=? (self, MessageState::OutDraft), ) .await? - } else if received { - // Received messages shouldn't mingle with just sent ones and appear somewhere in the - // middle of the chat, so we go after the newest non fresh message. - // - // But if a received outgoing message is older than some seen message, better sort the - // received message purely by timestamp. We could place it just before that seen - // message, but anyway the user may not notice it. - // - // NB: Received outgoing messages may break sorting of fresh incoming ones, but this - // shouldn't happen frequently. Seen incoming messages don't really break sorting of - // fresh ones, they rather mean that older incoming messages are actually seen as well. - context - .sql - .query_row_optional( - "SELECT MAX(timestamp), MAX(IIF(state=?,timestamp_sent,0)) - FROM msgs - WHERE chat_id=? AND hidden=0 AND state>? - HAVING COUNT(*) > 0", - (MessageState::InSeen, self, MessageState::InFresh), - |row| { - let ts: i64 = row.get(0)?; - let ts_sent_seen: i64 = row.get(1)?; - Ok((ts, ts_sent_seen)) - }, - ) - .await? - .and_then(|(ts, ts_sent_seen)| { - match incoming || ts_sent_seen <= message_timestamp { - true => Some(ts), - false => None, - } - }) } else { None }; @@ -1282,7 +1262,16 @@ SELECT id, rfc724_mid, pre_rfc724_mid, timestamp, ?, 1 FROM msgs WHERE chat_id=? sort_timestamp = last_msg_time; } - Ok(sort_timestamp) + if let Some(join_timestamp) = self.join_timestamp(context).await? { + // If we are the member of the chat, don't add messages + // before the timestamp of us joining it. + // This is needed to avoid sorting "Member added" + // or automatically sent bot welcome messages + // above SecureJoin system messages. + Ok(std::cmp::max(sort_timestamp, join_timestamp)) + } else { + Ok(sort_timestamp) + } } } @@ -4938,15 +4927,8 @@ pub(crate) async fn add_info_msg_with_cmd( 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, - ) + .calc_sort_timestamp(context, smeared_time(context), sort_to_bottom) .await? }; diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index ffd3932431..76cbc131c0 100644 --- a/src/chat/chat_tests.rs +++ b/src/chat/chat_tests.rs @@ -6112,3 +6112,54 @@ async fn test_leftgrps() -> Result<()> { Ok(()) } + +/// Tests that if the message arrives late, +/// it can still be sorted above the last seen message. +/// +/// Versions 2.47 and below always sorted incoming messages +/// after the last seen message, but with +/// the introduction of multi-relay it is possible +/// that some messages arrive only to some relays +/// and are fetched after the already arrived seen message. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_late_message_above_seen() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let charlie = &tcm.charlie().await; + + let alice_chat_id = alice + .create_group_with_members("Group", &[bob, charlie]) + .await; + let alice_sent = alice.send_text(alice_chat_id, "Hello everyone!").await; + let bob_chat_id = bob.recv_msg(&alice_sent).await.chat_id; + bob_chat_id.accept(bob).await?; + let charlie_chat_id = charlie.recv_msg(&alice_sent).await.chat_id; + charlie_chat_id.accept(charlie).await?; + + // Bob sends a message, but the message is delayed. + let bob_sent = bob.send_text(bob_chat_id, "Hello from Bob!").await; + SystemTime::shift(Duration::from_secs(1000)); + + let charlie_sent = charlie + .send_text(charlie_chat_id, "Hello from Charlie!") + .await; + + // Alice immediately receives a message from Charlie and reads it. + let alice_received_from_charlie = alice.recv_msg(&charlie_sent).await; + assert_eq!( + alice_received_from_charlie.get_text(), + "Hello from Charlie!" + ); + message::markseen_msgs(alice, vec![alice_received_from_charlie.id]).await?; + + // Bob message arrives later, it should be above the message from Charlie. + let alice_received_from_bob = alice.recv_msg(&bob_sent).await; + assert_eq!(alice_received_from_bob.get_text(), "Hello from Bob!"); + + // The last message in the chat is still from Charlie. + let last_msg = alice.get_last_msg_in(alice_chat_id).await; + assert_eq!(last_msg.get_text(), "Hello from Charlie!"); + + Ok(()) +} diff --git a/src/chatlist.rs b/src/chatlist.rs index b54ad79d3b..534dccf91b 100644 --- a/src/chatlist.rs +++ b/src/chatlist.rs @@ -142,7 +142,7 @@ impl Chatlist { AND c.blocked!=1 AND c.id IN(SELECT chat_id FROM chats_contacts WHERE contact_id=?2 AND add_timestamp >= remove_timestamp) GROUP BY c.id - ORDER BY c.archived=?3 DESC, IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", + ORDER BY c.archived=?3 DESC, IFNULL(NULLIF(m.timestamp,0),c.created_timestamp) DESC, m.id DESC;", (MessageState::OutDraft, query_contact_id, ChatVisibility::Pinned), process_row, ).await? @@ -168,7 +168,7 @@ impl Chatlist { AND c.blocked!=1 AND c.archived=1 GROUP BY c.id - ORDER BY IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", + ORDER BY IFNULL(NULLIF(m.timestamp,0),c.created_timestamp) DESC, m.id DESC;", (MessageState::OutDraft,), process_row, ) @@ -204,7 +204,7 @@ impl Chatlist { AND IFNULL(c.name_normalized,c.name) LIKE ?3 AND (NOT ?4 OR EXISTS (SELECT 1 FROM msgs m WHERE m.chat_id = c.id AND m.state == ?5 AND hidden=0)) GROUP BY c.id - ORDER BY IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", + ORDER BY IFNULL(NULLIF(m.timestamp,0),c.created_timestamp) DESC, m.id DESC;", (MessageState::OutDraft, skip_id, str_like_cmd, only_unread, MessageState::InFresh), process_row, ) @@ -253,7 +253,7 @@ impl Chatlist { AND NOT c.archived=? AND (c.type!=? OR c.id IN(SELECT chat_id FROM chats_contacts WHERE contact_id=? AND add_timestamp >= remove_timestamp)) GROUP BY c.id - ORDER BY c.id=? DESC, c.archived=? DESC, IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", + ORDER BY c.id=? DESC, c.archived=? DESC, IFNULL(NULLIF(m.timestamp,0),c.created_timestamp) DESC, m.id DESC;", ( MessageState::OutDraft, skip_id, ChatVisibility::Archived, Chattype::Group, ContactId::SELF, @@ -279,7 +279,7 @@ impl Chatlist { AND (c.blocked=0 OR c.blocked=2) AND NOT c.archived=? GROUP BY c.id - ORDER BY c.id=0 DESC, c.archived=? DESC, IFNULL(m.timestamp,c.created_timestamp) DESC, m.id DESC;", + ORDER BY c.id=0 DESC, c.archived=? DESC, IFNULL(NULLIF(m.timestamp,0),c.created_timestamp) DESC, m.id DESC;", (MessageState::OutDraft, skip_id, ChatVisibility::Archived, ChatVisibility::Pinned), process_row, ).await? diff --git a/src/message.rs b/src/message.rs index ff3964f3c2..440ac4bcdd 100644 --- a/src/message.rs +++ b/src/message.rs @@ -815,7 +815,11 @@ impl Message { /// Returns the timestamp of the message for sorting. pub fn get_sort_timestamp(&self) -> i64 { - self.timestamp_sort + if self.timestamp_sort != 0 { + self.timestamp_sort + } else { + self.timestamp_sent + } } /// Returns the text of the message. diff --git a/src/receive_imf.rs b/src/receive_imf.rs index a5fca4e74a..3a398d3e09 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -1839,6 +1839,16 @@ async fn add_parts( } } + // Sort message to the bottom if we are not in the chat + // so if we are added via QR code scan + // the message about our addition goes after all the info messages. + // Info messages are sorted by local smeared_timestamp() + // which advances quickly during SecureJoin, + // while "member added" message may have older timestamp + // corresponding to the sender clock. + // In practice inviter clock may even be slightly in the past. + let sort_to_bottom = !chat.is_self_in_chat(context).await?; + let is_location_kml = mime_parser.location_kml.is_some(); let mut group_changes = match chat.typ { _ if chat.id.is_special() => GroupChangesInfo::default(), @@ -1889,16 +1899,8 @@ async fn add_parts( }; let in_fresh = state == MessageState::InFresh; - let sort_to_bottom = false; - let received = true; let sort_timestamp = chat_id - .calc_sort_timestamp( - context, - mime_parser.timestamp_sent, - sort_to_bottom, - received, - mime_parser.incoming, - ) + .calc_sort_timestamp(context, mime_parser.timestamp_sent, sort_to_bottom) .await?; // Apply ephemeral timer changes to the chat. diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index 935c6b12ae..2bc281142b 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/src/receive_imf/receive_imf_tests.rs @@ -819,9 +819,12 @@ async fn load_imf_email(context: &Context, imf_raw: &[u8]) -> Message { .set_config(Config::ShowEmails, Some("2")) .await .unwrap(); - receive_imf(context, imf_raw, false).await.unwrap(); - let chats = Chatlist::try_load(context, 0, None, None).await.unwrap(); - let msg_id = chats.get_msg_id(0).unwrap().unwrap(); + let received_msg = receive_imf(context, imf_raw, false) + .await + .expect("receive_imf failure") + .expect("No message received"); + assert_eq!(received_msg.msg_ids.len(), 1); + let msg_id = received_msg.msg_ids[0]; Message::load_from_db(context, msg_id).await.unwrap() } @@ -2872,9 +2875,8 @@ async fn test_rfc1847_encapsulation() -> Result<()> { // Alice sends a message to Bob using Thunderbird. let raw = include_bytes!("../../test-data/message/rfc1847_encapsulation.eml"); - receive_imf(bob, raw, false).await?; - let msg = bob.get_last_msg().await; + let msg = load_imf_email(bob, raw).await; assert!(msg.get_showpadlock()); Ok(()) @@ -3082,8 +3084,8 @@ async fn test_auto_accept_for_bots() -> Result<()> { async fn test_auto_accept_group_for_bots() -> Result<()> { let t = TestContext::new_alice().await; t.set_config(Config::Bot, Some("1")).await.unwrap(); - receive_imf(&t, GRP_MAIL, false).await?; - let msg = t.get_last_msg().await; + let msg = load_imf_email(&t, GRP_MAIL).await; + let chat = chat::Chat::load_from_db(&t, msg.chat_id).await?; assert!(!chat.is_contact_request()); Ok(()) @@ -3556,9 +3558,9 @@ async fn test_messed_up_message_id() -> Result<()> { let t = TestContext::new_bob().await; let raw = include_bytes!("../../test-data/message/messed_up_message_id.eml"); - receive_imf(&t, raw, false).await?; + let msg = load_imf_email(&t, raw).await; assert_eq!( - t.get_last_msg().await.rfc724_mid, + msg.rfc724_mid, "0bb9ffe1-2596-d997-95b4-1fef8cc4808e@example.org" ); diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index 0c211c2af0..427cc3088e 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -246,46 +246,6 @@ async fn test_old_message_4() -> Result<()> { Ok(()) } -/// Alice is offline for some time. -/// When they come online, first their mvbox is synced and then their inbox. -/// This test tests that the messages are still in the right order. -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn test_old_message_5() -> Result<()> { - let alice = TestContext::new_alice().await; - let msg_sent = receive_imf( - &alice, - b"From: alice@example.org\n\ - To: Bob \n\ - Message-ID: <1234-2-4@example.org>\n\ - Date: Sat, 07 Dec 2019 19:00:27 +0000\n\ - \n\ - Happy birthday, Bob!\n", - true, - ) - .await? - .unwrap(); - - let msg_incoming = receive_imf( - &alice, - b"From: Bob \n\ - To: alice@example.org\n\ - Message-ID: <1234-2-3@example.org>\n\ - Date: Sun, 07 Dec 2019 19:00:26 +0000\n\ - \n\ - Happy birthday to me, Alice!\n", - false, - ) - .await? - .unwrap(); - - assert!(msg_sent.sort_timestamp == msg_incoming.sort_timestamp); - alice - .golden_test_chat(msg_sent.chat_id, "test_old_message_5") - .await; - - Ok(()) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_mdn_doesnt_disable_verification() -> Result<()> { let mut tcm = TestContextManager::new(); diff --git a/src/tools/tools_tests.rs b/src/tools/tools_tests.rs index 907af1868b..3c121dcb86 100644 --- a/src/tools/tools_tests.rs +++ b/src/tools/tools_tests.rs @@ -64,9 +64,11 @@ DKIM Results: Passed=true"; async fn check_parse_receive_headers_integration(raw: &[u8], expected: &str) { let t = TestContext::new_alice().await; - receive_imf(&t, raw, false).await.unwrap(); - let msg = t.get_last_msg().await; - let msg_info = msg.id.get_info(&t).await.unwrap(); + let received = receive_imf(&t, raw, false).await.unwrap().unwrap(); + + assert_eq!(received.msg_ids.len(), 1); + let msg_id = received.msg_ids[0]; + let msg_info = msg_id.get_info(&t).await.unwrap(); // Ignore the first rows of the msg_info because they contain a // received time that depends on the test time which makes it impossible to diff --git a/test-data/golden/receive_imf_delayed_removal_is_ignored b/test-data/golden/receive_imf_delayed_removal_is_ignored index 3333fb8338..22c60c966b 100644 --- a/test-data/golden/receive_imf_delayed_removal_is_ignored +++ b/test-data/golden/receive_imf_delayed_removal_is_ignored @@ -2,9 +2,9 @@ Group#Chat#1001: Group [5 member(s)] -------------------------------------------------------------------------------- Msg#1001: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO] Msg#1002🔒: Me (Contact#Contact#Self): populate √ +Msg#1007🔒: (Contact#Contact#1001): Member fiona@example.net removed by bob@example.net. [FRESH][INFO] Msg#1003: info (Contact#Contact#Info): Member dom@example.net added. [NOTICED][INFO] Msg#1004: info (Contact#Contact#Info): Member fiona@example.net removed. [NOTICED][INFO] Msg#1005🔒: (Contact#Contact#1001): Member elena@example.net added by bob@example.net. [FRESH][INFO] Msg#1006🔒: Me (Contact#Contact#Self): You added member fiona@example.net. [INFO] o -Msg#1007🔒: (Contact#Contact#1001): Member fiona@example.net removed by bob@example.net. [FRESH][INFO] -------------------------------------------------------------------------------- diff --git a/test-data/golden/receive_imf_older_message_from_2nd_device b/test-data/golden/receive_imf_older_message_from_2nd_device index b833432873..a89a4f6854 100644 --- a/test-data/golden/receive_imf_older_message_from_2nd_device +++ b/test-data/golden/receive_imf_older_message_from_2nd_device @@ -1,5 +1,5 @@ Single#Chat#1001: bob@example.net [bob@example.net] Icon: 4138c52e5bc1c576cda7dd44d088c07.png -------------------------------------------------------------------------------- -Msg#1001: Me (Contact#Contact#Self): We share this account √ Msg#1002: Me (Contact#Contact#Self): I'm Alice too √ +Msg#1001: Me (Contact#Contact#Self): We share this account √ -------------------------------------------------------------------------------- diff --git a/test-data/golden/test_old_message_5 b/test-data/golden/test_old_message_5 deleted file mode 100644 index 92bd8ac863..0000000000 --- a/test-data/golden/test_old_message_5 +++ /dev/null @@ -1,5 +0,0 @@ -Single#Chat#11001: Bob [bob@example.net] Icon: 4138c52e5bc1c576cda7dd44d088c07.png --------------------------------------------------------------------------------- -Msg#11001: Me (Contact#Contact#Self): Happy birthday, Bob! √ -Msg#11002: (Contact#Contact#11001): Happy birthday to me, Alice! [FRESH] --------------------------------------------------------------------------------- diff --git a/test-data/golden/test_sync_broadcast_alice1 b/test-data/golden/test_sync_broadcast_alice1 index 7d450fb717..82ce056e5f 100644 --- a/test-data/golden/test_sync_broadcast_alice1 +++ b/test-data/golden/test_sync_broadcast_alice1 @@ -1,7 +1,7 @@ OutBroadcast#Chat#1001: Channel [0 member(s)] -------------------------------------------------------------------------------- Msg#1001: info (Contact#Contact#Info): Messages are end-to-end encrypted. [NOTICED][INFO] -Msg#1006🔒: Me (Contact#Contact#Self): Member bob@example.net added. [INFO] √ Msg#1008🔒: Me (Contact#Contact#Self): hi √ +Msg#1006🔒: Me (Contact#Contact#Self): Member bob@example.net added. [INFO] √ Msg#1009🔒: Me (Contact#Contact#Self): You removed member bob@example.net. [INFO] √ --------------------------------------------------------------------------------