From eb1245bcebb7fcc6806b9214bdb4011d816f1316 Mon Sep 17 00:00:00 2001 From: Dessalines Date: Mon, 4 Mar 2024 08:19:51 -0500 Subject: [PATCH] When using `saved_only`, sort posts / comments by the saved publish time, not the item creation time (#4479) * Work on saved selection. * Using single value for join. * Removing unecessary check. * Remove saved_only pointless block. --- crates/db_views/Cargo.toml | 2 +- crates/db_views/src/comment_view.rs | 106 ++++++++++++++++++++-------- crates/db_views/src/post_view.rs | 42 ++++++----- 3 files changed, 98 insertions(+), 52 deletions(-) diff --git a/crates/db_views/Cargo.toml b/crates/db_views/Cargo.toml index cdd44869c..df8124c8a 100644 --- a/crates/db_views/Cargo.toml +++ b/crates/db_views/Cargo.toml @@ -39,10 +39,10 @@ tracing = { workspace = true, optional = true } ts-rs = { workspace = true, optional = true } actix-web = { workspace = true, optional = true } i-love-jesus = { workspace = true, optional = true } +chrono = { workspace = true } [dev-dependencies] serial_test = { workspace = true } tokio = { workspace = true } -chrono = { workspace = true } pretty_assertions = { workspace = true } url = { workspace = true } diff --git a/crates/db_views/src/comment_view.rs b/crates/db_views/src/comment_view.rs index ada83b93f..4a729c2b3 100644 --- a/crates/db_views/src/comment_view.rs +++ b/crates/db_views/src/comment_view.rs @@ -1,4 +1,5 @@ use crate::structs::{CommentView, LocalUserView}; +use chrono::{DateTime, Utc}; use diesel::{ dsl::{exists, not}, pg::Pg, @@ -53,13 +54,14 @@ fn queries<'a>() -> Queries< ); let is_saved = |person_id| { - exists( - comment_saved::table.filter( + comment_saved::table + .filter( comment::id .eq(comment_saved::comment_id) .and(comment_saved::person_id.eq(person_id)), - ), - ) + ) + .select(comment_saved::published.nullable()) + .single_value() }; let is_community_followed = |person_id| { @@ -110,9 +112,7 @@ fn queries<'a>() -> Queries< ), ); - let all_joins = move |query: comment::BoxedQuery<'a, Pg>, - my_person_id: Option, - saved_only: bool| { + let all_joins = move |query: comment::BoxedQuery<'a, Pg>, my_person_id: Option| { let score_selection: Box< dyn BoxableExpression<_, Pg, SqlType = sql_types::Nullable>, > = if let Some(person_id) = my_person_id { @@ -129,14 +129,13 @@ fn queries<'a>() -> Queries< Box::new(None::.into_sql::>()) }; - let is_saved_selection: Box> = - if saved_only { - Box::new(true.into_sql::()) - } else if let Some(person_id) = my_person_id { - Box::new(is_saved(person_id)) - } else { - Box::new(false.into_sql::()) - }; + let is_saved_selection: Box< + dyn BoxableExpression<_, Pg, SqlType = sql_types::Nullable>, + > = if let Some(person_id) = my_person_id { + Box::new(is_saved(person_id)) + } else { + Box::new(None::>.into_sql::>()) + }; let is_creator_blocked_selection: Box> = if let Some(person_id) = my_person_id { @@ -160,7 +159,7 @@ fn queries<'a>() -> Queries< creator_is_moderator, creator_is_admin, subscribed_type_selection, - is_saved_selection, + is_saved_selection.is_not_null(), is_creator_blocked_selection, score_selection, )) @@ -168,11 +167,7 @@ fn queries<'a>() -> Queries< let read = move |mut conn: DbConn<'a>, (comment_id, my_person_id): (CommentId, Option)| async move { - let mut query = all_joins( - comment::table.find(comment_id).into_boxed(), - my_person_id, - false, - ); + let mut query = all_joins(comment::table.find(comment_id).into_boxed(), my_person_id); // Hide local only communities from unauthenticated users if my_person_id.is_none() { query = query.filter(community::visibility.eq(CommunityVisibility::Public)); @@ -188,11 +183,7 @@ fn queries<'a>() -> Queries< let person_id_join = my_person_id.unwrap_or(PersonId(-1)); let local_user_id_join = my_local_user_id.unwrap_or(LocalUserId(-1)); - let mut query = all_joins( - comment::table.into_boxed(), - my_person_id, - options.saved_only, - ); + let mut query = all_joins(comment::table.into_boxed(), my_person_id); if let Some(creator_id) = options.creator_id { query = query.filter(comment::creator_id.eq(creator_id)); @@ -243,8 +234,11 @@ fn queries<'a>() -> Queries< } } + // If its saved only, then filter, and order by the saved time, not the comment creation time. if options.saved_only { - query = query.filter(is_saved(person_id_join)); + query = query + .filter(is_saved(person_id_join).is_not_null()) + .then_order_by(is_saved(person_id_join).desc()); } if options.liked_only { @@ -413,7 +407,15 @@ mod tests { newtypes::LanguageId, source::{ actor_language::LocalUserLanguage, - comment::{Comment, CommentInsertForm, CommentLike, CommentLikeForm, CommentUpdateForm}, + comment::{ + Comment, + CommentInsertForm, + CommentLike, + CommentLikeForm, + CommentSaved, + CommentSavedForm, + CommentUpdateForm, + }, community::{ Community, CommunityInsertForm, @@ -428,7 +430,7 @@ mod tests { person_block::{PersonBlock, PersonBlockForm}, post::{Post, PostInsertForm}, }, - traits::{Blockable, Crud, Joinable, Likeable}, + traits::{Blockable, Crud, Joinable, Likeable, Saveable}, utils::{build_db_pool_for_tests, RANK_DEFAULT}, CommunityVisibility, SubscribedType, @@ -927,6 +929,52 @@ mod tests { cleanup(data, pool).await; } + #[tokio::test] + #[serial] + async fn test_saved_order() { + let pool = &build_db_pool_for_tests().await; + let pool = &mut pool.into(); + let data = init_data(pool).await; + + // Save two comments + let save_comment_0_form = CommentSavedForm { + person_id: data.timmy_local_user_view.person.id, + comment_id: data.inserted_comment_0.id, + }; + CommentSaved::save(pool, &save_comment_0_form) + .await + .unwrap(); + + let save_comment_2_form = CommentSavedForm { + person_id: data.timmy_local_user_view.person.id, + comment_id: data.inserted_comment_2.id, + }; + CommentSaved::save(pool, &save_comment_2_form) + .await + .unwrap(); + + // Fetch the saved comments + let comments = CommentQuery { + local_user: Some(&data.timmy_local_user_view), + saved_only: true, + ..Default::default() + } + .list(pool) + .await + .unwrap(); + + // There should only be two comments + assert_eq!(2, comments.len()); + + // The first comment, should be the last one saved (descending order) + assert_eq!(comments[0].comment.id, data.inserted_comment_2.id); + + // The second comment, should be the first one saved + assert_eq!(comments[1].comment.id, data.inserted_comment_0.id); + + cleanup(data, pool).await; + } + async fn cleanup(data: Data, pool: &mut DbPool<'_>) { CommentLike::remove( pool, diff --git a/crates/db_views/src/post_view.rs b/crates/db_views/src/post_view.rs index 04e3e4d3c..80ca481f4 100644 --- a/crates/db_views/src/post_view.rs +++ b/crates/db_views/src/post_view.rs @@ -1,4 +1,5 @@ use crate::structs::{LocalUserView, PaginationCursor, PostView}; +use chrono::{DateTime, Utc}; use diesel::{ debug_query, dsl::{exists, not, IntervalDsl}, @@ -89,13 +90,14 @@ fn queries<'a>() -> Queries< ); let is_saved = |person_id| { - exists( - post_saved::table.filter( + post_saved::table + .filter( post_aggregates::post_id .eq(post_saved::post_id) .and(post_saved::person_id.eq(person_id)), - ), - ) + ) + .select(post_saved::published.nullable()) + .single_value() }; let is_read = |person_id| { @@ -140,16 +142,14 @@ fn queries<'a>() -> Queries< }; let all_joins = move |query: post_aggregates::BoxedQuery<'a, Pg>, - my_person_id: Option, - saved_only: bool| { - let is_saved_selection: Box> = - if saved_only { - Box::new(true.into_sql::()) - } else if let Some(person_id) = my_person_id { - Box::new(is_saved(person_id)) - } else { - Box::new(false.into_sql::()) - }; + my_person_id: Option| { + let is_saved_selection: Box< + dyn BoxableExpression<_, Pg, SqlType = sql_types::Nullable>, + > = if let Some(person_id) = my_person_id { + Box::new(is_saved(person_id)) + } else { + Box::new(None::>.into_sql::>()) + }; let is_read_selection: Box> = if let Some(person_id) = my_person_id { @@ -227,7 +227,7 @@ fn queries<'a>() -> Queries< creator_is_admin, post_aggregates::all_columns, subscribed_type_selection, - is_saved_selection, + is_saved_selection.is_not_null(), is_read_selection, is_hidden_selection, is_creator_blocked_selection, @@ -250,7 +250,6 @@ fn queries<'a>() -> Queries< .filter(post_aggregates::post_id.eq(post_id)) .into_boxed(), my_person_id, - false, ); // Hide deleted and removed for non-admins or mods @@ -298,11 +297,7 @@ fn queries<'a>() -> Queries< let person_id_join = my_person_id.unwrap_or(PersonId(-1)); let local_user_id_join = my_local_user_id.unwrap_or(LocalUserId(-1)); - let mut query = all_joins( - post_aggregates::table.into_boxed(), - my_person_id, - options.saved_only, - ); + let mut query = all_joins(post_aggregates::table.into_boxed(), my_person_id); // hide posts from deleted communities query = query.filter(community::deleted.eq(false)); @@ -408,8 +403,11 @@ fn queries<'a>() -> Queries< query = query.filter(person::bot_account.eq(false)); }; + // If its saved only, then filter, and order by the saved time, not the comment creation time. if let (true, Some(person_id)) = (options.saved_only, my_person_id) { - query = query.filter(is_saved(person_id)); + query = query + .filter(is_saved(person_id).is_not_null()) + .then_order_by(is_saved(person_id).desc()); } // Only hide the read posts, if the saved_only is false. Otherwise ppl with the hide_read // setting wont be able to see saved posts.