Saving scorecards in production is slow and causes SqlTimeoutExceptio…

…n. The slowest part is calculating probability. Change the calculation so it can be done at read time, removing the slowest part of the save. #619

Stoolball.Data.SqlServer.IntegrationTests/SqlServerIntegrationTestsRepository.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Collections.Generic;
 using System.Data;
 using System.Globalization;
 using Dapper;
@@ -99,18 +98,13 @@ public void CreateTestData(TestData data)
                 }
             }
 
-            var probabilities = new Dictionary<Guid, int>();
             foreach (var match in data.Matches)
             {
                 CreateMatch(match);
 
                 var statisticsRecords = _playerInMatchStatisticsBuilder.BuildStatisticsForMatch(match);
                 foreach (var record in statisticsRecords)
                 {
-                    var random = new Random();
-                    if (!probabilities.ContainsKey(record.PlayerIdentityId)) { probabilities.Add(record.PlayerIdentityId, random.Next(-999, 999)); }
-                    record.Probability = probabilities[record.PlayerIdentityId];
-
                     CreatePlayerInMatchStatisticsRecord(record);
                 }
             }
@@ -778,7 +772,7 @@ public void CreatePlayerInMatchStatisticsRecord(PlayerInMatchStatisticsRecord re
                      BowlingFiguresId, OverNumberOfFirstOverBowled, BallsBowled, Overs, Maidens, NoBalls, Wides, RunsConceded, HasRunsConceded, Wickets, WicketsWithBowling, 
                      WonToss, BattedFirst, PlayerInningsNumber, PlayerInningsId, BattingPosition, DismissalType, PlayerWasDismissed, BowledByPlayerIdentityId, BowledByPlayerIdentityName, BowledByPlayerRoute, 
                      CaughtByPlayerIdentityId, CaughtByPlayerIdentityName, CaughtByPlayerRoute, RunOutByPlayerIdentityId, RunOutByPlayerIdentityName, RunOutByPlayerRoute, 
-                     RunsScored, BallsFaced, Catches, RunOuts, WonMatch, PlayerOfTheMatch, Probability)
+                     RunsScored, BallsFaced, Catches, RunOuts, WonMatch, PlayerOfTheMatch)
                     VALUES
                     (@PlayerInMatchStatisticsId, @PlayerId, @PlayerIdentityId, 
                         (SELECT PlayerIdentityName FROM {Tables.PlayerIdentity} WHERE PlayerIdentityId = @PlayerIdentityId), 
@@ -810,7 +804,7 @@ public void CreatePlayerInMatchStatisticsRecord(PlayerInMatchStatisticsRecord re
                      @RunOutByPlayerIdentityId, 
                         (SELECT CASE WHEN @RunOutByPlayerIdentityId IS NULL THEN NULL ELSE (SELECT PlayerIdentityName FROM {Tables.PlayerIdentity} WHERE PlayerIdentityId = @RunOutByPlayerIdentityId) END),
                         (SELECT CASE WHEN @RunOutByPlayerIdentityId IS NULL THEN NULL ELSE (SELECT PlayerRoute FROM {Tables.PlayerIdentity} pi INNER JOIN {Tables.Player} p ON pi.PlayerId = p.PlayerId WHERE PlayerIdentityId = @RunOutByPlayerIdentityId) END),
-                     @RunsScored, @BallsFaced, @Catches, @RunOuts, @WonMatch, @PlayerOfTheMatch, @Probability)",
+                     @RunsScored, @BallsFaced, @Catches, @RunOuts, @WonMatch, @PlayerOfTheMatch)",
                         new
                         {
                             PlayerInMatchStatisticsId = Guid.NewGuid(),
@@ -858,8 +852,7 @@ public void CreatePlayerInMatchStatisticsRecord(PlayerInMatchStatisticsRecord re
                             record.Catches,
                             record.RunOuts,
                             record.WonMatch,
-                            record.PlayerOfTheMatch,
-                            record.Probability
+                            record.PlayerOfTheMatch
                         });
         }
     }

