feat: make group member add/remove response code less confusing; add tests

PluralKit.Bot/Commands/Groups.cs

@@ -457,7 +457,7 @@ namespace PluralKit.Bot
             }
             else return; // otherwise toAction "may be undefined"
 
-            await ctx.Reply(MiscUtils.GroupAddRemoveResponse<MemberId>(members, toAction, op));
+            await ctx.Reply(GroupAddRemoveResponseService.GenerateResponse(op, members.Count, 1, members.Count - existingMembersInGroup.Count, existingMembersInGroup.Count));
         }
 
         public async Task ListGroupMembers(Context ctx, PKGroup target)

PluralKit.Bot/Commands/MemberGroup.cs

@@ -55,7 +55,7 @@ namespace PluralKit.Bot
             }
             else return; // otherwise toAction "may be unassigned"
 
-            await ctx.Reply(MiscUtils.GroupAddRemoveResponse<GroupId>(groups, toAction, op));
+            await ctx.Reply(GroupAddRemoveResponseService.GenerateResponse(op, 1, groups.Count, groups.Count - existingGroups.Count, existingGroups.Count));
         }
 
         public async Task List(Context ctx, PKMember target)

PluralKit.Bot/Services/GroupAddRemoveResponseService.cs

@@ -0,0 +1,109 @@
+using System;
+
+using PluralKit.Core;
+
+namespace PluralKit.Bot
+{
+    public static class GroupAddRemoveResponseService
+    {
+        public static string GenerateResponse(Groups.AddRemoveOperation action, int memberCount, int groupCount, int actionedOn, int notActionedOn)
+            => new Response(action, memberCount, groupCount, actionedOn, notActionedOn).ToString();
+
+        private class Response
+        {
+            private readonly Groups.AddRemoveOperation _op;
+            
+            private readonly string _actionStr;
+            private readonly string _containStr;
+            private readonly string _emojiStr;
+
+            private readonly bool _memberPlural;
+            private readonly bool _groupPlural;
+            
+            private readonly int _actionedOn;
+            private readonly int _notActionedOn;
+
+            public Response(Groups.AddRemoveOperation action, int memberCount, int groupCount, int actionedOn,
+                            int notActionedOn)
+            {
+                _op = action;
+                
+                _actionStr = action == Groups.AddRemoveOperation.Add ? "added to" : "removed from";
+                _containStr = action == Groups.AddRemoveOperation.Add ? "in" : "not in";
+                _emojiStr = actionedOn > 0 ? Emojis.Success : Emojis.Error;
+
+                _memberPlural = memberCount > 1;
+                _groupPlural = groupCount > 1;
+                
+                // sanity checking: we can't add multiple groups to multiple members (at least for now)
+                if (_memberPlural && _groupPlural)
+                    throw new ArgumentOutOfRangeException();
+               
+                // sanity checking: we can't act/not act on a different number of entities than we have
+                if (_memberPlural && (actionedOn + notActionedOn) != memberCount)
+                    throw new ArgumentOutOfRangeException();
+                if (_groupPlural && (actionedOn + notActionedOn) != groupCount)
+                    throw new ArgumentOutOfRangeException();
+
+                _actionedOn = actionedOn;
+                _notActionedOn = notActionedOn;
+            }
+            
+            // name generators
+            private string MemberString(bool capitalize = false)
+                => capitalize
+                    ? (_memberPlural ? "Members" : "Member")
+                    : (_memberPlural ? "members" : "member");
+
+            private string MemberString(int count, bool capitalize = false)
+                => capitalize
+                    ? (count == 1 ? "Member" : "Members")
+                    : (count == 1 ? "member" : "members");
+            
+            private string GroupString() => _groupPlural ? "groups" : "group";
+
+            private string GroupString(int count)
+                => count == 1 ? "group" : "groups";
+
+            // string generators
+            
+            private string ResponseString()
+            {
+                if (_actionedOn > 0 && _notActionedOn > 0 && _memberPlural)
+                    return $"{_actionedOn} {MemberString(_actionedOn)} {_actionStr} {GroupString()}";
+                if (_actionedOn > 0 && _notActionedOn > 0 && _groupPlural)
+                    return $"{MemberString(capitalize: true)} {_actionStr} {_actionedOn} {GroupString(_actionedOn)}";
+                if (_notActionedOn == 0)
+                    return $"{MemberString(capitalize: true)} {_actionStr} {GroupString()}";
+                if (_actionedOn == 0)
+                    return $"{MemberString(capitalize: true)} not {_actionStr} {GroupString()}";
+
+                throw new ArgumentOutOfRangeException();
+            }
+
+            private string InfoMessage()
+            {
+                if (_notActionedOn == 0) return $"";
+
+                var msg = "";
+                if (_actionedOn > 0 && _memberPlural)
+                    msg += $"{_notActionedOn} {MemberString(_notActionedOn)}";
+                else
+                    msg += $"{MemberString()}";
+
+                msg += $" already {_containStr}";
+
+                if (_actionedOn > 0 && _groupPlural)
+                    msg += $" {_notActionedOn} {GroupString(_notActionedOn)}";
+                else
+                    msg += $" {GroupString()}";
+
+                return $" ({msg})";
+            }
+            
+            public string ToString() => $"{_emojiStr} {ResponseString()}{InfoMessage()}.";
+            
+            // |
+        }
+    }
+}

