Remove some bugs from message statistics

Author: Hocuri

deltachat-jsonrpc/src/api.rs

@@ -5,25 +5,26 @@ use std::sync::Arc;
 use std::time::Duration;
 use std::{collections::HashMap, str::FromStr};
 
-use anyhow::{anyhow, bail, ensure, Context, Result};
+use anyhow::{Context, Result, anyhow, bail, ensure};
+use deltachat::EventEmitter;
 pub use deltachat::accounts::Accounts;
 use deltachat::blob::BlobObject;
 use deltachat::chat::{
-    self, add_contact_to_chat, forward_msgs, get_chat_media, get_chat_msgs, get_chat_msgs_ex,
-    marknoticed_chat, remove_contact_from_chat, Chat, ChatId, ChatItem, MessageListOptions,
-    ProtectionStatus,
+    self, Chat, ChatId, ChatItem, MessageListOptions, ProtectionStatus, add_contact_to_chat,
+    forward_msgs, get_chat_media, get_chat_msgs, get_chat_msgs_ex, marknoticed_chat,
+    remove_contact_from_chat,
 };
 use deltachat::chatlist::Chatlist;
 use deltachat::config::Config;
 use deltachat::constants::DC_MSG_ID_DAYMARKER;
-use deltachat::contact::{may_be_valid_addr, Contact, ContactId, Origin};
+use deltachat::contact::{Contact, ContactId, Origin, may_be_valid_addr};
 use deltachat::context::get_info;
 use deltachat::ephemeral::Timer;
 use deltachat::imex;
 use deltachat::location;
 use deltachat::message::get_msg_read_receipts;
 use deltachat::message::{
-    self, delete_msgs_ex, markseen_msgs, Message, MessageState, MsgId, Viewtype,
+    self, Message, MessageState, MsgId, Viewtype, delete_msgs_ex, markseen_msgs,
 };
 use deltachat::peer_channels::{
     leave_webxdc_realtime, send_webxdc_realtime_advertisement, send_webxdc_realtime_data,
@@ -35,10 +36,9 @@ use deltachat::reaction::{get_msg_reactions, send_reaction};
 use deltachat::securejoin;
 use deltachat::stock_str::StockMessage;
 use deltachat::webxdc::StatusUpdateSerial;
-use deltachat::EventEmitter;
 use sanitize_filename::is_sanitized;
 use tokio::fs;
-use tokio::sync::{watch, Mutex, RwLock};
+use tokio::sync::{Mutex, RwLock, watch};
 use types::login_param::EnteredLoginParam;
 use walkdir::WalkDir;
 use yerpc::rpc;
@@ -64,7 +64,7 @@ use self::types::{
         JSONRPCMessageListItem, MessageNotificationInfo, MessageSearchResult, MessageViewtype,
     },
 };
-use crate::api::types::chat_list::{get_chat_list_item_by_id, ChatListItemFetchResult};
+use crate::api::types::chat_list::{ChatListItemFetchResult, get_chat_list_item_by_id};
 use crate::api::types::qr::QrObject;
 
 #[derive(Debug)]
@@ -2017,7 +2017,7 @@ impl CommandApi {
         let message = Message::load_from_db(&ctx, MsgId::new(instance_msg_id)).await?;
         let blob = message.get_webxdc_blob(&ctx, &path).await?;
 
-        use base64::{engine::general_purpose, Engine as _};
+        use base64::{Engine as _, engine::general_purpose};
         Ok(general_purpose::STANDARD_NO_PAD.encode(blob))
     }
 

deltachat-jsonrpc/src/api/types/chat.rs

@@ -1,7 +1,7 @@
 use std::time::{Duration, SystemTime};
 
-use anyhow::{bail, Context as _, Result};
-use deltachat::chat::{self, get_chat_contacts, get_past_chat_contacts, ChatVisibility};
+use anyhow::{Context as _, Result, bail};
+use deltachat::chat::{self, ChatVisibility, get_chat_contacts, get_past_chat_contacts};
 use deltachat::chat::{Chat, ChatId};
 use deltachat::constants::Chattype;
 use deltachat::contact::{Contact, ContactId};

deltachat-jsonrpc/src/api/types/chat_list.rs

@@ -4,7 +4,7 @@ use deltachat::chatlist::get_last_message_for_chat;
 use deltachat::constants::*;
 use deltachat::contact::{Contact, ContactId};
 use deltachat::{
-    chat::{get_chat_contacts, ChatVisibility},
+    chat::{ChatVisibility, get_chat_contacts},
     chatlist::Chatlist,
 };
 use num_traits::cast::ToPrimitive;

deltachat-jsonrpc/src/api/types/http.rs