Stoolball.Data.SqlServer.IntegrationTests/Statistics/SqlServerStatisticsRepositoryTests.cs

@@ -606,14 +606,6 @@ public async Task UpdatePlayerStatistics_inserts_multiple_complete_records()
             }
         }
 
-        [Fact]
-        public async Task UpdatePlayerProbability_throws_ArgumentNullException_if_transaction_is_null()
-        {
-            var repo = new SqlServerStatisticsRepository(Mock.Of<IPlayerRepository>());
-
-            await Assert.ThrowsAsync<ArgumentNullException>(async () => await repo.UpdatePlayerProbability(Guid.NewGuid(), null).ConfigureAwait(false)).ConfigureAwait(false);
-        }
-
         public void Dispose() => _scope.Dispose();
     }
 }

Stoolball.Data.SqlServer/SqlServerMatchRepository.cs

@@ -1052,7 +1052,6 @@ await connection.ExecuteAsync(
                     await _statisticsRepository.DeleteBowlingFigures(auditableInnings.MatchInningsId.Value, transaction).ConfigureAwait(false);
                     await _statisticsRepository.UpdateBowlingFigures(auditableInnings, memberKey, memberName, transaction).ConfigureAwait(false);
                     await _statisticsRepository.UpdatePlayerStatistics(playerStatistics, transaction).ConfigureAwait(false);
-                    foreach (var teamId in auditableMatch.Teams.Select(x => x.Team.TeamId)) { await _statisticsRepository.UpdatePlayerProbability(teamId, transaction).ConfigureAwait(false); }
 
                     var serialisedInnings = JsonConvert.SerializeObject(auditableInnings);
                     await _auditRepository.CreateAudit(new AuditRecord
@@ -1214,7 +1213,6 @@ await connection.ExecuteAsync($@"UPDATE {Tables.Over} SET
                     await _statisticsRepository.DeleteBowlingFigures(auditableInnings.MatchInningsId.Value, transaction).ConfigureAwait(false);
                     await _statisticsRepository.UpdateBowlingFigures(auditableInnings, memberKey, memberName, transaction).ConfigureAwait(false);
                     await _statisticsRepository.UpdatePlayerStatistics(playerStatistics, transaction).ConfigureAwait(false);
-                    await _statisticsRepository.UpdatePlayerProbability(auditableInnings.BowlingTeam.Team.TeamId, transaction).ConfigureAwait(false);
 
                     var serialisedInnings = JsonConvert.SerializeObject(auditableInnings);
                     await _auditRepository.CreateAudit(new AuditRecord
@@ -1288,6 +1286,17 @@ await connection.ExecuteAsync($@"UPDATE {Tables.Match} SET
                         },
                         transaction).ConfigureAwait(false);
 
+
+                    var awardsBefore = await connection.QueryAsync<(Guid playerIdentityId, string playerIdentityName, Guid teamId)>(
+                            $@"SELECT pi.PlayerIdentityId, pi.PlayerIdentityName, pi.TeamId 
+                                FROM {Tables.AwardedTo} a INNER JOIN {Tables.PlayerIdentity} pi ON a.PlayerIdentityId = pi.PlayerIdentityId 
+                                WHERE a.MatchId = @MatchId",
+                            new { auditableMatch.MatchId },
+                            transaction).ConfigureAwait(false);
+
+                    var playerIdentitiesWithAwardsBefore = awardsBefore.Select(x => x.playerIdentityId).ToList();
+                    var playerIdentitiesAffectedByAwards = new List<(Guid playerIdentityId, string playerIdentityName, Guid teamId)>();
+
                     await connection.ExecuteAsync($"DELETE FROM {Tables.AwardedTo} WHERE MatchId = @MatchId", new { auditableMatch.MatchId }, transaction).ConfigureAwait(false);
                     var awardId = await connection.QuerySingleOrDefaultAsync<Guid?>($"SELECT AwardId FROM {Tables.Award} WHERE AwardName = 'Player of the match'", null, transaction).ConfigureAwait(false);
                     if (awardId.HasValue)
@@ -1330,14 +1339,21 @@ await connection.ExecuteAsync($@"INSERT INTO {Tables.AwardedTo}
                                         award.Reason
                                     },
                                     transaction).ConfigureAwait(false);
+
+                            // If this is a new award, add to affected player identities
+                            if (!playerIdentitiesWithAwardsBefore.Contains(award.PlayerIdentity.PlayerIdentityId.Value))
+                            {
+                                playerIdentitiesAffectedByAwards.Add((award.PlayerIdentity.PlayerIdentityId.Value, award.PlayerIdentity.PlayerIdentityName, award.PlayerIdentity.Team.TeamId.Value));
+                            }
                         }
                     }