PluralKit.Bot/Utils/MiscUtils.cs

@@ -20,37 +20,6 @@ namespace PluralKit.Bot
         public static string ProxyTagsString(this PKMember member, string separator = ", ") => 
             string.Join(separator, member.ProxyTags.Select(t => t.ProxyString.AsCode()));
         
-        
-        private static String entityTerm<T>(int count, bool isTarget)
-        {
-            var ret = "";
-            ret += isTarget ? "Member" : "Group";
-            if ((
-                (typeof(T) == typeof(GroupId) && !isTarget) ||
-                (typeof(T) == typeof(MemberId) && isTarget)
-            ) && count > 1)
-                ret += "s";
-            return ret;
-        }
-
-        public static String GroupAddRemoveResponse<T>(List<T> entityList, List<T> actionedOn, Groups.AddRemoveOperation op)
-        {
-            var opStr = op == Groups.AddRemoveOperation.Add ? "added to" : "removed from";
-            var inStr = op == Groups.AddRemoveOperation.Add ? "in" : "not in";
-            var notActionedOn = entityList.Count - actionedOn.Count;
-
-            var groupNotActionedPosStr = typeof(T) == typeof(GroupId) ? notActionedOn.ToString() + " " : "";
-            var memberNotActionedPosStr = typeof(T) == typeof(MemberId) ? notActionedOn.ToString() + " " : "";
-            
-            if (actionedOn.Count == 0)
-                return $"{Emojis.Error} {entityTerm<T>(notActionedOn, true)} not {opStr} {entityTerm<T>(entityList.Count, false).ToLower()} ({entityTerm<T>(notActionedOn, true).ToLower()} already {inStr} {entityTerm<T>(entityList.Count, false).ToLower()}).";
-            else
-                if (notActionedOn == 0)
-                    return $"{Emojis.Success} {entityTerm<T>(actionedOn.Count, true)} {opStr} {entityTerm<T>(actionedOn.Count, false).ToLower()}.";
-                else
-                    return $"{Emojis.Success} {actionedOn.Count} {entityTerm<T>(actionedOn.Count, true).ToLower()} {opStr} {entityTerm<T>(actionedOn.Count, false).ToLower()} ({memberNotActionedPosStr}{entityTerm<T>(actionedOn.Count, true).ToLower()} already {inStr} {groupNotActionedPosStr}{entityTerm<T>(notActionedOn, false).ToLower()}).";
-        }
-
         public static bool IsOurProblem(this Exception e)
         {
             // This function filters out sporadic errors out of our control from being reported to Sentry

PluralKit.Tests/GroupAddRemoveResponseTests.cs

@@ -0,0 +1,185 @@
+using System;
+
+using PluralKit.Bot;
+using PluralKit.Core;
+
+using Xunit;
+
+namespace PluralKit.Tests
+{
+    public class GroupAddRemoveResponseTests
+    {
+        private static Func<Groups.AddRemoveOperation, int, int, int, int, string>
+            func = GroupAddRemoveResponseService.GenerateResponse;
+        private static Groups.AddRemoveOperation addOp = Groups.AddRemoveOperation.Add;
+        private static Groups.AddRemoveOperation removeOp = Groups.AddRemoveOperation.Remove;
+        private static string success = Emojis.Success;
+        private static string failure = Emojis.Error;
+
+        public class AddOp
+        {
+            public class MemberGroup
+            {
+                [Fact]
+                public void Success()
+                    => Assert.Equal(
+                        $"{success} Member added to group.",
+                        func(addOp, 1, 1, 1, 0)
+                    );
+
+                [Fact]
+                public void Failure()
+                    => Assert.Equal(
+                        $"{failure} Member not added to group (member already in group).",
+                        func(addOp, 1, 1, 0, 1)
+                    );
+            }
+
+            public class MemberGroups
+            {
+                [Fact]
+                public void Success()
+                    => Assert.Equal(
+                        $"{success} Member added to groups.",
+                        func(addOp, 1, 2, 2, 0)
+                    );
+
+                [Fact]
+                public void PartialSuccess1()
+                    => Assert.Equal(
+                      $"{success} Member added to 2 groups (member already in 1 group).",
+                      func(addOp, 1, 3, 2, 1)
+                    );
+
+                [Fact]
+                public void PartialSuccess2()
+                    => Assert.Equal(
+                      $"{success} Member added to 1 group (member already in 2 groups).",
+                      func(addOp, 1, 3, 1, 2)
+                    );
+                
+                [Fact]
+                public void Failure()
+                    => Assert.Equal(
+                        $"{failure} Member not added to groups (member already in groups).",
+                        func(addOp, 1, 2, 0, 2)
+                    );
+            }
+
+            public class MembersGroup
+            {
+                [Fact]
+                public void Success()
+                    => Assert.Equal(
+                        $"{success} Members added to group.",
+                        func(addOp, 2, 1, 2, 0)
+                    );
+
+                [Fact]
+                public void PartialSuccess1()
+                    => Assert.Equal(
+                      $"{success} 2 members added to group (1 member already in group).",
+                      func(addOp, 3, 1, 2, 1)
+                    );
+
+                [Fact]
+                public void PartialSuccess2()
+                    => Assert.Equal(
+                      $"{success} 1 member added to group (2 members already in group).",
+                      func(addOp, 3, 1, 1, 2)
+                    );
+                
+                [Fact]
+                public void Failure()
+                    => Assert.Equal(
+                        $"{failure} Members not added to group (members already in group).",
+                        func(addOp, 2, 1, 0, 2)
+                    );
+            }
+
+        }
+
+        public class RemoveOp
+        {
+            public class MemberGroup
+            {
+                [Fact]
+                public void Success()
+                    => Assert.Equal(
+                        $"{success} Member removed from group.",
+                        func(removeOp, 1, 1, 1, 0)
+                    );
+
+                [Fact]
+                public void Failure()
+                    => Assert.Equal(
+                        $"{failure} Member not removed from group (member already not in group).",
+                        func(removeOp, 1, 1, 0, 1)
+                    );
+            }
+
+            public class MemberGroups
+            {
+                [Fact]
+                public void Success()
+                    => Assert.Equal(
+                        $"{success} Member removed from groups.",
+                        func(removeOp, 1, 3, 3, 0)
+                    );
+
+                [Fact]
+                public void PartialSuccess1()
+                    => Assert.Equal(
+                        $"{success} Member removed from 1 group (member already not in 2 groups).",
+                        func(removeOp, 1, 3, 1, 2)
+                    );
+
+                [Fact]
+                public void PartialSuccess2()
+                    => Assert.Equal(
+                        $"{success} Member removed from 2 groups (member already not in 1 group).",
+                        func(removeOp, 1, 3, 2, 1)
+                    );
+                
+                [Fact]
+                public void Failure()
+                    => Assert.Equal(
+                        $"{failure} Member not removed from groups (member already not in groups).",
+                        func(removeOp, 1, 3, 0, 3)
+                    );
+            }
+
+            public class MembersGroup
+            {
+                [Fact]
+                public void Success()
+                    => Assert.Equal(
+                        $"{success} Members removed from group.",
+                        func(removeOp, 2, 1, 2, 0)
+                    );
+
+
+                public void PartialSuccess1()
+                    => Assert.Equal(
+                        $"{success} 1 member removed from group (2 members already not in group).",
+                        func(removeOp, 3, 1, 1, 2)
+                    );
+
+                [Fact]
+                public void PartialSuccess2()
+                    => Assert.Equal(
+                        $"{success} 2 members removed from group (1 member already not in group).",
+                        func(removeOp, 3, 1, 2, 1)
+                    );
+                
+                [Fact]
+                public void Failure()
+                    => Assert.Equal(
+                        $"{failure} Members not removed from group (members already not in group).",
+                        func(removeOp, 2, 1, 0, 2)
+                    );
+            }
+
+        }    
+    }
+}