some fixes and reply improvements, remove automatic announcements, fix newlines missing in help

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## v0.2.7
+- Fix some wrong responses to admin commands
+- Remove automatic announcements from some admin commands
+- Send no reply to unauthorized admin commands
+- Fix swapped `/opengroup` and `/closegroup` descriptions in help
+
 ## v0.2.6
 - Allow boosting group hashtags when they are in a reply, except when it is private/DM 
   or contains actionable commands

Cargo.lock

@@ -328,7 +328,7 @@ checksum = "e88a8acf291dafb59c2d96e8f59828f3838bb1a70398823ade51a84de6a6deed"
 
 [[package]]
 name = "fedigroups"
-version = "0.2.6"
+version = "0.2.7"
 dependencies = [
  "anyhow",
  "clap",

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "fedigroups"
-version = "0.2.6"
+version = "0.2.7"
 authors = ["Ondřej Hruška <ondra@ondrovo.com>"]
 edition = "2018"
 publish = false

README.md

@@ -175,8 +175,8 @@ For group hashtags to work, the group user must follow all its members; otherwis
 - `/unban domain.tld` - lift a server ban
 - `/op user@domain` (alias `/admin`) - grant admin rights to a user
 - `/deop user@domain` (alias `/deadmin`) - revoke admin rights
-- `/opengroup` - make the group member-only
+- `/closegroup` - make the group member-only
-- `/closegroup` - make the group public-access
+- `/opengroup` - make the group public-access
 - `/add user@domain` (alias `/follow`) - add a member
 - `/remove user@domain` (alias `/remove`) - remove a member
 - `/add #hashtag` (alias `/follow`) - add a hashtag to the group

src/group_handler/handle_mention.rs

@@ -134,11 +134,11 @@ impl<'a> ProcessMention<'a> {
     }
 
     fn add_reply(&mut self, line: impl AsRef<str>) {
-        self.replies.push(line.as_ref().trim().to_string())
+        self.replies.push(line.as_ref().trim_matches(' ').to_string())
     }
 
     fn add_announcement<'t>(&mut self, line: impl AsRef<str>) {
-        self.announcements.push(line.as_ref().trim().to_string())
+        self.announcements.push(line.as_ref().trim_matches(' ').to_string())
     }
 
     async fn handle(mut self) -> Result<(), GroupError> {
@@ -196,7 +196,7 @@ impl<'a> ProcessMention<'a> {
                         self.cmd_remove_tag(tag).await;
                     }
                     StatusCommand::GrantAdmin(u) => {
-                        self.cmd_grant_member(&u).await
+                        self.cmd_grant_admin(&u).await
                             .log_error("Error handling grant-admin cmd");
                     }
                     StatusCommand::RemoveAdmin(u) => {
@@ -292,8 +292,9 @@ impl<'a> ProcessMention<'a> {
                 // tokio::time::sleep(DELAY_BEFORE_ACTION).await;
                 self.reblog_status().await;
             } else {
-                self.add_reply("You are not allowed to post to this group");
+                warn!("User @{} can't post to group!", self.status_acct);
             }
+            // Otherwise, don't react
         } else {
             debug!("Not OP, ignore mention");
         }
@@ -308,7 +309,7 @@ impl<'a> ProcessMention<'a> {
         if self.can_write {
             self.do_boost_prev_post = self.status.in_reply_to_id.is_some();
         } else {
-            self.add_reply("You are not allowed to share to this group");
+            warn!("User @{} can't share to group!", self.status_acct);
         }
     }
 
@@ -321,7 +322,7 @@ impl<'a> ProcessMention<'a> {
                     info!("Deleting group post #{}", parent_status_id);
                     self.client.delete_status(parent_status_id).await?;
                 } else {
-                    warn!("Only admin can delete announcements.");
+                    warn!("Only admin can delete posts made by the group user");
                 }
             } else {
                 if self.is_admin || parent_account_id == &self.status_user_id {
@@ -329,7 +330,9 @@ impl<'a> ProcessMention<'a> {
                     // User unboosting own post boosted by accident, or admin doing it
                     self.client.unreblog(parent_status_id).await?;
                 } else {
-                    self.add_reply("You don't have rights to do that.");
+                    warn!("Only the author and admins can undo reblogs");
+                    // XXX this means when someone /b's someone else's post to a group,
+                    //  they then can't reverse that (only admin or the post's author can)
                 }
             }
         }