+                    // If awards removed, add those player identities to affected identities
+                    playerIdentitiesAffectedByAwards.AddRange(awardsBefore.Where(x => !auditableMatch.Awards.Select(award => award.PlayerIdentity.PlayerIdentityId).Contains(x.playerIdentityId)));
 
                     var playerStatistics = _playerInMatchStatisticsBuilder.BuildStatisticsForMatch(auditableMatch);
 
                     await _statisticsRepository.DeletePlayerStatistics(auditableMatch.MatchId.Value, transaction).ConfigureAwait(false);
                     await _statisticsRepository.UpdatePlayerStatistics(playerStatistics, transaction).ConfigureAwait(false);
-                    foreach (var teamId in auditableMatch.Teams.Select(x => x.Team.TeamId)) { await _statisticsRepository.UpdatePlayerProbability(teamId, transaction).ConfigureAwait(false); }
 
                     var redacted = CreateRedactedCopy(auditableMatch);
                     await _auditRepository.CreateAudit(new AuditRecord

Stoolball.Data.SqlServer/SqlServerPlayerDataSource.cs

@@ -92,13 +92,14 @@ public async Task<List<PlayerIdentity>> ReadPlayerIdentities(PlayerFilter filter
         {
             using (var connection = _databaseConnectionFactory.CreateDatabaseConnection())
             {
-                var sql = $@"SELECT stats.PlayerIdentityId, stats.PlayerIdentityName, stats.Probability, 
+                const string PROBABILITY_CALCULATION = "COUNT(DISTINCT MatchId)*10-DATEDIFF(DAY,MAX(MatchStartTime),GETDATE())";
+                var sql = $@"SELECT stats.PlayerIdentityId, stats.PlayerIdentityName, {PROBABILITY_CALCULATION} AS Probability, 
                             COUNT(DISTINCT MatchId) AS TotalMatches, MIN(MatchStartTime) AS FirstPlayed,  MAX(MatchStartTime) AS LastPlayed,
                             stats.PlayerId, stats.PlayerRoute, stats.TeamId, stats.TeamName
                             FROM {Tables.PlayerInMatchStatistics} AS stats 
                             <<WHERE>>
-                            GROUP BY stats.PlayerId, stats.PlayerRoute, stats.PlayerIdentityId, stats.PlayerIdentityName, stats.TeamId, stats.TeamName, stats.Probability
-                            ORDER BY stats.TeamId ASC, Probability DESC, stats.PlayerIdentityName ASC";
+                            GROUP BY stats.PlayerId, stats.PlayerRoute, stats.PlayerIdentityId, stats.PlayerIdentityName, stats.TeamId, stats.TeamName 
+                            ORDER BY stats.TeamId ASC, {PROBABILITY_CALCULATION} DESC, stats.PlayerIdentityName ASC";
 
                 var where = new List<string>();
                 var parameters = new Dictionary<string, object>();

Stoolball.Data.SqlServer/SqlServerStatisticsRepository.cs

@@ -210,14 +210,5 @@ await transaction.Connection.ExecuteAsync($@"INSERT INTO {Tables.PlayerInMatchSt
                         transaction).ConfigureAwait(false);
             }
         }
-
-        public async Task UpdatePlayerProbability(Guid? teamId, IDbTransaction transaction)
-        {
-            if (transaction is null)
-            {
-                throw new ArgumentNullException(nameof(transaction));
-            }
-            await transaction.Connection.ExecuteAsync("EXEC usp_Statistics_UpdateProbability @TeamId", new { TeamId = teamId }, transaction).ConfigureAwait(false);
-        }
     }
 }

