From: Dessalines <dessalines@users.noreply.github.com>
Date: Wed, 26 Jan 2022 17:57:16 +0000 (-0500)
Subject: Removing the site creator, adding leave_admin. Fixes #1808 (#2052)
X-Git-Url: http://these/git/%7B%60%24%7BwebArchiveUrl%7D/%22%7B%7D/%22https:/hacktivis.me/static/git-favicon.png?a=commitdiff_plain;h=e36ad9d98474a0e36a2d64111aa31e20df152d43;p=lemmy.git

Removing the site creator, adding leave_admin. Fixes #1808 (#2052)

* Removing the site creator, adding leave_admin. Fixes #1808

* Making sure there's at least one admin. Fixing unit tests
---

diff --git a/crates/api/src/community.rs b/crates/api/src/community.rs
index c8b44ace..a1a44f09 100644
--- a/crates/api/src/community.rs
+++ b/crates/api/src/community.rs
@@ -44,7 +44,6 @@ use lemmy_db_schema::{
     },
     person::Person,
     post::Post,
-    site::Site,
   },
   traits::{Bannable, Blockable, Crud, Followable, Joinable},
 };
@@ -457,20 +456,7 @@ impl Perform for TransferCommunity {
     let local_user_view =
       get_local_user_view_from_jwt(&data.auth, context.pool(), context.secret()).await?;
 
-    let site_creator_id = blocking(context.pool(), move |conn| {
-      Site::read(conn, 1).map(|s| s.creator_id)
-    })
-    .await??;
-
-    let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??;
-
-    // Making sure the site creator, if an admin, is at the top
-    let creator_index = admins
-      .iter()
-      .position(|r| r.person.id == site_creator_id)
-      .context(location_info!())?;
-    let creator_person = admins.remove(creator_index);
-    admins.insert(0, creator_person);
+    let admins = blocking(context.pool(), PersonViewSafe::admins).await??;
 
     // Fetch the community mods
     let community_id = data.community_id;
diff --git a/crates/api/src/lib.rs b/crates/api/src/lib.rs
index 3cffe016..c7c24062 100644
--- a/crates/api/src/lib.rs
+++ b/crates/api/src/lib.rs
@@ -111,9 +111,7 @@ pub async fn match_websocket_operation(
     UserOperation::TransferCommunity => {
       do_websocket_operation::<TransferCommunity>(context, id, op, data).await
     }
-    UserOperation::TransferSite => {
-      do_websocket_operation::<TransferSite>(context, id, op, data).await
-    }
+    UserOperation::LeaveAdmin => do_websocket_operation::<LeaveAdmin>(context, id, op, data).await,
 
     // Community ops
     UserOperation::FollowCommunity => {
diff --git a/crates/api/src/local_user.rs b/crates/api/src/local_user.rs
index 9452e754..974beb6e 100644
--- a/crates/api/src/local_user.rs
+++ b/crates/api/src/local_user.rs
@@ -1,6 +1,5 @@
 use crate::{captcha_as_wav_base64, Perform};
 use actix_web::web::Data;
-use anyhow::Context;
 use bcrypt::verify;
 use captcha::{gen, Difficulty};
 use chrono::Duration;
@@ -51,7 +50,6 @@ use lemmy_db_views_actor::{
 };
 use lemmy_utils::{
   claims::Claims,
-  location_info,
   utils::{is_valid_display_name, is_valid_matrix_id, naive_from_unix},
   ConnectionId,
   LemmyError,
@@ -409,18 +407,7 @@ impl Perform for AddAdmin {
 
     blocking(context.pool(), move |conn| ModAdd::create(conn, &form)).await??;
 
-    let site_creator_id = blocking(context.pool(), move |conn| {
-      Site::read(conn, 1).map(|s| s.creator_id)
-    })
-    .await??;
-
-    let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??;
-    let creator_index = admins
-      .iter()
-      .position(|r| r.person.id == site_creator_id)
-      .context(location_info!())?;
-    let creator_person = admins.remove(creator_index);
-    admins.insert(0, creator_person);
+    let admins = blocking(context.pool(), PersonViewSafe::admins).await??;
 
     let res = AddAdminResponse { admins };
 
diff --git a/crates/api/src/site.rs b/crates/api/src/site.rs
index 04cd099e..c77b177f 100644
--- a/crates/api/src/site.rs
+++ b/crates/api/src/site.rs
@@ -1,6 +1,5 @@
 use crate::Perform;
 use actix_web::web::Data;
-use anyhow::Context;
 use diesel::NotFound;
 use lemmy_api_common::{
   blocking,
@@ -27,6 +26,7 @@ use lemmy_db_schema::{
   source::{
     local_user::{LocalUser, LocalUserForm},
     moderator::*,
+    person::Person,
     registration_application::{RegistrationApplication, RegistrationApplicationForm},
     site::Site,
   },
@@ -62,7 +62,7 @@ use lemmy_db_views_moderator::{
   mod_sticky_post_view::ModStickyPostView,
   mod_transfer_community_view::ModTransferCommunityView,
 };
-use lemmy_utils::{location_info, settings::structs::Settings, version, ConnectionId, LemmyError};
+use lemmy_utils::{settings::structs::Settings, version, ConnectionId, LemmyError};
 use lemmy_websocket::LemmyContext;
 
 #[async_trait::async_trait(?Send)]
@@ -462,7 +462,7 @@ async fn convert_response(
 }
 
 #[async_trait::async_trait(?Send)]
-impl Perform for TransferSite {
+impl Perform for LeaveAdmin {
   type Response = GetSiteResponse;
 
   #[tracing::instrument(skip(context, _websocket_id))]
@@ -471,44 +471,36 @@ impl Perform for TransferSite {
     context: &Data<LemmyContext>,
     _websocket_id: Option<ConnectionId>,
   ) -> Result<GetSiteResponse, LemmyError> {
-    let data: &TransferSite = self;
+    let data: &LeaveAdmin = self;
     let local_user_view =
       get_local_user_view_from_jwt(&data.auth, context.pool(), context.secret()).await?;
 
     is_admin(&local_user_view)?;
 
-    let read_site = blocking(context.pool(), Site::read_simple).await??;
-
-    // Make sure user is the creator
-    if read_site.creator_id != local_user_view.person.id {
-      return Err(LemmyError::from_message("not_an_admin"));
+    // Make sure there isn't just one admin (so if one leaves, there will still be one left)
+    let admins = blocking(context.pool(), PersonViewSafe::admins).await??;
+    if admins.len() == 1 {
+      return Err(LemmyError::from_message("cannot_leave_admin"));
     }
 
-    let new_creator_id = data.person_id;
-    let transfer_site = move |conn: &'_ _| Site::transfer(conn, new_creator_id);
-    blocking(context.pool(), transfer_site)
-      .await?
-      .map_err(LemmyError::from)
-      .map_err(|e| e.with_message("couldnt_update_site"))?;
+    let person_id = local_user_view.person.id;
+    blocking(context.pool(), move |conn| {
+      Person::leave_admin(conn, person_id)
+    })
+    .await??;
 
     // Mod tables
     let form = ModAddForm {
-      mod_person_id: local_user_view.person.id,
-      other_person_id: data.person_id,
-      removed: Some(false),
+      mod_person_id: person_id,
+      other_person_id: person_id,
+      removed: Some(true),
     };
 
     blocking(context.pool(), move |conn| ModAdd::create(conn, &form)).await??;
 
+    // Reread site and admins
     let site_view = blocking(context.pool(), SiteView::read).await??;
-
-    let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??;
-    let creator_index = admins
-      .iter()
-      .position(|r| r.person.id == site_view.creator.id)
-      .context(location_info!())?;
-    let creator_person = admins.remove(creator_index);
-    admins.insert(0, creator_person);
+    let admins = blocking(context.pool(), PersonViewSafe::admins).await??;
 
     let federated_instances = build_federated_instances(
       context.pool(),
diff --git a/crates/api_common/src/site.rs b/crates/api_common/src/site.rs
index 07f7e853..e3bc52a2 100644
--- a/crates/api_common/src/site.rs
+++ b/crates/api_common/src/site.rs
@@ -155,8 +155,7 @@ pub struct MyUserInfo {
 }
 
 #[derive(Debug, Serialize, Deserialize)]
-pub struct TransferSite {
-  pub person_id: PersonId,
+pub struct LeaveAdmin {
   pub auth: Sensitive<String>,
 }
 
diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs
index 043a31c2..3de2c3ab 100644
--- a/crates/api_crud/src/site/create.rs
+++ b/crates/api_crud/src/site/create.rs
@@ -62,7 +62,6 @@ impl PerformCrud for CreateSite {
       description,
       icon,
       banner,
-      creator_id: local_user_view.person.id,
       enable_downvotes: data.enable_downvotes,
       open_registration: data.open_registration,
       enable_nsfw: data.enable_nsfw,
diff --git a/crates/api_crud/src/site/read.rs b/crates/api_crud/src/site/read.rs
index 229d5939..ee777d2d 100644
--- a/crates/api_crud/src/site/read.rs
+++ b/crates/api_crud/src/site/read.rs
@@ -79,18 +79,7 @@ impl PerformCrud for GetSite {
       }
     };
 
-    let mut admins = blocking(context.pool(), PersonViewSafe::admins).await??;
-
-    // Make sure the site creator is the top admin
-    if let Some(site_view) = site_view.to_owned() {
-      let site_creator_id = site_view.creator.id;
-      // TODO investigate why this is sometimes coming back null
-      // Maybe user_.admin isn't being set to true?
-      if let Some(creator_index) = admins.iter().position(|r| r.person.id == site_creator_id) {
-        let creator_person = admins.remove(creator_index);
-        admins.insert(0, creator_person);
-      }
-    }
+    let admins = blocking(context.pool(), PersonViewSafe::admins).await??;
 
     let online = context
       .chat_server()
diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs
index bcd6a1a3..0a94062a 100644
--- a/crates/api_crud/src/site/update.rs
+++ b/crates/api_crud/src/site/update.rs
@@ -54,7 +54,6 @@ impl PerformCrud for EditSite {
     }
 
     let site_form = SiteForm {
-      creator_id: found_site.creator_id,
       name: data.name.to_owned().unwrap_or(found_site.name),
       sidebar,
       description,
diff --git a/crates/db_schema/src/aggregates/site_aggregates.rs b/crates/db_schema/src/aggregates/site_aggregates.rs
index 08d4dd01..b9489924 100644
--- a/crates/db_schema/src/aggregates/site_aggregates.rs
+++ b/crates/db_schema/src/aggregates/site_aggregates.rs
@@ -55,7 +55,6 @@ mod tests {
 
     let site_form = SiteForm {
       name: "test_site".into(),
-      creator_id: inserted_person.id,
       sidebar: None,
       description: None,
       icon: None,
@@ -133,7 +132,12 @@ mod tests {
     let community_num_deleted = Community::delete(&conn, inserted_community.id).unwrap();
     assert_eq!(1, community_num_deleted);
 
-    let after_delete = SiteAggregates::read(&conn);
-    assert!(after_delete.is_err());
+    // Site should still exist, it can without a site creator.
+    let after_delete_creator = SiteAggregates::read(&conn);
+    assert!(after_delete_creator.is_ok());
+
+    Site::delete(&conn, 1).unwrap();
+    let after_delete_site = SiteAggregates::read(&conn);
+    assert!(after_delete_site.is_err());
   }
 }
diff --git a/crates/db_schema/src/impls/person.rs b/crates/db_schema/src/impls/person.rs
index 7e756543..17833253 100644
--- a/crates/db_schema/src/impls/person.rs
+++ b/crates/db_schema/src/impls/person.rs
@@ -274,6 +274,12 @@ impl Person {
   pub fn is_banned(&self) -> bool {
     is_banned(self.banned, self.ban_expires)
   }
+
+  pub fn leave_admin(conn: &PgConnection, person_id: PersonId) -> Result<Self, Error> {
+    diesel::update(person.find(person_id))
+      .set(admin.eq(false))
+      .get_result::<Self>(conn)
+  }
 }
 
 impl PersonSafe {
diff --git a/crates/db_schema/src/impls/site.rs b/crates/db_schema/src/impls/site.rs
index af0e8153..8a84bdfb 100644
--- a/crates/db_schema/src/impls/site.rs
+++ b/crates/db_schema/src/impls/site.rs
@@ -1,4 +1,4 @@
-use crate::{naive_now, newtypes::PersonId, source::site::*, traits::Crud};
+use crate::{source::site::*, traits::Crud};
 use diesel::{dsl::*, result::Error, *};
 
 impl Crud for Site {
@@ -27,13 +27,6 @@ impl Crud for Site {
 }
 
 impl Site {
-  pub fn transfer(conn: &PgConnection, new_creator_id: PersonId) -> Result<Site, Error> {
-    use crate::schema::site::dsl::*;
-    diesel::update(site.find(1))
-      .set((creator_id.eq(new_creator_id), updated.eq(naive_now())))
-      .get_result::<Self>(conn)
-  }
-
   pub fn read_simple(conn: &PgConnection) -> Result<Self, Error> {
     use crate::schema::site::dsl::*;
     site.first::<Self>(conn)
diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs
index c01114d9..681452d9 100644
--- a/crates/db_schema/src/schema.rs
+++ b/crates/db_schema/src/schema.rs
@@ -441,7 +441,6 @@ table! {
         id -> Int4,
         name -> Varchar,
         sidebar -> Nullable<Text>,
-        creator_id -> Int4,
         published -> Timestamp,
         updated -> Nullable<Timestamp>,
         enable_downvotes -> Bool,
@@ -648,7 +647,6 @@ joinable!(post_read -> post (post_id));
 joinable!(post_report -> post (post_id));
 joinable!(post_saved -> person (person_id));
 joinable!(post_saved -> post (post_id));
-joinable!(site -> person (creator_id));
 joinable!(site_aggregates -> site (site_id));
 joinable!(email_verification -> local_user (local_user_id));
 joinable!(registration_application -> local_user (local_user_id));
diff --git a/crates/db_schema/src/source/site.rs b/crates/db_schema/src/source/site.rs
index f99ffd88..01c5bc16 100644
--- a/crates/db_schema/src/source/site.rs
+++ b/crates/db_schema/src/source/site.rs
@@ -1,7 +1,4 @@
-use crate::{
-  newtypes::{DbUrl, PersonId},
-  schema::site,
-};
+use crate::{newtypes::DbUrl, schema::site};
 use serde::{Deserialize, Serialize};
 
 #[derive(Queryable, Identifiable, PartialEq, Debug, Clone, Serialize, Deserialize)]
@@ -10,7 +7,6 @@ pub struct Site {
   pub id: i32,
   pub name: String,
   pub sidebar: Option<String>,
-  pub creator_id: PersonId,
   pub published: chrono::NaiveDateTime,
   pub updated: Option<chrono::NaiveDateTime>,
   pub enable_downvotes: bool,
@@ -30,7 +26,6 @@ pub struct Site {
 #[table_name = "site"]
 pub struct SiteForm {
   pub name: String,
-  pub creator_id: PersonId,
   pub sidebar: Option<Option<String>>,
   pub updated: Option<chrono::NaiveDateTime>,
   pub enable_downvotes: Option<bool>,
diff --git a/crates/db_views/src/site_view.rs b/crates/db_views/src/site_view.rs
index 3c94975a..e77d7c6c 100644
--- a/crates/db_views/src/site_view.rs
+++ b/crates/db_views/src/site_view.rs
@@ -1,38 +1,24 @@
 use diesel::{result::Error, *};
 use lemmy_db_schema::{
   aggregates::site_aggregates::SiteAggregates,
-  schema::{person, site, site_aggregates},
-  source::{
-    person::{Person, PersonSafe},
-    site::Site,
-  },
-  traits::ToSafe,
+  schema::{site, site_aggregates},
+  source::site::Site,
 };
 use serde::{Deserialize, Serialize};
 
 #[derive(Debug, Serialize, Deserialize, Clone)]
 pub struct SiteView {
   pub site: Site,
-  pub creator: PersonSafe,
   pub counts: SiteAggregates,
 }
 
 impl SiteView {
   pub fn read(conn: &PgConnection) -> Result<Self, Error> {
-    let (site, creator, counts) = site::table
-      .inner_join(person::table)
+    let (site, counts) = site::table
       .inner_join(site_aggregates::table)
-      .select((
-        site::all_columns,
-        Person::safe_columns_tuple(),
-        site_aggregates::all_columns,
-      ))
-      .first::<(Site, PersonSafe, SiteAggregates)>(conn)?;
+      .select((site::all_columns, site_aggregates::all_columns))
+      .first::<(Site, SiteAggregates)>(conn)?;
 
-    Ok(SiteView {
-      site,
-      creator,
-      counts,
-    })
+    Ok(SiteView { site, counts })
   }
 }
diff --git a/crates/websocket/src/lib.rs b/crates/websocket/src/lib.rs
index e5c2730f..32456218 100644
--- a/crates/websocket/src/lib.rs
+++ b/crates/websocket/src/lib.rs
@@ -136,7 +136,7 @@ pub enum UserOperation {
   MarkAllAsRead,
   SaveUserSettings,
   TransferCommunity,
-  TransferSite,
+  LeaveAdmin,
   PasswordReset,
   PasswordChange,
   MarkPrivateMessageAsRead,
diff --git a/migrations/2022-01-20-160328_remove_site_creator/down.sql b/migrations/2022-01-20-160328_remove_site_creator/down.sql
new file mode 100644
index 00000000..8195f719
--- /dev/null
+++ b/migrations/2022-01-20-160328_remove_site_creator/down.sql
@@ -0,0 +1,14 @@
+--  Add the column back
+alter table site add column creator_id int references person on update cascade on delete cascade;
+
+-- Add the data, selecting the highest admin
+update site
+set creator_id = sub.id
+from (
+  select id from person
+  where admin = true
+  limit 1
+) as sub;
+
+-- Set to not null
+alter table site alter column creator_id set not null;
diff --git a/migrations/2022-01-20-160328_remove_site_creator/up.sql b/migrations/2022-01-20-160328_remove_site_creator/up.sql
new file mode 100644
index 00000000..78774445
--- /dev/null
+++ b/migrations/2022-01-20-160328_remove_site_creator/up.sql
@@ -0,0 +1,2 @@
+-- Drop the column
+alter table site drop column creator_id;
diff --git a/src/api_routes.rs b/src/api_routes.rs
index 5d5fe876..69bfe50b 100644
--- a/src/api_routes.rs
+++ b/src/api_routes.rs
@@ -19,7 +19,6 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimit) {
           // Admin Actions
           .route("", web::post().to(route_post_crud::<CreateSite>))
           .route("", web::put().to(route_post_crud::<EditSite>))
-          .route("/transfer", web::post().to(route_post::<TransferSite>))
           .route("/config", web::get().to(route_get::<GetSiteConfig>))
           .route("/config", web::put().to(route_post::<SaveSiteConfig>)),
       )
@@ -212,7 +211,8 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimit) {
           )
           .route("/report_count", web::get().to(route_get::<GetReportCount>))
           .route("/unread_count", web::get().to(route_get::<GetUnreadCount>))
-          .route("/verify_email", web::post().to(route_post::<VerifyEmail>)),
+          .route("/verify_email", web::post().to(route_post::<VerifyEmail>))
+          .route("/leave_admin", web::post().to(route_post::<LeaveAdmin>)),
       )
       // Admin Actions
       .service(