diff --git a/api/libs/handlers/src/collections/types.rs b/api/libs/handlers/src/collections/types.rs index 5ecbc0cbf..e7a0ed56c 100644 --- a/api/libs/handlers/src/collections/types.rs +++ b/api/libs/handlers/src/collections/types.rs @@ -110,7 +110,6 @@ pub struct UpdateCollectionAssetsRequest { #[derive(Debug, Clone, Deserialize, Serialize)] pub struct UpdateCollectionRequest { pub collection: Option, - pub assets: Option>, } // Delete collection types diff --git a/api/libs/handlers/src/collections/update_collection_handler.rs b/api/libs/handlers/src/collections/update_collection_handler.rs index 3377498d2..683b51cf2 100644 --- a/api/libs/handlers/src/collections/update_collection_handler.rs +++ b/api/libs/handlers/src/collections/update_collection_handler.rs @@ -1,20 +1,19 @@ use anyhow::{anyhow, Result}; -use chrono::{DateTime, Utc}; +use chrono::Utc; use database::{ collections::fetch_collection, enums::AssetPermissionRole, - models::CollectionToAsset, pool::get_pg_pool, - schema::{collections, collections_to_assets}, + schema::collections, }; -use diesel::{dsl::not, update, BoolExpressionMethods, ExpressionMethods}; +use diesel::{update, ExpressionMethods}; use diesel_async::RunQueryDsl; use std::sync::Arc; use tokio; use uuid::Uuid; use crate::collections::types::{ - CollectionState, UpdateCollectionAssetsRequest, UpdateCollectionObject, UpdateCollectionRequest, + CollectionState, UpdateCollectionObject, UpdateCollectionRequest, }; /// Handler for updating a collection @@ -22,7 +21,7 @@ use crate::collections::types::{ /// # Arguments /// * `user_id` - The ID of the user updating the collection /// * `collection_id` - The ID of the collection to update -/// * `req` - The request containing the collection updates +/// * `req` - The request containing the collection updates (only name and description) /// /// # Returns /// * `Result` - The updated collection state @@ -49,19 +48,7 @@ pub async fn update_collection_handler( None }; - // Update collection assets if provided - let update_collection_assets_handle = if let Some(assets) = req.assets { - let user_id = Arc::clone(&user_id); - let collection_id = Arc::clone(&collection_id); - Some(tokio::spawn(async move { - update_collection_assets(user_id, collection_id, assets).await - })) - } else { - None - }; - - // Wait for all update operations to complete - + // Wait for update operation to complete if let Some(update_collection_record_handle) = update_collection_record_handle { match update_collection_record_handle.await { Ok(Ok(_)) => (), @@ -76,20 +63,6 @@ pub async fn update_collection_handler( } } - if let Some(update_collection_assets_handle) = update_collection_assets_handle { - match update_collection_assets_handle.await { - Ok(Ok(_)) => (), - Ok(Err(e)) => { - tracing::error!("Error updating collection assets: {}", e); - return Err(anyhow!("Error updating collection assets: {}", e)); - } - Err(e) => { - tracing::error!("Error updating collection assets: {}", e); - return Err(anyhow!("Error updating collection assets: {}", e)); - } - } - } - // Get the updated collection let collection = match fetch_collection(&collection_id).await? { Some(collection) => collection, @@ -210,116 +183,39 @@ async fn update_collection_record( Ok(()) } -/// Update collection assets in the database -/// -/// # Arguments -/// * `user_id` - The ID of the user updating the collection -/// * `collection_id` - The ID of the collection to update -/// * `assets` - The assets to add to the collection -/// -/// # Returns -/// * `Result<()>` - Success or error -async fn update_collection_assets( - user_id: Arc, - collection_id: Arc, - assets: Vec, -) -> Result<()> { - let assets = Arc::new(assets); +#[cfg(test)] +mod tests { + use super::*; + use database::models::Collection; + use std::sync::Once; + use uuid::Uuid; - let upsert_handle = { - let assets = Arc::clone(&assets); - let collection_id = Arc::clone(&collection_id); - let user_id = Arc::clone(&user_id); - tokio::spawn(async move { - let mut conn = match get_pg_pool().get().await { - Ok(conn) => conn, - Err(e) => { - tracing::error!("Error getting pg connection: {}", e); - return Err(anyhow!("Error getting pg connection: {}", e)); - } - }; + static INIT: Once = Once::new(); - let new_asset_records: Vec = assets - .iter() - .map(|asset| CollectionToAsset { - collection_id: *collection_id, - asset_id: asset.id, - asset_type: asset.type_, - created_at: Utc::now(), - updated_at: Utc::now(), - deleted_at: None, - created_by: *user_id, - updated_by: *user_id, - }) - .collect(); + // Notice: This is a basic smoke test to check that our changes compile and work + // It doesn't actually hit the database, but verifies that the handler's structure is correct + #[tokio::test] + async fn test_update_collection_handler_smoke_test() { + // Create a mock request with only name and description + let req = UpdateCollectionRequest { + collection: Some(UpdateCollectionObject { + name: Some("New Name".to_string()), + description: Some("New Description".to_string()), + }), + }; - match diesel::insert_into(collections_to_assets::table) - .values(&new_asset_records) - .on_conflict(( - collections_to_assets::collection_id, - collections_to_assets::asset_id, - collections_to_assets::asset_type, - )) - .do_update() - .set(( - collections_to_assets::updated_at.eq(Utc::now()), - collections_to_assets::deleted_at.eq(Option::>::None), - )) - .execute(&mut conn) - .await - { - Ok(_) => Ok(()), - Err(e) => { - tracing::error!("Error updating collection assets: {}", e); - Err(anyhow!("Unable to upsert assets to collection: {}", e)) - } - } - }) - }; + // Create a mock user ID + let user_id = Uuid::new_v4(); + let collection_id = Uuid::new_v4(); - let remove_handle = { - let assets = Arc::clone(&assets); - let collection_id = Arc::clone(&collection_id); - tokio::spawn(async move { - let mut conn = match get_pg_pool().get().await { - Ok(conn) => conn, - Err(e) => { - tracing::error!("Error getting pg connection: {}", e); - return Err(anyhow!("Error getting pg connection: {}", e)); - } - }; - - match update(collections_to_assets::table) - .filter(collections_to_assets::collection_id.eq(*collection_id)) - .filter(not(collections_to_assets::asset_id - .eq_any(assets.iter().map(|a| a.id)) - .and( - collections_to_assets::asset_type.eq_any(assets.iter().map(|a| a.type_)), - ))) - .set(collections_to_assets::deleted_at.eq(Some(Utc::now()))) - .execute(&mut conn) - .await - { - Ok(_) => Ok(()), - Err(e) => { - tracing::error!("Error removing assets from collection: {}", e); - Err(anyhow!("Error removing assets from collection: {}", e)) - } - } - }) - }; - - match upsert_handle.await { - Ok(Ok(_)) => (), - Ok(Err(e)) => return Err(anyhow!("Error upserting assets to collection: {}", e)), - Err(e) => return Err(anyhow!("Error upserting assets to collection: {}", e)), + // Check that our handler function accepts the request with the correct types + // This is mostly a compilation test to verify our refactoring didn't break the interface + let result = update_collection_handler(&user_id, collection_id, req).await; + + // We expect an error since we're not actually hitting the database + assert!(result.is_err()); + + // Check that the error contains the expected message + assert!(result.unwrap_err().to_string().contains("Collection not found")); } - - match remove_handle.await { - Ok(Ok(_)) => (), - Ok(Err(e)) => return Err(anyhow!("Error removing assets from collection: {}", e)), - Err(e) => return Err(anyhow!("Error removing assets from collection: {}", e)), - } - - Ok(()) } diff --git a/api/src/routes/ws/collections/update_collection.rs b/api/src/routes/ws/collections/update_collection.rs index b6019fef3..ef53db0e5 100644 --- a/api/src/routes/ws/collections/update_collection.rs +++ b/api/src/routes/ws/collections/update_collection.rs @@ -1,6 +1,5 @@ use anyhow::{anyhow, Result}; -use chrono::{DateTime, Utc}; -use diesel::{dsl::not, update, AsChangeset, BoolExpressionMethods, ExpressionMethods}; +use diesel::{update, AsChangeset, ExpressionMethods}; use diesel_async::RunQueryDsl; use std::sync::Arc; use middleware::AuthenticatedUser; @@ -11,8 +10,7 @@ use uuid::Uuid; use database::{enums::{AssetPermissionRole, AssetType}, pool::get_pg_pool, - models::CollectionToAsset, - schema::{collections, collections_to_assets},}; + schema::collections}; use crate::{ routes::ws::{ ws::{SubscriptionRwLock, WsErrorCode, WsEvent, WsResponseMessage, WsSendMethod}, @@ -32,12 +30,7 @@ use super::{ collections_router::{CollectionEvent, CollectionRoute}, }; -#[derive(Debug, Clone, Deserialize)] -pub struct UpdateCollectionAssetsRequest { - pub id: Uuid, - #[serde(rename = "type")] - pub type_: AssetType, -} +// UpdateCollectionAssetsRequest removed as per refactoring requirement #[derive(Debug, Clone, Deserialize, AsChangeset)] #[diesel(table_name = collections)] @@ -51,7 +44,6 @@ pub struct UpdateCollectionRequest { pub id: Uuid, #[serde(flatten)] pub collection: Option, - pub assets: Option>, pub team_permissions: Option>, pub user_permissions: Option>, pub remove_teams: Option>, @@ -131,20 +123,7 @@ pub async fn update_collection( None }; - let update_collection_assets_handle = if let Some(assets) = req.assets { - let user_id = Arc::clone(&user_id); - let collection_id = Arc::clone(&collection_id); - Some(tokio::spawn(async move { - match update_collection_assets(user_id, collection_id, assets).await { - Ok(_) => Ok(()), - Err(e) => { - return Err(e); - } - } - })) - } else { - None - }; + // Assets handling has been removed as per refactoring requirement let update_collection_permissions_handle = if req.team_permissions.is_some() || req.user_permissions.is_some() @@ -197,16 +176,7 @@ pub async fn update_collection( } } - if let Some(update_collection_assets_handle) = update_collection_assets_handle { - match update_collection_assets_handle.await { - Ok(_) => (), - Err(e) => { - tracing::error!("Error updating collection assets: {}", e); - send_sentry_error(&e.to_string(), None); - return Err(anyhow!("Error updating collection assets: {}", e)); - } - } - } + // Assets update handling removed as per refactoring requirement let collection = match get_collection_by_id(&user.id, &req.id).await { Ok(collection) => collection, @@ -331,104 +301,4 @@ async fn update_collection_record( Ok(()) } -async fn update_collection_assets( - user_id: Arc, - collection_id: Arc, - assets: Vec, -) -> Result<()> { - let assets = Arc::new(assets); - - let upsert_handle = { - let assets = Arc::clone(&assets); - let collection_id = Arc::clone(&collection_id); - let user_id = Arc::clone(&user_id); - let mut conn = get_pg_pool().get().await?; - tokio::spawn(async move { - let new_asset_records: Vec = assets - .iter() - .map(|asset| CollectionToAsset { - collection_id: *collection_id, - asset_id: asset.id, - asset_type: asset.type_, - created_at: chrono::Utc::now(), - updated_at: chrono::Utc::now(), - deleted_at: None, - created_by: *user_id, - updated_by: *user_id, - }) - .collect(); - match diesel::insert_into(collections_to_assets::table) - .values(&new_asset_records) - .on_conflict(( - collections_to_assets::collection_id, - collections_to_assets::asset_id, - collections_to_assets::asset_type, - )) - .do_update() - .set(( - collections_to_assets::updated_at.eq(chrono::Utc::now()), - collections_to_assets::deleted_at.eq(Option::>::None), - )) - .execute(&mut conn) - .await - { - Ok(_) => Ok(()), - Err(e) => { - tracing::error!("Error updating collection assets: {}", e); - Err(anyhow!("Unable to upsert assets to collection: {}", e)) - } - } - }) - }; - - let remove_handle = { - let assets = Arc::clone(&assets); - let collection_id = Arc::clone(&collection_id); - let mut conn = get_pg_pool().get().await?; - tokio::spawn(async move { - match update(collections_to_assets::table) - .filter(collections_to_assets::collection_id.eq(*collection_id)) - .filter(not(collections_to_assets::asset_id - .eq_any(assets.iter().map(|a| a.id)) - .and( - collections_to_assets::asset_type.eq_any(assets.iter().map(|a| a.type_)), - ))) - .set(collections_to_assets::deleted_at.eq(Some(chrono::Utc::now()))) - .execute(&mut conn) - .await - { - Ok(_) => Ok(()), - Err(e) => { - tracing::error!("Error removing assets from collection: {}", e); - Err(anyhow!("Error removing assets from collection: {}", e)) - } - } - }) - }; - - match upsert_handle.await { - Ok(Ok(_)) => (), - Ok(Err(e)) => { - tracing::error!("Error upserting assets to collection: {}", e); - return Err(anyhow!("Error upserting assets to collection: {}", e)); - } - Err(e) => { - tracing::error!("Error upserting assets to collection: {}", e); - return Err(anyhow!("Error upserting assets to collection: {}", e)); - } - } - - match remove_handle.await { - Ok(Ok(_)) => (), - Ok(Err(e)) => { - tracing::error!("Error removing assets from collection: {}", e); - return Err(anyhow!("Error removing assets from collection: {}", e)); - } - Err(e) => { - tracing::error!("Error removing assets from collection: {}", e); - return Err(anyhow!("Error removing assets from collection: {}", e)); - } - } - - Ok(()) -} +// update_collection_assets function removed as per refactoring requirement