Stoolball.Data.UmbracoMigrations/StatisticsAddProbability.cs → Stoolball.Data.UmbracoMigrations/036_Add_Probability/StatisticsAddProbability.cs

@@ -5,7 +5,7 @@
 namespace Stoolball.Data.UmbracoMigrations
 {
     /// <summary>
-    /// Adds a table for recording the home grounds and venues of stoolball teams which can change over time
+    /// Adds a probability column and a stored procedure to populate it. Probability is used to order player name suggestions.
     /// </summary>
     public partial class StatisticsAddProbability : MigrationBase
     {

Stoolball.Data.UmbracoMigrations/038_Remove_Probability/StatisticsRemoveProbability.cs

@@ -0,0 +1,31 @@
+using Microsoft.Extensions.Logging;
+using Stoolball.Data.SqlServer;
+using Umbraco.Cms.Infrastructure.Migrations;
+
+namespace Stoolball.Data.UmbracoMigrations
+{
+    /// <summary>
+    /// Removes the probability column and the stored procedure to populate it. Probability is now calculated differently as part of the SELECT.
+    /// </summary>
+    public partial class StatisticsRemoveProbability : MigrationBase
+    {
+        public StatisticsRemoveProbability(IMigrationContext context) : base(context)
+        {
+        }
+
+        protected override void Migrate()
+        {
+            Logger.LogDebug("Running migration {MigrationStep}", typeof(StatisticsRemoveProbability).Name);
+
+            if (ColumnExists(Tables.PlayerInMatchStatistics, "Probability"))
+            {
+                Delete.Column("Probability").FromTable(Tables.PlayerInMatchStatistics).Do();
+                Execute.Sql("DROP PROCEDURE IF EXISTS usp_Statistics_UpdateProbability").Do();
+            }
+            else
+            {
+                Logger.LogDebug("The database column {DbTable}.{DbColumn} does not exist, skipping", Tables.PlayerInMatchStatistics, "Probability");
+            }
+        }
+    }
+}

Stoolball.Data.UmbracoMigrations/StoolballDataMigrationPlan.cs

@@ -48,7 +48,8 @@ public StoolballDataMigrationPlan() : base("StoolballData")
                 // Initial state when website launched Easter 2021
                 .To<CommentRemoveMemberNameAndMakeMemberKeyNullable>(typeof(CommentRemoveMemberNameAndMakeMemberKeyNullable).ToString())
                 .To<StatisticsAddProbability>(typeof(StatisticsAddProbability).ToString())
-                .To<Add_fnTeamName>(typeof(Add_fnTeamName).ToString());
+                .To<Add_fnTeamName>(typeof(Add_fnTeamName).ToString())
+                .To<StatisticsRemoveProbability>(typeof(StatisticsRemoveProbability).ToString());
         }
     }
 }

Stoolball.UnitTests/Matches/BattingScorecardComparerTests.cs

@@ -217,7 +217,7 @@ public void Removed_innings_is_identified_by_index()
         }
 
         [Fact]
