From 603c38d60429c1b9ab7372ae77d8af323be101a4 Mon Sep 17 00:00:00 2001 From: Brendan O'Connell Date: Wed, 25 Sep 2024 17:12:15 +0200 Subject: [PATCH] Fix formatter and clippy errors --- thoth-api/src/graphql/model.rs | 3 +- thoth-api/src/model/location/crud.rs | 53 +++++++++++----------------- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/thoth-api/src/graphql/model.rs b/thoth-api/src/graphql/model.rs index ad2e068e..a162205d 100644 --- a/thoth-api/src/graphql/model.rs +++ b/thoth-api/src/graphql/model.rs @@ -1967,8 +1967,7 @@ impl MutationRoot { #[graphql(description = "Update an existing location with the specified values")] fn update_location( context: &Context, - #[graphql(description = "Values to apply to existing location")] - data: PatchLocation, + #[graphql(description = "Values to apply to existing location")] data: PatchLocation, ) -> FieldResult { context.token.jwt.as_ref().ok_or(ThothError::Unauthorised)?; diff --git a/thoth-api/src/model/location/crud.rs b/thoth-api/src/model/location/crud.rs index c9525009..29a2af39 100644 --- a/thoth-api/src/model/location/crud.rs +++ b/thoth-api/src/model/location/crud.rs @@ -126,9 +126,8 @@ impl Crud for Location { crate::model::publication::Publication::from_id(db, &self.publication_id)?.publisher_id(db) } - // `crud_methods!` cannot be used for update(), because - // we sometimes need to execute multiple statements in the same transaction - // for changing a non-canonical location to canonical. + // `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()?; @@ -158,42 +157,36 @@ impl Crud for Location { data: &PatchLocation, account_id: &Uuid, ) -> ThothResult { - // if changes to a location don't change its canonical or non-canonical status, perform a single update to that location. - if &data.canonical == &self.canonical { + // if changes to a location don't change its canonical or non-canonical status, only update that location. + if data.canonical == self.canonical { let mut connection = db.get()?; connection.transaction(|connection| { match diesel::update(location::table.find(&self.location_id)) .set(data) .get_result::(connection) { - // On success, create a new history table entry. - Ok(l) => match self.new_history_entry(account_id).insert(connection) { - Ok(_) => Ok(l), - Err(e) => Err(e), - }, - Err(e) => Err(ThothError::from(e)), + // On success, create a new history table entry. + Ok(l) => match self.new_history_entry(account_id).insert(connection) { + Ok(_) => Ok(l), + Err(e) => Err(e), + }, + Err(e) => Err(ThothError::from(e)), } }) // trying to change canonical location to non-canonical results in error. } else if self.canonical && (data.canonical != self.canonical) { - Err(ThothError::CanonicalLocationError.into()) + Err(ThothError::CanonicalLocationError) // if user changes a non-canonical location to canonical, perform two simultaneous updates: - // change the old canonical location to non-canonical, and change the old non-canonical location to canonical + // change the former canonical location to non-canonical, the former non-canonical location to canonical } else { - let canonical_location = self.get_canonical_location(db); - let final_canonical_location = match canonical_location { - Ok(location) => location, - Err(e) => { - return Err(ThothError::from(e).into()); - } - }; + let canonical_location = self.get_canonical_location(db).map_err(ThothError::from)?; let old_canonical_location = PatchLocation { - location_id: final_canonical_location.location_id, - publication_id: final_canonical_location.publication_id, - landing_page: final_canonical_location.landing_page.clone(), - full_text_url: final_canonical_location.full_text_url.clone(), - location_platform: final_canonical_location.location_platform.clone(), + location_id: canonical_location.location_id, + publication_id: canonical_location.publication_id, + landing_page: canonical_location.landing_page.clone(), + full_text_url: canonical_location.full_text_url.clone(), + location_platform: canonical_location.location_platform.clone(), canonical: false, }; let mut connection = db.get()?; @@ -203,7 +196,8 @@ impl Crud for Location { .set(old_canonical_location) .execute(connection)?; match diesel::update(location::table.find(&self.location_id)) - // Update the data from the currently non-canonical location to canonical + // Update the data from the currently non-canonical location to make it canonical, + // along with any other changes from PatchLocation .set(data) .get_result::(connection) { @@ -227,8 +221,6 @@ impl Crud for Location { } }) } - - } impl HistoryEntry for Location { @@ -284,10 +276,7 @@ impl NewLocation { } impl Location { - pub fn get_canonical_location( - &self, - db: &crate::db::PgPool - ) -> ThothResult { + pub fn get_canonical_location(&self, db: &crate::db::PgPool) -> ThothResult { let mut connection = db.get()?; let canonical_location = crate::schema::location::table .filter(crate::schema::location::publication_id.eq(self.publication_id))