Merged PR 177: Fix bug where team roles were ignored by !setRosters i…

…f they didn't have access to the channel

- Fix a bug where we ignored team roles that didn't have access to a the channel where the !setRosters command was called
- Bump version to 3.8.2

QuizBowlDiscordScoreTracker/Commands/AdminCommandHandler.cs

@@ -352,8 +352,6 @@ public async Task UnpairChannelAsync(ITextChannel textChannel)
             await this.Context.Channel.SendMessageAsync("Text and voice channel unpaired successfully");
         }
 
-        [SuppressMessage("Design", "CA1054:URI-like parameters should not be strings",
-            Justification = "Discord.Net can't parse the argument directly as a URI")]
         private async Task SetRostersFromRolesForSheets(string sheetsUrl, GoogleSheetsType type)
         {
             if (!(this.Context.Channel is IGuildChannel guildChannel))
@@ -384,7 +382,7 @@ await this.Context.Channel.SendMessageAsync(
                 return;
             }
 
-            ITeamManager teamManager = new ByRoleTeamManager(guildChannel, teamRolePrefix);
+            IByRoleTeamManager teamManager = new ByRoleTeamManager(guildChannel, teamRolePrefix);
             IGoogleSheetsGenerator generator = this.GoogleSheetsGeneratorFactory.Create(type);
             IResult<string> result = await generator.TryUpdateRosters(teamManager, sheetsUri);
 

QuizBowlDiscordScoreTracker/QuizBowlDiscordScoreTracker.csproj

@@ -3,7 +3,7 @@
   <PropertyGroup>
     <OutputType>Exe</OutputType>
     <TargetFramework>netcoreapp3.1</TargetFramework>
-    <Version>3.8.1</Version>
+    <Version>3.8.2</Version>
     <Authors>Alejandro Lopez-Lago</Authors>
     <Company />
     <Product>Quiz Bowl Discord Score Tracker</Product>

QuizBowlDiscordScoreTracker/Scoresheet/BaseGoogleSheetsGenerator.cs

@@ -159,32 +159,32 @@ public async Task<IResult<string>> TryCreateScoresheet(GameState game, Uri sheet
             return new SuccessResult<string>(message);
         }
 
-        public async Task<IResult<string>> TryUpdateRosters(ITeamManager teamManager, Uri sheetsUri)
+        public async Task<IResult<string>> TryUpdateRosters(IByRoleTeamManager teamManager, Uri sheetsUri)
         {
             Verify.IsNotNull(teamManager, nameof(teamManager));
             Verify.IsNotNull(sheetsUri, nameof(sheetsUri));
 
-            IReadOnlyDictionary<string, string> teamIdToNames = await teamManager.GetTeamIdToNames();
-
-            // Convert it to an array so we don't have to keep re-evaluating the GroupBy
-            IEnumerable<PlayerTeamPair> playerTeamPairs = await teamManager.GetKnownPlayers();
-            IGrouping<string, PlayerTeamPair>[] groupings = playerTeamPairs
-                .GroupBy(pair => pair.TeamId)
-                .Where(grouping => grouping.Any())
-                .ToArray();
+            IEnumerable<IGrouping<string, PlayerTeamPair>> groupings = await teamManager.GetPlayerTeamPairsForServer();
 
             if (groupings.Any(grouping => grouping.Count() > this.PlayersPerTeamLimit))
             {
                 return CreateFailureResult(
                     $"Rosters can only support up to {this.PlayersPerTeamLimit} players per team.");
             }
 
-            if (groupings.Length > this.TeamsLimit)
+            int groupingsCount = groupings.Count();
+
+            if (groupingsCount == 0)
+            {
+                return CreateFailureResult($"No teams were found, so the rosters remain unchanged.");
+            }
+            else if (groupingsCount > this.TeamsLimit)
             {
                 return CreateFailureResult(
                     $"Rosters can only support up to {this.TeamsLimit} teams.");
             }
 
+            IReadOnlyDictionary<string, string> teamIdToNames = await teamManager.GetTeamIdToNamesForServer();
             IResult<List<ValueRange>> rangesResult = this.GetUpdateRangesForRoster(teamIdToNames, groupings);
             if (!rangesResult.Success)
             {

QuizBowlDiscordScoreTracker/Scoresheet/IGoogleSheetsGenerator.cs

@@ -32,6 +32,6 @@ public interface IGoogleSheetsGenerator
         /// <param name="teamManager">Team Manager used in the server or game.</param>
         /// <param name="sheetsUri">URI to the Google Sheet with the worksheet for the tournament's rosters</param>
         /// <returns>A result with an error code if the update failed, or a result with an empty string on success</returns>
-        Task<IResult<string>> TryUpdateRosters(ITeamManager teamManager, Uri sheetsUri);
+        Task<IResult<string>> TryUpdateRosters(IByRoleTeamManager teamManager, Uri sheetsUri);
     }
 }

QuizBowlDiscordScoreTracker/TeamManager/ByRoleTeamManager.cs

@@ -7,7 +7,7 @@
 
 namespace QuizBowlDiscordScoreTracker.TeamManager
 {
-    public class ByRoleTeamManager : ITeamManager, IByRoleTeamManager
+    public class ByRoleTeamManager : IByRoleTeamManager
     {
         private readonly object teamIdToNameLock = new object();
 
@@ -31,19 +31,25 @@ public ByRoleTeamManager(IGuildChannel channel, string teamRolePrefix)
 
         private string TeamRolePrefix { get; }
 
-        private IDictionary<string, string> TeamIdToName { get; set; }
+        private IReadOnlyDictionary<string, string> ChannelTeamIdToName { get; set; }
 
-        public async Task<IEnumerable<PlayerTeamPair>> GetKnownPlayers()
+        private IReadOnlyDictionary<string, string> ServerTeamIdToName { get; set; }
+
+        public Task<IEnumerable<PlayerTeamPair>> GetKnownPlayers()
         {
-            IReadOnlyCollection<IGuildUser> users = await this.Guild.GetUsersAsync();
-            return users
-                .Select(user => new Tuple<ulong, ulong, string>(
-                    user.Id,
-                    user.RoleIds.FirstOrDefault(id => this.TeamIdToName.ContainsKey(id.ToString(CultureInfo.InvariantCulture))),
-                    user.Nickname ?? user.Username))
-                .Where(kvp => kvp.Item2 != default)
-                .Select(tuple => new PlayerTeamPair(
-                    tuple.Item1, tuple.Item3, tuple.Item2.ToString(CultureInfo.InvariantCulture)));
+            return this.GetKnownPlayers(this.ChannelTeamIdToName);
+        }
+
+        public async Task<IEnumerable<IGrouping<string, PlayerTeamPair>>> GetPlayerTeamPairsForServer()
+        {
+            IReadOnlyDictionary<string, string> teamIdToNames = this.ServerTeamIdToName;
+            IEnumerable<PlayerTeamPair> playerTeamPairs = await this.GetKnownPlayers(this.ServerTeamIdToName);
+            IGrouping<string, PlayerTeamPair>[] groupings = playerTeamPairs
+                .GroupBy(pair => pair.TeamId)
+                .Where(grouping => grouping.Any())
+                .ToArray();
+
+            return groupings;
         }
 
         public async Task<string> GetTeamIdOrNull(ulong userId)
@@ -57,21 +63,38 @@ public async Task<string> GetTeamIdOrNull(ulong userId)
             lock (this.teamIdToNameLock)
             {
                 ulong matchingRoleId = user.RoleIds.FirstOrDefault(
-                    id => this.TeamIdToName.ContainsKey(id.ToString(CultureInfo.InvariantCulture)));
+                    id => this.ChannelTeamIdToName.ContainsKey(id.ToString(CultureInfo.InvariantCulture)));
                 return matchingRoleId == default ? null : matchingRoleId.ToString(CultureInfo.InvariantCulture);
             }
         }
 
         public Task<IReadOnlyDictionary<string, string>> GetTeamIdToNames()
         {
-            IReadOnlyDictionary<string, string> teamIdToName = (IReadOnlyDictionary<string, string>)this.TeamIdToName;
-            return Task.FromResult(teamIdToName);
+            return Task.FromResult(this.ChannelTeamIdToName);
+        }
+
+        public Task<IReadOnlyDictionary<string, string>> GetTeamIdToNamesForServer()
+        {
+            return Task.FromResult(this.ServerTeamIdToName);
         }
 
         public string ReloadTeamRoles()
         {
             this.InitiailzeTeamIdToName();
-            return $@"Team roles reloaded. There are now {this.TeamIdToName.Count} team(s)";
+            return $@"Team roles reloaded. There are now {this.ChannelTeamIdToName.Count} team(s)";
+        }
+
+        private async Task<IEnumerable<PlayerTeamPair>> GetKnownPlayers(IReadOnlyDictionary<string, string> teamIdToName)
+        {
+            IReadOnlyCollection<IGuildUser> users = await this.Guild.GetUsersAsync();
+            return users
+                .Select(user => new Tuple<ulong, ulong, string>(
+                    user.Id,
+                    user.RoleIds.FirstOrDefault(id => teamIdToName.ContainsKey(id.ToString(CultureInfo.InvariantCulture))),
+                    user.Nickname ?? user.Username))
+                .Where(kvp => kvp.Item2 != default)
+                .Select(tuple => new PlayerTeamPair(
+                    tuple.Item1, tuple.Item3, tuple.Item2.ToString(CultureInfo.InvariantCulture)));
         }
 
         private void InitiailzeTeamIdToName()
@@ -82,8 +105,15 @@ private void InitiailzeTeamIdToName()
                 PermValue everyoneViewPermissions = everyonePermissions?.ViewChannel ?? PermValue.Inherit;
                 PermValue everyoneSendPermissions = everyonePermissions?.SendMessages ?? PermValue.Inherit;
 
-                this.TeamIdToName = this.Guild.Roles
-                    .Where(role => role.Name.StartsWith(this.TeamRolePrefix, StringComparison.InvariantCultureIgnoreCase))
+                IEnumerable<IRole> teamRoles = this.Guild.Roles
+                    .Where(role => role.Name.StartsWith(this.TeamRolePrefix, StringComparison.InvariantCultureIgnoreCase));
+
+                this.ServerTeamIdToName = teamRoles
+                    .ToDictionary(
+                        role => role.Id.ToString(CultureInfo.InvariantCulture),
+                        role => role.Name.Substring(this.TeamRolePrefix.Length).Trim());
+
+                this.ChannelTeamIdToName = teamRoles
                     .Where(role =>
                     {
                         // Players need both View and Send permissions to play, so make sure either the role or the

QuizBowlDiscordScoreTracker/TeamManager/IByRoleTeamManager.cs

@@ -1,16 +1,24 @@
-using System;
-using System.Collections.Generic;
+using System.Collections.Generic;
 using System.Linq;
 using System.Threading.Tasks;
 
 namespace QuizBowlDiscordScoreTracker.TeamManager
 {
-    public interface IByRoleTeamManager
+    public interface IByRoleTeamManager : ITeamManager
     {
+        Task<IReadOnlyDictionary<string, string>> GetTeamIdToNamesForServer();
+
         /// <summary>
         /// Reloads teams from the roles
         /// </summary>
         /// <returns>Returns the message to report after the roles were reloaded</returns>
         string ReloadTeamRoles();
+
+        /// <summary>
+        /// Gets a grouping of every player to their team in the server. This will ignore channel permissions for
+        /// learning who is on a team
+        /// </summary>
+        /// <returns>A grouping of team names to PlayerTeamPairs for every team in the server.</returns>
+        Task<IEnumerable<IGrouping<string, PlayerTeamPair>>> GetPlayerTeamPairsForServer();
     }
 }

QuizBowlDiscordScoreTrackerUnitTests/AdminCommandHandlerTests.cs

@@ -493,7 +493,7 @@ private async Task SetRostersFromRolesForGoogleSheetsFails(
 
             Mock<IGoogleSheetsGenerator> mockGenerator = new Mock<IGoogleSheetsGenerator>();
             mockGenerator
-                .Setup(generator => generator.TryUpdateRosters(It.IsAny<ITeamManager>(), It.IsAny<Uri>()))
+                .Setup(generator => generator.TryUpdateRosters(It.IsAny<IByRoleTeamManager>(), It.IsAny<Uri>()))
                 .Returns(Task.FromResult<IResult<string>>(new FailureResult<string>(errorMessage)))
                 .Verifiable();
 
@@ -521,7 +521,7 @@ private async Task SetRostersFromRolesForGoogleSheetsSucceeds(
         {
             Mock<IGoogleSheetsGenerator> mockGenerator = new Mock<IGoogleSheetsGenerator>();
             mockGenerator
-                .Setup(generator => generator.TryUpdateRosters(It.IsAny<ITeamManager>(), It.IsAny<Uri>()))
+                .Setup(generator => generator.TryUpdateRosters(It.IsAny<IByRoleTeamManager>(), It.IsAny<Uri>()))
                 .Returns(Task.FromResult<IResult<string>>(new SuccessResult<string>(string.Empty)))
                 .Verifiable();
 
@@ -549,7 +549,7 @@ private async Task SetRostersFromRolesForGoogleSheetsWithoutByRoleTeamsFails(
         {
             Mock<IGoogleSheetsGenerator> mockGenerator = new Mock<IGoogleSheetsGenerator>();
             mockGenerator
-                .Setup(generator => generator.TryUpdateRosters(It.IsAny<ITeamManager>(), It.IsAny<Uri>()))
+                .Setup(generator => generator.TryUpdateRosters(It.IsAny<IByRoleTeamManager>(), It.IsAny<Uri>()))
                 .Returns(Task.FromResult<IResult<string>>(new SuccessResult<string>(string.Empty)))
                 .Verifiable();
 
@@ -572,7 +572,7 @@ private async Task SetRostersFromRolesForGoogleSheetsWithBadUrlFails(
         {
             Mock<IGoogleSheetsGenerator> mockGenerator = new Mock<IGoogleSheetsGenerator>();
             mockGenerator
-                .Setup(generator => generator.TryUpdateRosters(It.IsAny<ITeamManager>(), It.IsAny<Uri>()))
+                .Setup(generator => generator.TryUpdateRosters(It.IsAny<IByRoleTeamManager>(), It.IsAny<Uri>()))
                 .Returns(Task.FromResult<IResult<string>>(new SuccessResult<string>(string.Empty)));
 
             Mock<IGoogleSheetsGeneratorFactory> mockFactory = new Mock<IGoogleSheetsGeneratorFactory>();