-        public void Added_batter_PlayerIdentity_is_identified()
+        public void Added_batter_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -232,10 +232,13 @@ public void Added_batter_PlayerIdentity_is_identified()
 
             Assert.Single(result.BattingPlayerIdentitiesAdded);
             Assert.Contains(playerThree.PlayerIdentityName, result.BattingPlayerIdentitiesAdded);
+
+            Assert.Equal(2, result.BattingPlayerIdentitiesAffected.Count);
+            Assert.Contains(playerThree.PlayerIdentityName, result.BattingPlayerIdentitiesAffected);
         }
 
         [Fact]
-        public void Affected_batter_PlayerIdentity_is_identified()
+        public void Changed_batter_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -252,7 +255,7 @@ public void Affected_batter_PlayerIdentity_is_identified()
         }
 
         [Fact]
-        public void Removed_batter_PlayerIdentity_is_identified()
+        public void Removed_batter_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -267,11 +270,14 @@ public void Removed_batter_PlayerIdentity_is_identified()
 
             Assert.Single(result.BattingPlayerIdentitiesRemoved);
             Assert.Contains(playerTwo.PlayerIdentityName, result.BattingPlayerIdentitiesRemoved);
+
+            Assert.Equal(2, result.BattingPlayerIdentitiesAffected.Count);
+            Assert.Contains(playerTwo.PlayerIdentityName, result.BattingPlayerIdentitiesAffected);
         }
 
 
         [Fact]
-        public void Added_fielder_PlayerIdentity_is_identified()
+        public void Added_fielder_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -287,10 +293,13 @@ public void Added_fielder_PlayerIdentity_is_identified()
 
             Assert.Single(result.BowlingPlayerIdentitiesAdded);
             Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesAdded);
+
+            Assert.Equal(2, result.BowlingPlayerIdentitiesAffected.Count);
+            Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesAffected);
         }
 
         [Fact]
-        public void Affected_fielder_PlayerIdentity_is_identified()
+        public void Changed_fielder_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -308,7 +317,7 @@ public void Affected_fielder_PlayerIdentity_is_identified()
         }
 
         [Fact]
-        public void Removed_fielder_PlayerIdentity_is_identified()
+        public void Removed_fielder_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -325,11 +334,14 @@ public void Removed_fielder_PlayerIdentity_is_identified()
 
             Assert.Single(result.BowlingPlayerIdentitiesRemoved);
             Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesRemoved);
+
+            Assert.Equal(2, result.BowlingPlayerIdentitiesAffected.Count);
+            Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesAffected);
         }
 
 
         [Fact]
-        public void Added_bowler_PlayerIdentity_is_identified()
+        public void Added_bowler_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -345,10 +357,13 @@ public void Added_bowler_PlayerIdentity_is_identified()
 
             Assert.Single(result.BowlingPlayerIdentitiesAdded);
             Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesAdded);
+
+            Assert.Equal(2, result.BowlingPlayerIdentitiesAffected.Count);
+            Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesAffected);
         }
 
         [Fact]
-        public void Affected_bowler_PlayerIdentity_is_identified()
+        public void Changed_bowler_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -366,7 +381,7 @@ public void Affected_bowler_PlayerIdentity_is_identified()
         }
 
         [Fact]
-        public void Removed_bowler_PlayerIdentity_is_identified()
+        public void Removed_bowler_PlayerIdentity_is_identified_and_added_to_affected_identities()
         {
             var playerOne = new PlayerIdentity { PlayerIdentityName = "Player one" };
             var playerTwo = new PlayerIdentity { PlayerIdentityName = "Player two" };
@@ -383,6 +398,9 @@ public void Removed_bowler_PlayerIdentity_is_identified()
 
             Assert.Single(result.BowlingPlayerIdentitiesRemoved);
             Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesRemoved);
+
+            Assert.Equal(2, result.BowlingPlayerIdentitiesAffected.Count);
+            Assert.Contains(playerFour.PlayerIdentityName, result.BowlingPlayerIdentitiesAffected);
         }
     }
 }