@@ -16,7 +16,7 @@ pub struct HttpResponse {
 
 impl From<CoreHttpResponse> for HttpResponse {
     fn from(response: CoreHttpResponse) -> Self {
-        use base64::{engine::general_purpose, Engine as _};
+        use base64::{Engine as _, engine::general_purpose};
         let blob = general_purpose::STANDARD_NO_PAD.encode(response.blob);
         let mimetype = response.mimetype;
         let encoding = response.encoding;

src/config.rs

@@ -22,7 +22,7 @@ use crate::login_param::ConfiguredLoginParam;
 use crate::mimefactory::RECOMMENDED_FILE_SIZE;
 use crate::provider::{Provider, get_provider_by_id};
 use crate::sync::{self, Sync::*, SyncData};
-use crate::tools::{get_abs_path, time};
+use crate::tools::get_abs_path;
 
 /// The available configuration keys.
 #[derive(
@@ -442,8 +442,11 @@ pub enum Config {
     /// without storing the email address
     SelfReportingId,
 
-    /// Timestamp of enabling SelfReporting.
-    SelfReportingEnabledTimestamp,
+    /// The last message id that was already included in the previous report,
+    /// or that already existed before the user opted in.
+    /// Only messages with an id larger than this
+    /// will be counted in the next report.
+    SelfReportingLastMsgId,
 
     /// MsgId of webxdc map integration.
     WebxdcIntegration,
@@ -839,9 +842,7 @@ impl Context {
             }
             Config::SelfReporting => {
                 self.sql.set_raw_config(key.as_ref(), value).await?;
-                self.sql
-                    .set_raw_config_int64(Config::SelfReportingEnabledTimestamp.as_ref(), time())
-                    .await?;
+                crate::self_reporting::set_last_msgid(self).await?;
             }
             _ => {
                 self.sql.set_raw_config(key.as_ref(), value).await?;

src/context.rs

@@ -1075,8 +1075,8 @@ impl Context {
                 .to_string(),
         );
         res.insert(
-            "self_reporting_enabled_timestamp",
-            self.get_config_i64(Config::SelfReportingEnabledTimestamp)
+            "self_reporting_last_msg_id",
+            self.get_config_i64(Config::SelfReportingLastMsgId)
                 .await?
                 .to_string(),
         );

src/self_reporting.rs

@@ -12,11 +12,9 @@ use crate::config::Config;
 use crate::constants::{Chattype, DC_CHAT_ID_TRASH};
 use crate::contact::{ContactId, Origin, import_vcard, mark_contact_id_as_verified};
 use crate::context::{Context, get_version_str};
-use crate::download::DownloadState;
 use crate::key::load_self_public_key;
 use crate::log::LogExt;
 use crate::message::{Message, Viewtype};
-use crate::param::{Param, Params};
 use crate::tools::{create_id, time};
 
 pub(crate) const SELF_REPORTING_BOT_EMAIL: &str = "self_reporting@testrun.org";
@@ -105,8 +103,6 @@ pub async fn maybe_send_self_report(context: &Context) -> Result<Option<ChatId>>
 async fn send_self_report(context: &Context) -> Result<ChatId> {
     info!(context, "Sending self report.");
 
-    let last_selfreport_time = context.get_config_i64(Config::LastSelfReportSent).await?;
-
     // Setting this config at the beginning avoids endless loops when things do not
     // work out for whatever reason.
     context
@@ -126,7 +122,7 @@ See TODO[blog post] for more information."
             .to_string(),
     );
 
-    let self_report = get_self_report(context, last_selfreport_time).await?;
+    let self_report = get_self_report(context).await?;
 
     msg.set_file_from_bytes(
         context,
@@ -141,10 +137,34 @@ See TODO[blog post] for more information."
         .log_err(context)
         .ok();
 
+    set_last_msgid(context).await?;
+
     Ok(chat_id)
 }
 
-async fn get_self_report(context: &Context, last_selfreport_time: i64) -> Result<String> {
+pub(crate) async fn set_last_msgid(context: &Context) -> Result<()> {
+    let last_msgid: u64 = context
+        .sql
+        .query_get_value("SELECT MAX(id) FROM msgs", ())
+        .await?
+        .context("All messages are gone")?;
+
+    context
+        .sql
+        .set_raw_config(
+            Config::SelfReportingLastMsgId.as_ref(),
+            Some(&last_msgid.to_string()),
+        )
+        .await?;
+
+    Ok(())
+}
+
+async fn get_self_report(context: &Context) -> Result<String> {
+    let last_msgid = context
+        .get_config_u64(Config::SelfReportingLastMsgId)
+        .await?;
+
     let key_created = load_self_public_key(context)
         .await?
         .primary_key
@@ -161,12 +181,13 @@ async fn get_self_report(context: &Context, last_selfreport_time: i64) -> Result
             id
         }
     };
+
     let statistics = Statistics {
         core_version: get_version_str().to_string(),
         key_created,
         self_reporting_id,
         contact_stats: get_contact_stats(context).await?,
-        message_stats: get_message_stats(context, last_selfreport_time).await?,
+        message_stats: get_message_stats(context, last_msgid).await?, // TODO
         securejoin_source_stats: get_securejoin_source_stats(context).await?,
     };
 
@@ -303,66 +324,89 @@ async fn get_contact_stats(context: &Context) -> Result<Vec<ContactStat>> {
     Ok(contacts)
 }
 
-async fn get_message_stats(
-    context: &Context,
-    mut last_selfreport_time: i64,
-) -> Result<MessageStats> {
-    let enabled_ts: i64 = context
-        .get_config_i64(Config::SelfReportingEnabledTimestamp)
-        .await?;
-    if last_selfreport_time == 0 {
-        last_selfreport_time = enabled_ts;
-    }
-    ensure!(last_selfreport_time > 0, "Enabled Timestamp missing");
+async fn get_message_stats(context: &Context, last_msgid: u64) -> Result<MessageStats> {
+    ensure!(
+        last_msgid >= 9,
+        "Last_msgid < 9 would mean including 'special' messages in the report"
+    );
 
     let selfreporting_bot_chat_id = get_selfreporting_bot(context).await?;
 
     let trans_fn = |t: &mut rusqlite::Transaction| {
         t.pragma_update(None, "query_only", "0")?;
+
+        // This table will hold all 'protected' chats.
+        // Protected chats guarantee that all messages in the chat
+        // are sent with verified encryption
+        // are marked with a green checkmark in the UI.
         t.execute(
-            "CREATE TEMP TABLE temp.verified_chats (
+            "CREATE TEMP TABLE temp.protected_chats (
                 id INTEGER PRIMARY KEY
             ) STRICT",
             (),
         )?;
 
+        // id>9 because chat ids 0..9 are "special" chats like the trash chat.
         t.execute(
-            "INSERT INTO temp.verified_chats
+            "INSERT INTO temp.protected_chats
             SELECT id FROM chats
             WHERE protected=1 AND id>9",
             (),
         )?;
 
+        // In the following SQL statements,
+        // - we always have the line
+        //   `AND from_id=? AND chat_id<>? AND id>? AND hidden=0 AND chat_id<>?`.
+        //   `from_id=?` is to count only outgoing messages.
+        //   The rest excludes:
+        //   - the chat with the self-reporting bot itself,
+        //   - messages sent with the last report, or before the config was enabled
+        //   - hidden system messages, which are not actually shown to the user
+        //   - messages in the 'Trash' chat, which is an internal chat assigned to messages that are not shown to the user
+        // - `(param GLOB '*\nc=1*' OR param GLOB 'c=1*')`
+        //   matches all messages that are end-to-end encrypted
         let to_verified = t.query_row(
             "SELECT COUNT(*) FROM msgs
-            WHERE chat_id IN temp.verified_chats
-            AND chat_id<>? AND id>9 AND timestamp>=? AND hidden=0
-            AND NOT (param GLOB '*\nS=*' OR param GLOB 'S=*')",
-            (selfreporting_bot_chat_id, last_selfreport_time),
+            WHERE chat_id IN temp.protected_chats
+            AND from_id=? AND chat_id<>? AND id>? AND hidden=0 AND chat_id<>?",
+            (
+                ContactId::SELF,
+                selfreporting_bot_chat_id,
+                last_msgid,
+                DC_CHAT_ID_TRASH,
+            ),
             |row| row.get(0),
         )?;
 
         let unverified_encrypted = t.query_row(
             "SELECT COUNT(*) FROM msgs
-            WHERE chat_id not IN temp.verified_chats
+            WHERE chat_id not IN temp.protected_chats
             AND (param GLOB '*\nc=1*' OR param GLOB 'c=1*')
-            AND chat_id<>? AND id>9 AND timestamp>=? AND hidden=0
-            AND NOT (param GLOB '*\nS=*' OR param GLOB 'S=*')",
-            (selfreporting_bot_chat_id, last_selfreport_time),
+            AND from_id=? AND chat_id<>? AND id>? AND hidden=0 AND chat_id<>?",
+            (
+                ContactId::SELF,
+                selfreporting_bot_chat_id,
+                last_msgid,
+                DC_CHAT_ID_TRASH,
+            ),
             |row| row.get(0),
         )?;
 
         let unencrypted = t.query_row(
             "SELECT COUNT(*) FROM msgs
-            WHERE chat_id not IN temp.verified_chats
+            WHERE chat_id not IN temp.protected_chats
             AND NOT (param GLOB '*\nc=1*' OR param GLOB 'c=1*')
-            AND chat_id<>? AND id>9 AND timestamp>=? AND hidden=0
-            AND NOT (param GLOB '*\nS=*' OR param GLOB 'S=*')",
-            (selfreporting_bot_chat_id, last_selfreport_time),
+            AND from_id=? AND chat_id<>? AND id>? AND hidden=0 AND chat_id<>?",
+            (
+                ContactId::SELF,
+                selfreporting_bot_chat_id,
+                last_msgid,
+                DC_CHAT_ID_TRASH,
+            ),
             |row| row.get(0),
         )?;
 
-        t.execute("DROP TABLE temp.verified_chats", ())?;
+        t.execute("DROP TABLE temp.protected_chats", ())?;
 
         Ok(MessageStats {
             to_verified,