@@ -351,9 +354,11 @@ impl<'a> ProcessMention<'a> {
                         self.add_reply(format!("Failed to ban user {}: {}", u, e));
                     }
                 }
+            } else {
+                self.add_reply(format!("No action, user {} is already banned", u));
             }
         } else {
-            self.add_reply("Only admins can manage user bans");
+            warn!("Ignore cmd, user not admin");
         }
         Ok(())
     }
@@ -371,9 +376,11 @@ impl<'a> ProcessMention<'a> {
                         unreachable!()
                     }
                 }
+            } else {
+                self.add_reply(format!("No action, user {} is not banned", u));
             }
         } else {
-            self.add_reply("Only admins can manage user bans");
+            warn!("Ignore cmd, user not admin");
         }
         Ok(())
     }
@@ -383,16 +390,17 @@ impl<'a> ProcessMention<'a> {
             if !self.config.is_server_banned(s) {
                 match self.config.ban_server(s, true) {
                     Ok(_) => {
-                        self.add_announcement(format!("Server \"{}\" has been banned.", s));
                         self.add_reply(format!("Server {} banned from group!", s));
                     }
                     Err(e) => {
                         self.add_reply(format!("Failed to ban server {}: {}", s, e));
                     }
                 }
+            } else {
+                self.add_reply(format!("No action, server {} already banned", s));
             }
         } else {
-            self.add_reply("Only admins can manage server bans");
+            warn!("Ignore cmd, user not admin");
         }
     }
 
@@ -401,22 +409,24 @@ impl<'a> ProcessMention<'a> {
             if self.config.is_server_banned(s) {
                 match self.config.ban_server(s, false) {
                     Ok(_) => {
-                        self.add_announcement(format!("Server \"{}\" has been un-banned.", s));
                         self.add_reply(format!("Server {} un-banned!", s));
                     }
-                    Err(_) => {
+                    Err(e) => {
-                        unreachable!()
+                        self.add_reply(format!("Unexpected error occured: {}", e));
                     }
                 }
+            } else {
+                self.add_reply(format!("No action, server {} is not banned", s));
             }
         } else {
-            self.add_reply("Only admins can manage server bans");
+            warn!("Ignore cmd, user not admin");
         }
     }
 
     async fn cmd_add_member(&mut self, user: &str) -> Result<(), GroupError> {
         let u = normalize_acct(user, &self.group_acct)?;
         if self.is_admin {
+            // Allow even if the user is already a member - that will trigger re-follow
             match self.config.set_member(&u, true) {
                 Ok(_) => {
                     self.add_reply(format!("User {} added to the group!", u));
@@ -428,7 +438,7 @@ impl<'a> ProcessMention<'a> {
                 }
             }
         } else {
-            self.add_reply("Only admins can manage members");
+            warn!("Ignore cmd, user not admin");
         }
         Ok(())
     }
@@ -442,26 +452,26 @@ impl<'a> ProcessMention<'a> {
                     self.unfollow_by_acct(&u).await
                         .log_error("Failed to unfollow removed user");
                 }
-                Err(_) => {
+                Err(e) => {
-                    unreachable!()
+                    self.add_reply(format!("Unexpected error occured: {}", e));
                 }
             }
         } else {
-            self.add_reply("Only admins can manage members");
+            warn!("Ignore cmd, user not admin");
         }
         Ok(())
     }
 
     async fn cmd_add_tag(&mut self, tag: String) {
         if self.is_admin {
-            if self.config.is_tag_followed(&tag) {
+            if !self.config.is_tag_followed(&tag) {
+                self.config.add_tag(&tag);
                 self.add_reply(format!("Tag \"{}\" added to the group!", tag));
             } else {
-                self.config.add_tag(&tag);
+                self.add_reply(format!("No action, \"{}\" is already a group tag", tag));
-                self.add_reply(format!("Tag \"{}\" was already in group!", tag));
             }
         } else {
-            self.add_reply("Only admins can manage group tags");
+            warn!("Ignore cmd, user not admin");
         }
     }
 
@@ -471,14 +481,14 @@ impl<'a> ProcessMention<'a> {
                 self.config.remove_tag(&tag);
                 self.add_reply(format!("Tag \"{}\" removed from the group!", tag));
             } else {
-                self.add_reply(format!("Tag \"{}\" was not in group!", tag));
+                self.add_reply(format!("No action, \"{}\" is not a group tag", tag));
             }
         } else {
-            self.add_reply("Only admins can manage group tags");
+            warn!("Ignore cmd, user not admin");
         }
     }
 
