diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bb8f269..4995f9ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + - [324](https://github.com/thoth-pub/thoth/issues/324) - Make Locations editable, including the ability to change the Canonical Location for a Publication ## [[0.12.10]](https://github.com/thoth-pub/thoth/releases/tag/v0.12.10) - 2024-10-01 ### Added diff --git a/thoth-api/src/graphql/model.rs b/thoth-api/src/graphql/model.rs index 0537f31a..830cb905 100644 --- a/thoth-api/src/graphql/model.rs +++ b/thoth-api/src/graphql/model.rs @@ -1984,13 +1984,6 @@ impl MutationRoot { )?)?; } - if data.canonical != location.canonical { - // Each publication must have exactly one canonical location. - // Updating an existing location would always violate this, - // as it should always result in either zero or two canonical locations. - return Err(ThothError::CanonicalLocationError.into()); - } - if data.canonical { data.canonical_record_complete(&context.db)?; } diff --git a/thoth-api/src/model/location/crud.rs b/thoth-api/src/model/location/crud.rs index 1692dce4..878e3530 100644 --- a/thoth-api/src/model/location/crud.rs +++ b/thoth-api/src/model/location/crud.rs @@ -2,11 +2,11 @@ use super::{ Location, LocationField, LocationHistory, LocationOrderBy, LocationPlatform, NewLocation, NewLocationHistory, PatchLocation, }; +use crate::db_insert; use crate::graphql::utils::Direction; use crate::model::{Crud, DbInsert, HistoryEntry}; use crate::schema::{location, location_history}; -use crate::{crud_methods, db_insert}; -use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl}; +use diesel::{Connection, ExpressionMethods, QueryDsl, RunQueryDsl}; use thoth_errors::{ThothError, ThothResult}; use uuid::Uuid; @@ -126,7 +126,73 @@ impl Crud for Location { crate::model::publication::Publication::from_id(db, &self.publication_id)?.publisher_id(db) } - crud_methods!(location::table, location::dsl::location); + // `crud_methods!` cannot be used for update(), because we need to execute multiple statements + // in the same transaction for changing a non-canonical location to canonical. + // These functions recreate the `crud_methods!` logic. + fn from_id(db: &crate::db::PgPool, entity_id: &Uuid) -> ThothResult { + let mut connection = db.get()?; + location::table + .find(entity_id) + .get_result::(&mut connection) + .map_err(Into::into) + } + + fn create(db: &crate::db::PgPool, data: &NewLocation) -> ThothResult { + db.get()?.transaction(|connection| { + diesel::insert_into(location::table) + .values(data) + .get_result::(connection) + .map_err(Into::into) + }) + } + + fn update( + &self, + db: &crate::db::PgPool, + data: &PatchLocation, + account_id: &Uuid, + ) -> ThothResult { + let mut connection = db.get()?; + connection + .transaction(|connection| { + if data.canonical == self.canonical { + // No change in canonical status, just update the current location + diesel::update(location::table.find(&self.location_id)) + .set(data) + .get_result::(connection) + .map_err(Into::into) + } else if self.canonical && !data.canonical { + // Trying to change canonical location to non-canonical results in error. + Err(ThothError::CanonicalLocationError) + } else { + // Update the existing canonical location to non-canonical + let mut old_canonical_location = + PatchLocation::from(self.get_canonical_location(db)?); + old_canonical_location.canonical = false; + diesel::update(location::table.find(old_canonical_location.location_id)) + .set(old_canonical_location) + .execute(connection)?; + diesel::update(location::table.find(&self.location_id)) + .set(data) + .get_result::(connection) + .map_err(Into::into) + } + }) + .and_then(|location| { + self.new_history_entry(account_id) + .insert(&mut connection) + .map(|_| location) + }) + } + + fn delete(self, db: &crate::db::PgPool) -> ThothResult { + db.get()?.transaction(|connection| { + diesel::delete(location::table.find(self.location_id)) + .execute(connection) + .map(|_| self) + .map_err(Into::into) + }) + } } impl HistoryEntry for Location { @@ -181,6 +247,17 @@ impl NewLocation { } } +impl Location { + pub fn get_canonical_location(&self, db: &crate::db::PgPool) -> ThothResult { + let mut connection = db.get()?; + location::table + .filter(location::publication_id.eq(self.publication_id)) + .filter(location::canonical.eq(true)) + .first::(&mut connection) + .map_err(Into::into) + } +} + impl PatchLocation { pub fn canonical_record_complete(&self, db: &crate::db::PgPool) -> ThothResult<()> { location_canonical_record_complete( diff --git a/thoth-api/src/model/location/mod.rs b/thoth-api/src/model/location/mod.rs index bdf894b7..fba42a0a 100644 --- a/thoth-api/src/model/location/mod.rs +++ b/thoth-api/src/model/location/mod.rs @@ -242,6 +242,42 @@ impl Default for LocationOrderBy { } } +impl From for PatchLocation { + fn from(location: Location) -> Self { + PatchLocation { + location_id: location.location_id, + publication_id: location.publication_id, + landing_page: location.landing_page, + full_text_url: location.full_text_url, + location_platform: location.location_platform, + canonical: location.canonical, + } + } +} + +#[test] +fn test_location_to_patch_location() { + let location = Location { + location_id: Uuid::parse_str("00000000-0000-0000-AAAA-000000000001").unwrap(), + publication_id: Uuid::parse_str("00000000-0000-0000-AAAA-000000000002").unwrap(), + landing_page: Some("https://www.book.com/pb_landing".to_string()), + full_text_url: Some("https://example.com/full_text.pdf".to_string()), + location_platform: LocationPlatform::PublisherWebsite, + created_at: Default::default(), + updated_at: Default::default(), + canonical: true, + }; + + let patch_location = PatchLocation::from(location.clone()); + + assert_eq!(patch_location.location_id, location.location_id); + assert_eq!(patch_location.publication_id, location.publication_id); + assert_eq!(patch_location.landing_page, location.landing_page); + assert_eq!(patch_location.full_text_url, location.full_text_url); + assert_eq!(patch_location.location_platform, location.location_platform); + assert_eq!(patch_location.canonical, location.canonical); +} + #[test] fn test_locationplatform_default() { let locationplatform: LocationPlatform = Default::default(); diff --git a/thoth-api/src/model/mod.rs b/thoth-api/src/model/mod.rs index e35126b8..42fb26c4 100644 --- a/thoth-api/src/model/mod.rs +++ b/thoth-api/src/model/mod.rs @@ -400,24 +400,18 @@ macro_rules! crud_methods { use diesel::{QueryDsl, RunQueryDsl}; let mut connection = db.get()?; - match $entity_dsl + $entity_dsl .find(entity_id) .get_result::(&mut connection) - { - Ok(t) => Ok(t), - Err(e) => Err(ThothError::from(e)), - } + .map_err(Into::into) } fn create(db: &$crate::db::PgPool, data: &Self::NewEntity) -> ThothResult { let mut connection = db.get()?; - match diesel::insert_into($table_dsl) + diesel::insert_into($table_dsl) .values(data) .get_result::(&mut connection) - { - Ok(t) => Ok(t), - Err(e) => Err(ThothError::from(e)), - } + .map_err(Into::into) } /// Makes a database transaction that first updates the entity and then creates a new @@ -483,13 +477,10 @@ macro_rules! db_insert { fn insert(&self, connection: &mut diesel::PgConnection) -> ThothResult { use diesel::RunQueryDsl; - match diesel::insert_into($table_dsl) + diesel::insert_into($table_dsl) .values(self) .get_result(connection) - { - Ok(t) => Ok(t), - Err(e) => Err(ThothError::from(e)), - } + .map_err(Into::into) } }; } diff --git a/thoth-api/src/model/work_relation/crud.rs b/thoth-api/src/model/work_relation/crud.rs index 9f5fda3f..1e7fbfdd 100644 --- a/thoth-api/src/model/work_relation/crud.rs +++ b/thoth-api/src/model/work_relation/crud.rs @@ -118,13 +118,10 @@ impl Crud for WorkRelation { // This function recreates the `crud_methods!` from_id() logic. fn from_id(db: &crate::db::PgPool, entity_id: &Uuid) -> ThothResult { let mut connection = db.get()?; - match work_relation::table + work_relation::table .find(entity_id) .get_result::(&mut connection) - { - Ok(t) => Ok(t), - Err(e) => Err(ThothError::from(e)), - } + .map_err(Into::into) } fn create(db: &crate::db::PgPool, data: &NewWorkRelation) -> ThothResult { diff --git a/thoth-app/src/component/locations_form.rs b/thoth-app/src/component/locations_form.rs index 8ee8c1f8..4515f938 100644 --- a/thoth-app/src/component/locations_form.rs +++ b/thoth-app/src/component/locations_form.rs @@ -22,7 +22,7 @@ use crate::models::location::create_location_mutation::CreateLocationRequest; use crate::models::location::create_location_mutation::CreateLocationRequestBody; use crate::models::location::create_location_mutation::PushActionCreateLocation; use crate::models::location::create_location_mutation::PushCreateLocation; -use crate::models::location::create_location_mutation::Variables; +use crate::models::location::create_location_mutation::Variables as CreateVariables; use crate::models::location::delete_location_mutation::DeleteLocationRequest; use crate::models::location::delete_location_mutation::DeleteLocationRequestBody; use crate::models::location::delete_location_mutation::PushActionDeleteLocation; @@ -30,8 +30,14 @@ use crate::models::location::delete_location_mutation::PushDeleteLocation; use crate::models::location::delete_location_mutation::Variables as DeleteVariables; use crate::models::location::location_platforms_query::FetchActionLocationPlatforms; use crate::models::location::location_platforms_query::FetchLocationPlatforms; +use crate::models::location::update_location_mutation::PushActionUpdateLocation; +use crate::models::location::update_location_mutation::PushUpdateLocation; +use crate::models::location::update_location_mutation::UpdateLocationRequest; +use crate::models::location::update_location_mutation::UpdateLocationRequestBody; +use crate::models::location::update_location_mutation::Variables as UpdateVariables; use crate::models::location::LocationPlatformValues; use crate::string::CANCEL_BUTTON; +use crate::string::EDIT_BUTTON; use crate::string::EMPTY_LOCATIONS; use crate::string::NO; use crate::string::REMOVE_BUTTON; @@ -42,11 +48,13 @@ use super::ToOption; pub struct LocationsFormComponent { data: LocationsFormData, - new_location: Location, - show_add_form: bool, + location: Location, + show_modal_form: bool, + in_edit_mode: bool, fetch_location_platforms: FetchLocationPlatforms, - push_location: PushCreateLocation, + create_location: PushCreateLocation, delete_location: PushDeleteLocation, + update_location: PushUpdateLocation, notification_bus: NotificationDispatcher, } @@ -56,13 +64,15 @@ struct LocationsFormData { } pub enum Msg { - ToggleAddFormDisplay(bool), + ToggleModalFormDisplay(bool, Option), SetLocationPlatformsFetchState(FetchActionLocationPlatforms), GetLocationPlatforms, - SetLocationPushState(PushActionCreateLocation), + SetLocationCreateState(PushActionCreateLocation), CreateLocation, SetLocationDeleteState(PushActionDeleteLocation), DeleteLocation(Uuid), + SetLocationUpdateState(PushActionUpdateLocation), + UpdateLocation, ChangeLandingPage(String), ChangeFullTextUrl(String), ChangeLocationPlatform(LocationPlatform), @@ -73,7 +83,7 @@ pub enum Msg { pub struct Props { pub locations: Option>, pub publication_id: Uuid, - pub update_locations: Callback>>, + pub update_locations: Callback<()>, } impl Component for LocationsFormComponent { @@ -82,35 +92,50 @@ impl Component for LocationsFormComponent { fn create(ctx: &Context) -> Self { let data: LocationsFormData = Default::default(); - let show_add_form = false; + let show_modal_form = false; + let in_edit_mode = false; // The first location needs to be canonical = true (as it will be // the only location); subsequent locations need to be canonical = false - let new_location = Location { + let location = Location { canonical: ctx.props().locations.as_ref().unwrap_or(&vec![]).is_empty(), ..Default::default() }; let fetch_location_platforms = Default::default(); - let push_location = Default::default(); + let create_location = Default::default(); let delete_location = Default::default(); + let update_location = Default::default(); let notification_bus = NotificationBus::dispatcher(); ctx.link().send_message(Msg::GetLocationPlatforms); LocationsFormComponent { data, - new_location, - show_add_form, + location, + show_modal_form, + in_edit_mode, fetch_location_platforms, - push_location, + create_location, delete_location, + update_location, notification_bus, } } fn update(&mut self, ctx: &Context, msg: Self::Message) -> bool { match msg { - Msg::ToggleAddFormDisplay(value) => { - self.show_add_form = value; + Msg::ToggleModalFormDisplay(show_form, l) => { + self.show_modal_form = show_form; + self.in_edit_mode = l.is_some(); + + if self.in_edit_mode { + if let Some(location) = l { + // Editing existing location: load its current values. + self.location = location; + } + } else { + self.location = Default::default(); + self.location.location_platform = LocationPlatform::Other; + } true } Msg::SetLocationPlatformsFetchState(fetch_state) => { @@ -133,9 +158,9 @@ impl Component for LocationsFormComponent { .send_message(Msg::SetLocationPlatformsFetchState(FetchAction::Fetching)); false } - Msg::SetLocationPushState(fetch_state) => { - self.push_location.apply(fetch_state); - match self.push_location.as_ref().state() { + Msg::SetLocationCreateState(fetch_state) => { + self.create_location.apply(fetch_state); + match self.create_location.as_ref().state() { FetchState::NotFetching(_) => false, FetchState::Fetching(_) => false, FetchState::Fetched(body) => match &body.data.create_location { @@ -144,12 +169,14 @@ impl Component for LocationsFormComponent { let mut locations: Vec = ctx.props().locations.clone().unwrap_or_default(); locations.push(location); - ctx.props().update_locations.emit(Some(locations)); - ctx.link().send_message(Msg::ToggleAddFormDisplay(false)); + ctx.props().update_locations.emit(()); + ctx.link() + .send_message(Msg::ToggleModalFormDisplay(false, None)); true } None => { - ctx.link().send_message(Msg::ToggleAddFormDisplay(false)); + ctx.link() + .send_message(Msg::ToggleModalFormDisplay(false, None)); self.notification_bus.send(Request::NotificationBusMsg(( "Failed to save".to_string(), NotificationStatus::Danger, @@ -158,7 +185,8 @@ impl Component for LocationsFormComponent { } }, FetchState::Failed(_, err) => { - ctx.link().send_message(Msg::ToggleAddFormDisplay(false)); + ctx.link() + .send_message(Msg::ToggleModalFormDisplay(false, None)); self.notification_bus.send(Request::NotificationBusMsg(( ThothError::from(err).to_string(), NotificationStatus::Danger, @@ -169,21 +197,77 @@ impl Component for LocationsFormComponent { } Msg::CreateLocation => { let body = CreateLocationRequestBody { - variables: Variables { + variables: CreateVariables { publication_id: ctx.props().publication_id, - landing_page: self.new_location.landing_page.clone(), - full_text_url: self.new_location.full_text_url.clone(), - location_platform: self.new_location.location_platform.clone(), - canonical: self.new_location.canonical, + landing_page: self.location.landing_page.clone(), + full_text_url: self.location.full_text_url.clone(), + location_platform: self.location.location_platform.clone(), + canonical: self.location.canonical, }, ..Default::default() }; let request = CreateLocationRequest { body }; - self.push_location = Fetch::new(request); + self.create_location = Fetch::new(request); ctx.link() - .send_future(self.push_location.fetch(Msg::SetLocationPushState)); + .send_future(self.create_location.fetch(Msg::SetLocationCreateState)); ctx.link() - .send_message(Msg::SetLocationPushState(FetchAction::Fetching)); + .send_message(Msg::SetLocationCreateState(FetchAction::Fetching)); + false + } + Msg::SetLocationUpdateState(fetch_state) => { + self.update_location.apply(fetch_state); + match self.update_location.as_ref().state() { + FetchState::NotFetching(_) => false, + FetchState::Fetching(_) => false, + FetchState::Fetched(body) => match &body.data.update_location { + Some(_l) => { + ctx.props().update_locations.emit(()); + ctx.link() + .send_message(Msg::ToggleModalFormDisplay(false, None)); + // changed the return value to false below, but this doesn't fix the display + // issue where the page jumps during refresh when modal is exited + false + } + None => { + ctx.link() + .send_message(Msg::ToggleModalFormDisplay(false, None)); + self.notification_bus.send(Request::NotificationBusMsg(( + "Failed to save".to_string(), + NotificationStatus::Danger, + ))); + false + } + }, + FetchState::Failed(_, err) => { + ctx.link() + .send_message(Msg::ToggleModalFormDisplay(false, None)); + self.notification_bus.send(Request::NotificationBusMsg(( + ThothError::from(err).to_string(), + NotificationStatus::Danger, + ))); + false + } + } + } + Msg::UpdateLocation => { + let body = UpdateLocationRequestBody { + variables: UpdateVariables { + location_id: self.location.location_id, + publication_id: self.location.publication_id, + landing_page: self.location.landing_page.clone(), + full_text_url: self.location.full_text_url.clone(), + location_platform: self.location.location_platform.clone(), + canonical: self.location.canonical, + }, + ..Default::default() + }; + let request = UpdateLocationRequest { body }; + self.update_location = Fetch::new(request); + ctx.link() + .send_future(self.update_location.fetch(Msg::SetLocationUpdateState)); + ctx.link() + .send_message(Msg::SetLocationUpdateState(FetchAction::Fetching)); + false } Msg::SetLocationDeleteState(fetch_state) => { @@ -192,16 +276,8 @@ impl Component for LocationsFormComponent { FetchState::NotFetching(_) => false, FetchState::Fetching(_) => false, FetchState::Fetched(body) => match &body.data.delete_location { - Some(location) => { - let to_keep: Vec = ctx - .props() - .locations - .clone() - .unwrap_or_default() - .into_iter() - .filter(|l| l.location_id != location.location_id) - .collect(); - ctx.props().update_locations.emit(Some(to_keep)); + Some(_location) => { + ctx.props().update_locations.emit(()); true } None => { @@ -234,18 +310,14 @@ impl Component for LocationsFormComponent { .send_message(Msg::SetLocationDeleteState(FetchAction::Fetching)); false } - Msg::ChangeLandingPage(val) => self - .new_location - .landing_page - .neq_assign(val.to_opt_string()), - Msg::ChangeFullTextUrl(val) => self - .new_location - .full_text_url - .neq_assign(val.to_opt_string()), - Msg::ChangeLocationPlatform(code) => { - self.new_location.location_platform.neq_assign(code) + Msg::ChangeLandingPage(val) => { + self.location.landing_page.neq_assign(val.to_opt_string()) + } + Msg::ChangeFullTextUrl(val) => { + self.location.full_text_url.neq_assign(val.to_opt_string()) } - Msg::ChangeCanonical(val) => self.new_location.canonical.neq_assign(val), + Msg::ChangeLocationPlatform(code) => self.location.location_platform.neq_assign(code), + Msg::ChangeCanonical(val) => self.location.canonical.neq_assign(val), } } @@ -253,11 +325,11 @@ impl Component for LocationsFormComponent { let locations = ctx.props().locations.clone().unwrap_or_default(); let open_modal = ctx.link().callback(|e: MouseEvent| { e.prevent_default(); - Msg::ToggleAddFormDisplay(true) + Msg::ToggleModalFormDisplay(true, None) }); let close_modal = ctx.link().callback(|e: MouseEvent| { e.prevent_default(); - Msg::ToggleAddFormDisplay(false) + Msg::ToggleModalFormDisplay(false, None) }); html! {