Page MenuHomePhabricator
Differential D14329

[keyserver] Code for updating last_message_for_unread_check
ClosedPublic
Actions

Authored by ashoat on Feb 10 2025, 11:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 11, 3:49 PM
Unknown Object (File)
Tue, Mar 11, 3:40 PM
Unknown Object (File)
Feb 26 2025, 10:36 PM
Unknown Object (File)
Feb 26 2025, 11:03 AM
Unknown Object (File)
Feb 24 2025, 9:36 PM
Unknown Object (File)
Feb 24 2025, 3:25 AM
Unknown Object (File)
Feb 22 2025, 9:20 PM
Unknown Object (File)
Feb 22 2025, 9:20 PM
View All 30 Files
Subscribers
None

Details

Reviewers
tomek
angelika
Commits
rCOMMaf2ce3b4be78: [keyserver] Code for updating last_message_for_unread_check
Summary

This diff sets up code for keeping last_message_for_unread_check updated, but does not (yet) support excluding certain message types as outlined in ENG-9557.

We are doing one of two things for each change:

  1. Setting last_message_for_unread_check to the same value as last_message
  2. Where appropriate, replacing a get or set of last_message with a set of last_message_for_unread_check. I will explain these inline

Depends on D14328

Test Plan

Tested in combination with later diffs, to confirm that it's possible for some message types to not set a thread to unread

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat created this revision.Feb 10 2025, 11:53 PM
ashoat added inline comments.Feb 11 2025, 12:05 AM
keyserver/src/creators/message-creator.js
668 ↗(On Diff #46977)

The changes here simply set last_message_for_unread_check to the same value as last_message (though this will change in a later diff)

keyserver/src/updaters/activity-updaters.js
164–166 ↗(On Diff #46977)

This is the only place where lastMessage (the one changed below) is used. It's used to determine what to set last_read_message to. Since this value is compared to last_message_for_unread_check now, it makes sense to set it based on that. Ultimately it's not super important, but I think this is the more "correct" approach

507 ↗(On Diff #46977)

The changes here update us to use last_message_for_unread_check instead of last_message. See comment above (in same file) for how this is used

keyserver/src/updaters/thread-permission-updaters.js
961 ↗(On Diff #46977)

I removed this line (and last_read_message below) because it already the DEFAULT value for the column, so there's no point specifying it

976 ↗(On Diff #46977)

I replaced last_message with last_message_for_unread_check here because:

  1. last_message only exists to join a message with that ID, and no message with ID 0 exists
  2. We want to set last_message_for_unread_check so that the thread might appear unread in the case of rowToSave.unread
1010–1062 ↗(On Diff #46977)

This query was introduced in D9598 to address ENG-4101.

There was some complexity here to deal with the fact that last_message was previously used for two things. The query needed to set last_message correctly to make sure we could fetch at least one message for the thread, but that previously meant we also needed to set last_read_message correctly, so that the thread unread status would appear correctly.

Now that last_message isn't used for the unread status, we don't have to set last_read_message in this query, and can rely on the approach in the INSERT above (leaving last_read_message as 0, and setting last_message_for_unread_check to either 0 or 1, depending on whether the thread should appear as unread).

1061–1078 ↗(On Diff #46977)

Here, we simply set last_message_for_unread_check to the same value as last_read_message when a user leaves a thread, so that we can make sure that the thread is "marked as read". It doesn't particularly matter as this doesn't appear in the UI, but it seemed like the "correct" thing to do

ashoat added a child revision: D14330: [keyserver] Update unread checks to use new last_message_for_unread_check column.Feb 11 2025, 12:06 AM
Harbormaster completed remote builds in B33445: Diff 46977.Feb 11 2025, 12:11 AM
ashoat requested review of this revision.Feb 11 2025, 12:11 AM
tomek accepted this revision.Feb 11 2025, 5:15 AM
This revision is now accepted and ready to land.Feb 11 2025, 5:15 AM
Closed by commit rCOMMaf2ce3b4be78: [keyserver] Code for updating last_message_for_unread_check (authored by ashoat). · Explain WhyFeb 12 2025, 5:59 AM
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMMaf2ce3b4be78: [keyserver] Code for updating last_message_for_unread_check.

Revision Contents
Changeset List

PathSize
keyserver/
src/
creators/
12 lines
updaters/
4 lines
43 lines

Diff 47025

View Options

keyserver/src/creators/message-creator.js

use backup_client::{ use backup_client::{
BackupClient, BackupData, BackupDescriptor, RequestedData, BackupClient, BackupData, BackupDescriptor, RequestedData,
}; };
use bytesize::ByteSize; use bytesize::ByteSize;
use comm_lib::{auth::UserIdentity, backup::LatestBackupIDResponse}; use comm_lib::{auth::UserIdentity, backup::LatestBackupIDResponse};
use commtest::identity::device::register_user_device;
use commtest::{ use commtest::{
service_addr, service_addr,
tools::{generate_stable_nbytes, obtain_number_of_threads, Error}, tools::{generate_stable_nbytes, obtain_number_of_threads, Error},
}; };
use grpc_clients::identity::DeviceType;
use tokio::{runtime::Runtime, task::JoinSet}; use tokio::{runtime::Runtime, task::JoinSet};
#[tokio::test] #[tokio::test]
async fn backup_performance_test() -> Result<(), Error> { async fn backup_performance_test() -> Result<(), Error> {
let backup_client = BackupClient::new(service_addr::BACKUP_SERVICE_HTTP)?; let backup_client = BackupClient::new(service_addr::BACKUP_SERVICE_HTTP)?;
let number_of_threads = obtain_number_of_threads(); let number_of_threads = obtain_number_of_threads();
Show All 16 Lines backup_data.push(BackupData {
ByteSize::mib(4).as_u64() as usize, ByteSize::mib(4).as_u64() as usize,
Some(i as u8), Some(i as u8),
), ),
attachments: vec![], attachments: vec![],
siwe_backup_msg: None, siwe_backup_msg: None,
}); });
} }
let device_info_1 = register_user_device(None, Some(DeviceType::Ios)).await;
let device_info_2 = register_user_device(None, Some(DeviceType::Ios)).await;
let user_identities = [ let user_identities = [
UserIdentity { UserIdentity {
user_id: "1".to_string(), user_id: device_info_1.user_id.clone(),
access_token: "dummy access token".to_string(), access_token: device_info_1.access_token,
device_id: "dummy device_id".to_string(), device_id: device_info_1.device_id,
}, },
UserIdentity { UserIdentity {
user_id: "2".to_string(), user_id: device_info_2.user_id.clone(),
access_token: "dummy access token".to_string(), access_token: device_info_2.access_token,
device_id: "dummy device_id".to_string(), device_id: device_info_2.device_id,
}, },
]; ];
tokio::task::spawn_blocking(move || { tokio::task::spawn_blocking(move || {
println!("Creating new backups"); println!("Creating new backups");
rt.block_on(async { rt.block_on(async {
let mut set = JoinSet::new(); let mut set = JoinSet::new();
for (i, item) in backup_data.iter().cloned().enumerate() { for (i, item) in backup_data.iter().cloned().enumerate() {
▲ Show 20 Lines • Show All 113 Lines • Show Last 20 Lines
View Options

keyserver/src/updaters/activity-updaters.js

use backup_client::{ use backup_client::{
BackupClient, BackupData, BackupDescriptor, RequestedData, BackupClient, BackupData, BackupDescriptor, RequestedData,
}; };
use bytesize::ByteSize; use bytesize::ByteSize;
use comm_lib::{auth::UserIdentity, backup::LatestBackupIDResponse}; use comm_lib::{auth::UserIdentity, backup::LatestBackupIDResponse};
use commtest::identity::device::register_user_device;
use commtest::{ use commtest::{
service_addr, service_addr,
tools::{generate_stable_nbytes, obtain_number_of_threads, Error}, tools::{generate_stable_nbytes, obtain_number_of_threads, Error},
}; };
use grpc_clients::identity::DeviceType;
use tokio::{runtime::Runtime, task::JoinSet}; use tokio::{runtime::Runtime, task::JoinSet};
#[tokio::test] #[tokio::test]
async fn backup_performance_test() -> Result<(), Error> { async fn backup_performance_test() -> Result<(), Error> {
let backup_client = BackupClient::new(service_addr::BACKUP_SERVICE_HTTP)?; let backup_client = BackupClient::new(service_addr::BACKUP_SERVICE_HTTP)?;
let number_of_threads = obtain_number_of_threads(); let number_of_threads = obtain_number_of_threads();
Show All 16 Lines backup_data.push(BackupData {
ByteSize::mib(4).as_u64() as usize, ByteSize::mib(4).as_u64() as usize,
Some(i as u8), Some(i as u8),
), ),
attachments: vec![], attachments: vec![],
siwe_backup_msg: None, siwe_backup_msg: None,
}); });
} }
let device_info_1 = register_user_device(None, Some(DeviceType::Ios)).await;
let device_info_2 = register_user_device(None, Some(DeviceType::Ios)).await;
let user_identities = [ let user_identities = [
UserIdentity { UserIdentity {
user_id: "1".to_string(), user_id: device_info_1.user_id.clone(),
access_token: "dummy access token".to_string(), access_token: device_info_1.access_token,
device_id: "dummy device_id".to_string(), device_id: device_info_1.device_id,
}, },
UserIdentity { UserIdentity {
user_id: "2".to_string(), user_id: device_info_2.user_id.clone(),
access_token: "dummy access token".to_string(), access_token: device_info_2.access_token,
device_id: "dummy device_id".to_string(), device_id: device_info_2.device_id,
}, },
]; ];
tokio::task::spawn_blocking(move || { tokio::task::spawn_blocking(move || {
println!("Creating new backups"); println!("Creating new backups");
rt.block_on(async { rt.block_on(async {
let mut set = JoinSet::new(); let mut set = JoinSet::new();
for (i, item) in backup_data.iter().cloned().enumerate() { for (i, item) in backup_data.iter().cloned().enumerate() {
▲ Show 20 Lines • Show All 113 Lines • Show Last 20 Lines
View Options

keyserver/src/updaters/thread-permission-updaters.js

use backup_client::{ use backup_client::{
BackupClient, BackupData, BackupDescriptor, RequestedData, BackupClient, BackupData, BackupDescriptor, RequestedData,
}; };
use bytesize::ByteSize; use bytesize::ByteSize;
use comm_lib::{auth::UserIdentity, backup::LatestBackupIDResponse}; use comm_lib::{auth::UserIdentity, backup::LatestBackupIDResponse};
use commtest::identity::device::register_user_device;
use commtest::{ use commtest::{
service_addr, service_addr,
tools::{generate_stable_nbytes, obtain_number_of_threads, Error}, tools::{generate_stable_nbytes, obtain_number_of_threads, Error},
}; };
use grpc_clients::identity::DeviceType;
use tokio::{runtime::Runtime, task::JoinSet}; use tokio::{runtime::Runtime, task::JoinSet};
#[tokio::test] #[tokio::test]
async fn backup_performance_test() -> Result<(), Error> { async fn backup_performance_test() -> Result<(), Error> {
let backup_client = BackupClient::new(service_addr::BACKUP_SERVICE_HTTP)?; let backup_client = BackupClient::new(service_addr::BACKUP_SERVICE_HTTP)?;
let number_of_threads = obtain_number_of_threads(); let number_of_threads = obtain_number_of_threads();
Show All 16 Lines backup_data.push(BackupData {
ByteSize::mib(4).as_u64() as usize, ByteSize::mib(4).as_u64() as usize,
Some(i as u8), Some(i as u8),
), ),
attachments: vec![], attachments: vec![],
siwe_backup_msg: None, siwe_backup_msg: None,
}); });
} }
let device_info_1 = register_user_device(None, Some(DeviceType::Ios)).await;
let device_info_2 = register_user_device(None, Some(DeviceType::Ios)).await;
let user_identities = [ let user_identities = [
UserIdentity { UserIdentity {
user_id: "1".to_string(), user_id: device_info_1.user_id.clone(),
access_token: "dummy access token".to_string(), access_token: device_info_1.access_token,
device_id: "dummy device_id".to_string(), device_id: device_info_1.device_id,
}, },
UserIdentity { UserIdentity {
user_id: "2".to_string(), user_id: device_info_2.user_id.clone(),
access_token: "dummy access token".to_string(), access_token: device_info_2.access_token,
device_id: "dummy device_id".to_string(), device_id: device_info_2.device_id,
}, },
]; ];
tokio::task::spawn_blocking(move || { tokio::task::spawn_blocking(move || {
println!("Creating new backups"); println!("Creating new backups");
rt.block_on(async { rt.block_on(async {
let mut set = JoinSet::new(); let mut set = JoinSet::new();
for (i, item) in backup_data.iter().cloned().enumerate() { for (i, item) in backup_data.iter().cloned().enumerate() {
▲ Show 20 Lines • Show All 113 Lines • Show Last 20 Lines