-    async fn cmd_grant_member(&mut self, user: &str) -> Result<(), GroupError> {
+    async fn cmd_grant_admin(&mut self, user: &str) -> Result<(), GroupError> {
         let u = normalize_acct(user, &self.group_acct)?;
         if self.is_admin {
             if !self.config.is_admin(&u) {
@@ -488,7 +498,6 @@ impl<'a> ProcessMention<'a> {
                         let _ = self.config.set_member(&u, true);
 
                         self.add_reply(format!("User {} is now a group admin!", u));
-                        self.add_announcement(format!("User @{} can now manage this group!", u));
                     }
                     Err(e) => {
                         self.add_reply(format!(
@@ -497,9 +506,11 @@ impl<'a> ProcessMention<'a> {
                         ));
                     }
                 }
+            } else {
+                self.add_reply(format!("No action, \"{}\" is admin already", u));
             }
         } else {
-            self.add_reply("Only admins can manage admins");
+            warn!("Ignore cmd, user not admin");
         }
         Ok(())
     }
@@ -511,15 +522,16 @@ impl<'a> ProcessMention<'a> {
                 match self.config.set_admin(&u, false) {
                     Ok(_) => {
                         self.add_reply(format!("User {} is no longer a group admin!", u));
-                        self.add_announcement(format!("User @{} no longer manages this group.", u));
                     }
                     Err(e) => {
                         self.add_reply(format!("Failed to revoke {}'s group admin: {}", u, e));
                     }
                 }
+            } else {
+                self.add_reply(format!("No action, user {} is not admin", u));
             }
         } else {
-            self.add_reply("Only admins can manage admins");
+            warn!("Ignore cmd, user not admin");
         }
         Ok(())
     }
@@ -529,10 +541,11 @@ impl<'a> ProcessMention<'a> {
             if self.config.is_member_only() {
                 self.config.set_member_only(false);
                 self.add_reply("Group changed to open-access");
-                self.add_announcement("This group is now open-access!");
+            } else {
+                self.add_reply("No action, group is open-access already");
             }
         } else {
-            self.add_reply("Only admins can set group options");
+            warn!("Ignore cmd, user not admin");
         }
     }
 
@@ -541,33 +554,34 @@ impl<'a> ProcessMention<'a> {
             if !self.config.is_member_only() {
                 self.config.set_member_only(true);
                 self.add_reply("Group changed to member-only");
-                self.add_announcement("This group is now member-only!");
+            } else {
+                self.add_reply("No action, group is member-only already");
             }
         } else {
-            self.add_reply("Only admins can set group options");
+            warn!("Ignore cmd, user not admin");
         }
     }
 
     async fn cmd_help(&mut self) {
         self.want_markdown = true;
 
-        if self.config.is_member_only() {
+        let membership_line = if self.is_admin {
-            self.add_reply("This is a member-only group. ");
+            "*You are an admin.*"
-        } else {
-            self.add_reply("This is a public-access group. ");
-        }
-
-        if self.is_admin {
-            self.add_reply("*You are an admin.*");
         } else if self.config.is_member(&self.status_acct) {
-            self.add_reply("*You are a member.*");
+            "*You are a member.*"
         } else if self.config.is_member_only() {
-            self.add_reply("*You are not a member, ask one of the admins to add you.*");
+            "*You are not a member, ask one of the admins to add you.*"
         } else {
-            self.add_reply("*You are not a member, follow or use /join to join the group.*");
+            "*You are not a member, follow or use /join to join the group.*"
+        };
+
+        if self.config.is_member_only() {
+            self.add_reply(format!("This is a member-only group. {}", membership_line));
+        } else {
+            self.add_reply(format!("This is a public-access group. {}", membership_line));
         }
 
-        self.add_reply("\n\
+        self.add_reply("\
             To share a post, @ the group user or use a group hashtag.\n\
             \n\
             **Supported commands:**\n\
@@ -600,8 +614,8 @@ impl<'a> ProcessMention<'a> {
                 `/unban x` - lift a ban\n\
                 `/admin user` - grant admin rights\n\
                 `/deadmin user` - revoke admin rights\n\
-                `/opengroup` - make member-only\n\
+                `/closegroup` - make member-only\n\
-                `/closegroup` - make public-access\n\
+                `/opengroup` - make public-access\n\
                 `/announce x` - make a public announcement");
         }
     }