Effort system revamp - #53
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive planning documentation for the effort system revamp, providing a complete roadmap for migrating the legacy ColdFusion system to the modern VIPER2 architecture using an agile sprint-based approach.
Key changes include:
- Master plan with 16-sprint agile development strategy
- Complete technical implementation plans covering Entity Framework migration and database consolidation
- Detailed field-by-field mapping and API specifications
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Effort_System_Documentation.md | Legacy system analysis with stored procedure categorization and migration timeline |
| Effort_Proposed_Database_Schema.sql | Complete target schema with 11 tables, constraints, and indexes |
| Effort_EF_Migration_Plan.md | Entity Framework migration strategy and service layer architecture |
| Effort_Database_Field_Mapping.md | Field-by-field transformation specification with rationale |
| Effort_Data_Migration.sql | Production-ready Sprint 14 migration script with validation |
| Effort_API_Design.md | REST API specification with VIPER2 Areas routing |
| EFFORT_MASTER_PLAN.md | High-level agile sprint plan with deliverables and timeline |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@bsedwards, I am not familiar with the Efforts system, but I worked with Claude Code over several iterations, and I hope these documents will help kickstart the project. A significant decision point would be whether to stick with stored procedures or use the EF system. I can see the benefits of the EF system, in which the logic would be in the doc and not hidden behind a stored procedure. But there is overhead, and it might not be as fast/performant. But the EF system does do query optimization, so it might be on par for the simpler queries. We can have SPs for the more intensive reporting queries. |
1st draft of planning docs - EFFORT_MASTER_PLAN.md - Overall, high level plan Technical Implementation - Effort_EF_Migration_Plan.md - Entity Framework migration strategy and service layer architecture - Effort_Proposed_Database_Schema.sql - Complete target schema (11 tables, 27 constraints, 12 indexes) - Effort_Database_Field_Mapping.md - Field-by-field transformation specification with rationale - Effort_API_Design.md - REST API specification with VIPER2 Areas routing Execution & Reference - Effort_Data_Migration.sql - Production-ready Sprint 14 migration script with validation - Effort_System_Documentation.md - Legacy ColdFusion system analysis and requirements
- Repalced MothraId with PersonId - Updating course table to include course units as part of unique key - Added Sabbaticals table - Added migration of Stored Procedures to the plan - Added data analysis output detailing problems with adding foreign keys and unique constraints to the existing database schema
ecbaa95 to
d534614
Compare
|
@bsedwards If you look at web/Areas/Effort/docs/EffortMigrationAnalysis_20250926_162054.txt, this is the output of a data analysis script that looks at data issues with adding the planned foreign key:
|
- Created scripts to perform data analysis on Effort database and fix issues
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 22 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add ValidateOutputPath helper to prevent path traversal attacks - Simplified script helper functions
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add schema export tool to document legacy database structure with tables, indexes, constraints, stored procedures, and sample data - Addressing code review feedback on analysis/remediation scripts
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (2)
web/Areas/Effort/docs/Effort_Proposed_Database_Schema.sql:1
- The query references legacy column names 'person_EffortDept' and 'person_JobGrpID' but the table definition uses new names 'EffortDept' and 'JobGroupId' (lines 53, 55). Update to match the new schema: 'EffortDept' and 'JobGroupId'.
-- =================================================================
web/Areas/Effort/docs/Effort_Data_Migration.sql:1
- Attempting to insert into 'ClientId' column which was removed in the new schema design according to the field mapping document. Remove this field from the INSERT statement (line 299 and 308).
-- =================================================================
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IF EXISTS (SELECT * FROM [Effort].[INFORMATION_SCHEMA].[TABLES] WHERE TABLE_NAME = 'tblPercent') | ||
| BEGIN | ||
| INSERT INTO [effort].[Percentages] ( | ||
| Id, PersonId, TermCode, EffortType, Percentage, Unit, |
There was a problem hiding this comment.
The column name should be 'EffortTypeId' (INT FK) according to the schema in Effort_Proposed_Database_Schema.sql line 95, not 'EffortType'. Update to match the schema definition.
| Id, PersonId, TermCode, EffortType, Percentage, Unit, | |
| Id, PersonId, TermCode, EffortTypeId, Percentage, Unit, |
- VPR-2/VPR-3: Analyzed tables/stored procedures and documented which ones to migrate/skip in web/Areas/Effort/docs/EFFORT_MASTER_PLAN.md - VPR-4: Updated data analysis scripts to give warnings for orphaned entries due to previous deletions - VPR-5: CreateEffortDatabase.cs and Effort_Database_Schema.sql document new database design and added migration script - VPR-6: Migration scripts mapping mothraId to PersonId, added foreign key constraint
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 19 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var item in legacyData) | ||
| { | ||
| // Skip if both abbrev and unit are null | ||
| if (string.IsNullOrEmpty(item.Abbrev) && string.IsNullOrEmpty(item.Unit)) continue; | ||
|
|
||
| string unitCode = item.Abbrev ?? item.Unit ?? "UNKNOWN"; | ||
|
|
||
| // Check if already exists | ||
| using var checkCmd = new SqlCommand("SELECT COUNT(*) FROM [effort].[ReportUnits] WHERE UnitCode = @UnitCode", viperConnection, transaction); | ||
| checkCmd.Parameters.AddWithValue("@UnitCode", unitCode); | ||
| int exists = (int)checkCmd.ExecuteScalar(); | ||
| if (exists > 0) continue; | ||
|
|
||
| insertCmd.Parameters["@UnitCode"].Value = unitCode; | ||
| insertCmd.Parameters["@UnitName"].Value = item.Unit ?? item.Abbrev ?? unitCode; | ||
| insertCmd.ExecuteNonQuery(); | ||
| rows++; | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| reader.IsDBNull(4) ? null : reader.GetString(4), // person_MiddleIni | ||
| reader.GetString(5), // person_EffortTitleCode | ||
| reader.GetString(6), // person_EffortDept | ||
| reader.GetDouble(7) == 0 ? 0 : (decimal)reader.GetDouble(7), // person_PercentAdmin (float to decimal) |
There was a problem hiding this comment.
Equality checks on floating point values can yield unexpected results.
| reader.GetDouble(7) == 0 ? 0 : (decimal)reader.GetDouble(7), // person_PercentAdmin (float to decimal) | |
| Math.Abs(reader.GetDouble(7)) < 1e-6 ? 0 : (decimal)reader.GetDouble(7), // person_PercentAdmin (float to decimal) |
| var tableMapping = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| { "tblEffort", "vw_tblEffort" }, | ||
| { "tblPerson", "vw_tblPerson" }, | ||
| { "tblCourses", "vw_tblCourses" }, | ||
| { "tblPercent", "vw_tblPercent" }, | ||
| { "tblStatus", "vw_tblStatus" }, | ||
| { "tblSabbatic", "vw_tblSabbatic" }, | ||
| { "tblRoles", "vw_tblRoles" }, | ||
| { "tblEffortType_LU", "vw_tblEffortType_LU" }, | ||
| { "userAccess", "vw_userAccess" }, | ||
| { "tblUnits_LU", "vw_tblUnits_LU" }, | ||
| { "tblJobCode", "vw_tblJobCode" }, | ||
| { "tblReportUnits", "vw_tblReportUnits" }, | ||
| { "tblReviewYears", "vw_tblReviewYears" }, | ||
| { "tblAltTitles", "vw_tblAltTitles" }, | ||
| { "months", "vw_months" }, | ||
| { "workdays", "vw_workdays" }, | ||
| { "tblCourseRelationships", "vw_tblCourseRelationships" }, | ||
| { "additionalQuestion", "vw_additionalQuestion" }, | ||
| { "tblAudit", "vw_tblAudit" } | ||
| }; |
There was a problem hiding this comment.
The contents of this container are never accessed.
| var tableMapping = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) | |
| { | |
| { "tblEffort", "vw_tblEffort" }, | |
| { "tblPerson", "vw_tblPerson" }, | |
| { "tblCourses", "vw_tblCourses" }, | |
| { "tblPercent", "vw_tblPercent" }, | |
| { "tblStatus", "vw_tblStatus" }, | |
| { "tblSabbatic", "vw_tblSabbatic" }, | |
| { "tblRoles", "vw_tblRoles" }, | |
| { "tblEffortType_LU", "vw_tblEffortType_LU" }, | |
| { "userAccess", "vw_userAccess" }, | |
| { "tblUnits_LU", "vw_tblUnits_LU" }, | |
| { "tblJobCode", "vw_tblJobCode" }, | |
| { "tblReportUnits", "vw_tblReportUnits" }, | |
| { "tblReviewYears", "vw_tblReviewYears" }, | |
| { "tblAltTitles", "vw_tblAltTitles" }, | |
| { "months", "vw_months" }, | |
| { "workdays", "vw_workdays" }, | |
| { "tblCourseRelationships", "vw_tblCourseRelationships" }, | |
| { "additionalQuestion", "vw_additionalQuestion" }, | |
| { "tblAudit", "vw_tblAudit" } | |
| }; |
| var viperMothraIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| using (var viperConn = new SqlConnection(_viperConnectionString)) | ||
| { | ||
| viperConn.Open(); | ||
| using (var cmd = new SqlCommand("SELECT MothraId FROM users.Person WHERE MothraId IS NOT NULL", viperConn)) | ||
| using (var reader = cmd.ExecuteReader()) | ||
| { | ||
| while (reader.Read()) | ||
| { | ||
| viperMothraIds.Add(reader.GetString(0)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The contents of this container are never accessed.
| var viperMothraIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | |
| using (var viperConn = new SqlConnection(_viperConnectionString)) | |
| { | |
| viperConn.Open(); | |
| using (var cmd = new SqlCommand("SELECT MothraId FROM users.Person WHERE MothraId IS NOT NULL", viperConn)) | |
| using (var reader = cmd.ExecuteReader()) | |
| { | |
| while (reader.Read()) | |
| { | |
| viperMothraIds.Add(reader.GetString(0)); | |
| } | |
| } | |
| } |
| description.Append($" INCLUDE ({includedColumns})"); | ||
| } | ||
|
|
||
| _output.AppendLine(description.ToString()); |
There was a problem hiding this comment.
Redundant call to 'ToString' on a String object.
| catch (Exception ex) | ||
| { | ||
| Console.ForegroundColor = ConsoleColor.Red; | ||
| Console.WriteLine($"\nERROR: {ex.Message}"); | ||
| Console.WriteLine("\nStack Trace:"); | ||
| Console.WriteLine(ex.StackTrace); | ||
| Console.ResetColor(); | ||
| Environment.Exit(1); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"ERROR: {ex.Message}"); | ||
| Console.WriteLine($"Stack Trace: {ex.StackTrace}"); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($" ✗ Cannot connect to legacy Efforts database: {ex.Message}"); | ||
| Console.WriteLine(" Check the 'Effort' connection string in appsettings"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
| catch | ||
| { | ||
| Console.WriteLine(" ⚠ WARNING: Could not verify VIPER.users.Person table. MothraId mapping may fail."); |
There was a problem hiding this comment.
Generic catch clause.
| catch | |
| { | |
| Console.WriteLine(" ⚠ WARNING: Could not verify VIPER.users.Person table. MothraId mapping may fail."); | |
| catch (Exception ex) | |
| { | |
| Console.WriteLine(" ⚠ WARNING: Could not verify VIPER.users.Person table. MothraId mapping may fail."); | |
| Console.WriteLine($" Exception: {ex.Message}"); |
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($" ⚠ WARNING: Could not check legacy data: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
Generic catch clause.
|
@bsedwards Lots of changes in the last commit, but here are the docs highlights from the last commit: Migration Documentation
Planning Documentation
Technical Documentation
Supporting Documentation
Removed (5 obsolete files)
I tested the creation of the Effort database and data migration. The tables are at [VIPER].[effort]. I have not yet tested the creation of the "EffortShadow" database. |
bsedwards
left a comment
bsedwards
left a comment
There was a problem hiding this comment.
I reviewed the new schema and it looks good. Is it possible to have an FK between TermStatus.termcode and other termcodes in the system, or is that prevented by the legacy data?
- Add CreateEffortReportingProcedures.cs with 16 reporting stored procedures - Update EFFORT_MASTER_PLAN.md to reflect reporting procedures implementation
@bsedwards I found that the invalid terms are all summer terms:
I can either map these to the "Summer Quarter" that is in the [vwTerms] table. Or add these terms to courses.dbo.terminfo? Or skip these records during migration? Any reason they are not in terminfo? |
- Remove redundant `legacyException == null` check (always true at that point) - Combine nested if statements for boolean flag detection - Use LINQ .Where() instead of foreach with continue for column filtering
| foreach (var col in columnMapping.Keys.OrderBy(k => k) | ||
| .Where(col => ignoredColumns == null || !ignoredColumns.Contains(col))) | ||
| { | ||
| var value = row[col]; | ||
| if (value == DBNull.Value) | ||
| values.Add("NULL"); | ||
| else if (value is string str) | ||
| values.Add(str.Trim()); | ||
| else if (value is DateTime dt) | ||
| values.Add(dt.ToString("yyyy-MM-dd HH:mm:ss")); | ||
| else if (IsNumeric(value)) | ||
| values.Add(Convert.ToDecimal(value).ToString("F2")); | ||
| else | ||
| values.Add(value.ToString() ?? ""); | ||
| } |
| if (legacyValue is string legacyStr && shadowValue is string shadowStr) | ||
| { | ||
| if (legacyStr != shadowStr && legacyStr.Trim() == shadowStr.Trim()) | ||
| { | ||
| result.Warnings.Add( | ||
| $"Row {rowIndex}, Column '{legacyCol}': WHITESPACE PADDING DIFFERENCE - " + | ||
| $"Legacy='{legacyStr}' ({legacyStr.Length} chars), " + | ||
| $"Shadow='{shadowStr}' ({shadowStr.Length} chars). " + | ||
| "This indicates a char/varchar type mismatch in the schema."); | ||
| } | ||
| } |
| reader.IsDBNull(7) ? null : reader.GetString(7), // percent_Comment | ||
| reader.GetDateTime(8), // percent_start (NOT NULL per WHERE clause) | ||
| reader.IsDBNull(9) ? null : reader.GetDateTime(9), // percent_end | ||
| reader.IsDBNull(10) ? false : reader.GetBoolean(10) // percent_compensated |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 35 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var values = new List<string>(); | ||
| foreach (var col in columnMapping.Keys.OrderBy(k => k) | ||
| .Where(col => ignoredColumns == null || !ignoredColumns.Contains(col))) | ||
| { | ||
| var value = row[col]; | ||
| if (value == DBNull.Value) | ||
| values.Add("NULL"); | ||
| else if (value is string str) | ||
| values.Add(str.Trim()); | ||
| else if (value is DateTime dt) | ||
| values.Add(dt.ToString("yyyy-MM-dd HH:mm:ss")); | ||
| else if (IsNumeric(value)) | ||
| values.Add(Convert.ToDecimal(value).ToString("F2")); | ||
| else | ||
| values.Add(value.ToString() ?? ""); | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| var values = new List<string>(); | |
| foreach (var col in columnMapping.Keys.OrderBy(k => k) | |
| .Where(col => ignoredColumns == null || !ignoredColumns.Contains(col))) | |
| { | |
| var value = row[col]; | |
| if (value == DBNull.Value) | |
| values.Add("NULL"); | |
| else if (value is string str) | |
| values.Add(str.Trim()); | |
| else if (value is DateTime dt) | |
| values.Add(dt.ToString("yyyy-MM-dd HH:mm:ss")); | |
| else if (IsNumeric(value)) | |
| values.Add(Convert.ToDecimal(value).ToString("F2")); | |
| else | |
| values.Add(value.ToString() ?? ""); | |
| } | |
| var values = columnMapping.Keys | |
| .OrderBy(k => k) | |
| .Where(col => ignoredColumns == null || !ignoredColumns.Contains(col)) | |
| .Select(col => | |
| { | |
| var value = row[col]; | |
| if (value == DBNull.Value) | |
| return "NULL"; | |
| else if (value is string str) | |
| return str.Trim(); | |
| else if (value is DateTime dt) | |
| return dt.ToString("yyyy-MM-dd HH:mm:ss"); | |
| else if (IsNumeric(value)) | |
| return Convert.ToDecimal(value).ToString("F2"); | |
| else | |
| return value.ToString() ?? ""; | |
| }) | |
| .ToList(); |
| description.Append($" INCLUDE ({includedColumns})"); | ||
| } | ||
|
|
||
| _output.AppendLine(description.ToString()); |
There was a problem hiding this comment.
Redundant call to 'ToString' on a String object.
| foreach (Match match in matches) | ||
| { | ||
| var name = match.Groups[2].Value; | ||
| var comments = match.Groups[4].Value; // Optional comments before CREATE |
| } | ||
|
|
||
| // Compare values (still uses trimmed comparison for compatibility) | ||
| if (!ValuesEqual(legacyValue, shadowValue)) | ||
| { | ||
| result.Differences.Add($"Row {rowIndex}, Column '{legacyCol}': Legacy='{legacyValue}' vs Shadow='{shadowValue}'"); |
There was a problem hiding this comment.
These 'if' statements can be combined.
| } | |
| // Compare values (still uses trimmed comparison for compatibility) | |
| if (!ValuesEqual(legacyValue, shadowValue)) | |
| { | |
| result.Differences.Add($"Row {rowIndex}, Column '{legacyCol}': Legacy='{legacyValue}' vs Shadow='{shadowValue}'"); | |
| // Compare trimmed strings for actual difference | |
| if (legacyStr.Trim() != shadowStr.Trim()) | |
| { | |
| result.Differences.Add($"Row {rowIndex}, Column '{legacyCol}': Legacy='{legacyStr}' vs Shadow='{shadowStr}'"); | |
| } | |
| } | |
| else | |
| { | |
| // Compare non-string values | |
| if (!ValuesEqual(legacyValue, shadowValue)) | |
| { | |
| result.Differences.Add($"Row {rowIndex}, Column '{legacyCol}': Legacy='{legacyValue}' vs Shadow='{shadowValue}'"); | |
| } |
| reader.IsDBNull(7) ? null : reader.GetString(7), // percent_Comment | ||
| reader.GetDateTime(8), // percent_start (NOT NULL per WHERE clause) | ||
| reader.IsDBNull(9) ? null : reader.GetDateTime(9), // percent_end | ||
| reader.IsDBNull(10) ? false : reader.GetBoolean(10) // percent_compensated |
There was a problem hiding this comment.
The expression 'A ? false : B' can be simplified to '!A && B'.
| reader.IsDBNull(10) ? false : reader.GetBoolean(10) // percent_compensated | |
| !reader.IsDBNull(10) && reader.GetBoolean(10) // percent_compensated |
| REM Run the verification script | ||
| echo Running verification script... | ||
| echo. | ||
| dotnet run --project EffortMigration.csproj -- verify-shadow%SCRIPT_ARGS% |
There was a problem hiding this comment.
Unsanitized forwarding of user-controlled arguments into a shell command can lead to command injection. Passing a value like --test-mothraid 123 & calc.exe would cause calc.exe to run since %SCRIPT_ARGS% is concatenated directly into the cmd.exe command line. Fix by strictly whitelisting allowed flags/values and safely quoting each token when invoking dotnet (e.g., build an array of validated args and call: dotnet run --project EffortMigration.csproj -- "verify-shadow" "--test-mothraid" "%SAFE_ID%"), or reject any argument containing shell metacharacters &|><^".
| REM Run the C# data migration tool | ||
| echo Running data migration script... | ||
| echo. | ||
| dotnet run --project EffortMigration.csproj -- migrate-data%SCRIPT_ARGS% |
There was a problem hiding this comment.
User input is appended directly into the command line via %SCRIPT_ARGS%, enabling Windows CMD metacharacter injection. An attacker (or malicious copy/paste) could run arbitrary commands by supplying &, |, or > in arguments (e.g., --apply & whoami). Mitigate by validating against a strict allowlist of flags, rejecting arguments containing shell metacharacters, and passing each argument as a separately quoted parameter to dotnet (e.g., dotnet run --project EffortMigration.csproj -- "migrate-data" %SAFE_ARGS%).
| REM Run the C# shadow creation tool | ||
| echo Running shadow schema creation script... | ||
| echo. | ||
| dotnet run --project EffortMigration.csproj -- create-shadow%SCRIPT_ARGS% |
There was a problem hiding this comment.
The script concatenates untrusted arguments into the dotnet run command (create-shadow%SCRIPT_ARGS%), allowing command injection via CMD metacharacters. For example, invoking RunCreateShadow.bat --apply & powershell -nop -c whoami will execute the extra command. Resolve by implementing a strict argument allowlist, rejecting/escaping &|><^", and quoting each argument when calling dotnet (e.g., dotnet run ... -- "create-shadow" %SAFE_QUOTED_ARGS%).
| REM Run the C# schema creation tool | ||
| echo Running schema creation script... | ||
| echo. | ||
| dotnet run --project EffortMigration.csproj -- create-database%SCRIPT_ARGS% |
There was a problem hiding this comment.
Unvalidated %SCRIPT_ARGS% is interpolated directly into the command line (create-database%SCRIPT_ARGS%), enabling OS command injection if arguments contain CMD metacharacters (e.g., --force & net user). Prevent this by only allowing known-safe flags (--apply, --force, --drop, environments), rejecting any argument with &|><^", and passing each argument as a distinct, quoted parameter to dotnet instead of string-concatenating them.
- Simplify Audits table: replace dual-column approach (ChangesLegacy/ ChangeDetails/IsLegacyFormat) with single Changes column plus legacy preservation columns for 1:1 migration verification - Add comprehensive view/table verification to shadow schema validator comparing row counts and sample data between legacy and shadow views - Add tblPercent migration verification checking top 25 users' data - Align JobCodes table with legacy schema (Code + IncludeClinSchedule) - Extract shared argument parsing into ParseArgs.bat (DRY) - Add MothraId/LoginId lookup helpers to EffortScriptHelper with caching - Add NULL academic year detection and remediation task - Add linting support for EffortMigration.csproj sub-project - Document post-deployment cleanup steps for legacy decommissioning
- Add INSERT/UPDATE triggers for tblCourses (derived CAST column) - Add INSERT trigger for tblCourseRelationships (Cross→CrossList mapping) - Fix CRUD SP verification to use valid FK-satisfying test data - Align Roles.Id/Records.Role to int matching legacy schema - Move Roles/EffortTypes seeding to MigrateEffortData - Fix --force flag exiting early after drop
…Year - Replace Percentages.TermCode with AcademicYear (char(9)) to match legacy tblPercent schema and simplify academic year queries - Add INSTEAD OF triggers for tblPercent and tblStatus shadow views to handle derived column updates correctly - Fix delete procedure tests to properly set result.Status on failure, preventing false positives in shadow verification - Cast datetime2 columns to datetime in shadow views for ColdFusion compatibility (CF expects legacy datetime precision)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 37 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var kvp in legacyMothraIdToSignatures) | ||
| { | ||
| if (kvp.Value.Contains(sig)) | ||
| { | ||
| mothraIdsWithMissingRows.Add(kvp.Key); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var kvp in legacyMothraIdToSignatures) | |
| { | |
| if (kvp.Value.Contains(sig)) | |
| { | |
| mothraIdsWithMissingRows.Add(kvp.Key); | |
| } | |
| foreach (var kvp in legacyMothraIdToSignatures.Where(kvp => kvp.Value.Contains(sig))) | |
| { | |
| mothraIdsWithMissingRows.Add(kvp.Key); |
| foreach (var kvp in legacyMothraIdToSignatures) | ||
| { | ||
| if (kvp.Value.Contains(sig) && orphanedMothraIds.Contains(kvp.Key)) | ||
| { | ||
| isOrphaned = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var kvp in legacyMothraIdToSignatures) | |
| { | |
| if (kvp.Value.Contains(sig) && orphanedMothraIds.Contains(kvp.Key)) | |
| { | |
| isOrphaned = true; | |
| break; | |
| } | |
| foreach (var kvp in legacyMothraIdToSignatures | |
| .Where(kvp => kvp.Value.Contains(sig) && orphanedMothraIds.Contains(kvp.Key))) | |
| { | |
| isOrphaned = true; | |
| break; |
| Console.ResetColor(); | ||
| return 0; | ||
| } | ||
| else if (tablesExist && !forceRecreate) |
There was a problem hiding this comment.
Condition is always false because of access to local variable tablesExist.
| if (!hasMothraId) _report.AuditDataQuality.RecordsWithNullMothraId += count; | ||
|
|
||
| // Collect invalid TermCodes | ||
| if (hasTermCode && termCode.HasValue && !validTermCodes.Contains(termCode.Value)) |
There was a problem hiding this comment.
Condition is always true because of access to local variable hasTermCode.
| if (hasTermCode && termCode.HasValue && !validTermCodes.Contains(termCode.Value)) | |
| if (hasTermCode && !validTermCodes.Contains(termCode.Value)) |
| } | ||
|
|
||
| // Categorize mappability - only consider records with valid, populated fields as mappable | ||
| bool termCodeValid = hasTermCode && termCode.HasValue && validTermCodes.Contains(termCode.Value); |
There was a problem hiding this comment.
Condition is always true because of access to local variable hasTermCode.
| bool termCodeValid = hasTermCode && termCode.HasValue && validTermCodes.Contains(termCode.Value); | |
| bool termCodeValid = termCode.HasValue && validTermCodes.Contains(termCode.Value); |
| if (!rowDifferences.ContainsKey(pkDisplay)) | ||
| { | ||
| if (rowDifferences.Count < 5) | ||
| { | ||
| rowDifferences[pkDisplay] = new List<(string, string, string)>(); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (!rowDifferences.ContainsKey(pkDisplay)) | |
| { | |
| if (rowDifferences.Count < 5) | |
| { | |
| rowDifferences[pkDisplay] = new List<(string, string, string)>(); | |
| } | |
| if (!rowDifferences.ContainsKey(pkDisplay) && rowDifferences.Count < 5) | |
| { | |
| rowDifferences[pkDisplay] = new List<(string, string, string)>(); |
| if (expectedShadow == "*" && shadowValue != "NULL") | ||
| return true; | ||
| if (string.Equals(expectedShadow, shadowValue, StringComparison.OrdinalIgnoreCase)) | ||
| return true; |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (expectedShadow == "*" && shadowValue != "NULL") | |
| return true; | |
| if (string.Equals(expectedShadow, shadowValue, StringComparison.OrdinalIgnoreCase)) | |
| return true; | |
| if ((expectedShadow == "*" && shadowValue != "NULL") || | |
| string.Equals(expectedShadow, shadowValue, StringComparison.OrdinalIgnoreCase)) | |
| return true; |
| if (ViewsWithTermNameDifferences.Contains(viewName)) | ||
| { | ||
| // Term name format differences are acceptable (e.g., "Summer Session II 2002" vs "Summer 2002") | ||
| if (columnName.EndsWith("_TermName", StringComparison.OrdinalIgnoreCase)) | ||
| return true; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (useRandomSampling) | ||
| { | ||
| legacyQuery = $@" | ||
| SELECT TOP {sampleSize} {columnList} | ||
| FROM [{legacyTable}] | ||
| ORDER BY NEWID()"; | ||
| } | ||
| else | ||
| { | ||
| legacyQuery = $"SELECT TOP {sampleSize} {columnList} FROM [{legacyTable}] ORDER BY {orderByClause}"; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (useRandomSampling) | |
| { | |
| legacyQuery = $@" | |
| SELECT TOP {sampleSize} {columnList} | |
| FROM [{legacyTable}] | |
| ORDER BY NEWID()"; | |
| } | |
| else | |
| { | |
| legacyQuery = $"SELECT TOP {sampleSize} {columnList} FROM [{legacyTable}] ORDER BY {orderByClause}"; | |
| } | |
| legacyQuery = useRandomSampling | |
| ? $@" | |
| SELECT TOP {sampleSize} {columnList} | |
| FROM [{legacyTable}] | |
| ORDER BY NEWID()" | |
| : $"SELECT TOP {sampleSize} {columnList} FROM [{legacyTable}] ORDER BY {orderByClause}"; |
| string query; | ||
| if (isLegacy) | ||
| { | ||
| query = @" | ||
| SELECT COLUMN_NAME, DATA_TYPE, ISNULL(CHARACTER_MAXIMUM_LENGTH, 0), IS_NULLABLE | ||
| FROM INFORMATION_SCHEMA.COLUMNS | ||
| WHERE TABLE_NAME = @TableName | ||
| ORDER BY ORDINAL_POSITION"; | ||
| } | ||
| else | ||
| { | ||
| query = @" | ||
| SELECT COLUMN_NAME, DATA_TYPE, ISNULL(CHARACTER_MAXIMUM_LENGTH, 0), IS_NULLABLE | ||
| FROM INFORMATION_SCHEMA.COLUMNS | ||
| WHERE TABLE_SCHEMA = 'EffortShadow' AND TABLE_NAME = @TableName | ||
| ORDER BY ORDINAL_POSITION"; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| string query; | |
| if (isLegacy) | |
| { | |
| query = @" | |
| SELECT COLUMN_NAME, DATA_TYPE, ISNULL(CHARACTER_MAXIMUM_LENGTH, 0), IS_NULLABLE | |
| FROM INFORMATION_SCHEMA.COLUMNS | |
| WHERE TABLE_NAME = @TableName | |
| ORDER BY ORDINAL_POSITION"; | |
| } | |
| else | |
| { | |
| query = @" | |
| SELECT COLUMN_NAME, DATA_TYPE, ISNULL(CHARACTER_MAXIMUM_LENGTH, 0), IS_NULLABLE | |
| FROM INFORMATION_SCHEMA.COLUMNS | |
| WHERE TABLE_SCHEMA = 'EffortShadow' AND TABLE_NAME = @TableName | |
| ORDER BY ORDINAL_POSITION"; | |
| } | |
| string query = isLegacy | |
| ? @" | |
| SELECT COLUMN_NAME, DATA_TYPE, ISNULL(CHARACTER_MAXIMUM_LENGTH, 0), IS_NULLABLE | |
| FROM INFORMATION_SCHEMA.COLUMNS | |
| WHERE TABLE_NAME = @TableName | |
| ORDER BY ORDINAL_POSITION" | |
| : @" | |
| SELECT COLUMN_NAME, DATA_TYPE, ISNULL(CHARACTER_MAXIMUM_LENGTH, 0), IS_NULLABLE | |
| FROM INFORMATION_SCHEMA.COLUMNS | |
| WHERE TABLE_SCHEMA = 'EffortShadow' AND TABLE_NAME = @TableName | |
| ORDER BY ORDINAL_POSITION"; |
- Remove redundant .HasValue checks already guarded by prior conditions - Use LINQ Where clauses and ternary expressions to reduce nesting - Consolidate duplicate dotnet-tools.json to root
| catch (Exception ex) | ||
| { | ||
| Console.ForegroundColor = ConsoleColor.Red; | ||
| Console.WriteLine($"\nERROR: {ex.Message}"); | ||
| Console.WriteLine("\nStack Trace:"); | ||
| Console.WriteLine(ex.StackTrace); | ||
| Console.ResetColor(); | ||
| Environment.Exit(1); | ||
| } |
| catch (Exception ex) | ||
| { | ||
| Console.ForegroundColor = ConsoleColor.Yellow; | ||
| Console.WriteLine($"⚠ Warning: Some procedures may not have recompiled: {ex.Message}"); | ||
| Console.ResetColor(); | ||
| } |
| catch (Exception ex) | ||
| { | ||
| Console.ForegroundColor = ConsoleColor.Red; | ||
| Console.WriteLine($" ✗ FAILED: usp_getJobGroups - {ex.Message}"); | ||
| Console.ResetColor(); | ||
| failureCount++; | ||
| } |
left a comment
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 39 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (tablesExist && !forceRecreate) | ||
| { | ||
| // Tables exist but user didn't specify --force | ||
| Console.ForegroundColor = ConsoleColor.Yellow; | ||
| Console.WriteLine("⚠ Effort tables already exist in VIPER database. Skipping creation."); | ||
| Console.WriteLine(); | ||
| Console.WriteLine("To recreate the schema and tables, use:"); | ||
| Console.WriteLine(" dotnet script CreateEffortDatabase.cs --force"); | ||
| Console.WriteLine(); | ||
| Console.WriteLine("To drop the schema and tables without recreating, use:"); | ||
| Console.WriteLine(" dotnet script CreateEffortDatabase.cs --drop"); | ||
| Console.ResetColor(); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Condition is always false because of access to local variable tablesExist.
| else if (tablesExist && !forceRecreate) | |
| { | |
| // Tables exist but user didn't specify --force | |
| Console.ForegroundColor = ConsoleColor.Yellow; | |
| Console.WriteLine("⚠ Effort tables already exist in VIPER database. Skipping creation."); | |
| Console.WriteLine(); | |
| Console.WriteLine("To recreate the schema and tables, use:"); | |
| Console.WriteLine(" dotnet script CreateEffortDatabase.cs --force"); | |
| Console.WriteLine(); | |
| Console.WriteLine("To drop the schema and tables without recreating, use:"); | |
| Console.WriteLine(" dotnet script CreateEffortDatabase.cs --drop"); | |
| Console.ResetColor(); | |
| return 0; | |
| } |
| foreach (var kvp in legacyMothraIdToSignatures | ||
| .Where(kvp => kvp.Value.Contains(sig) && orphanedMothraIds.Contains(kvp.Key))) | ||
| { | ||
| isOrphaned = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This assignment to kvp is useless, since its value is never read.
| foreach (var kvp in legacyMothraIdToSignatures | |
| .Where(kvp => kvp.Value.Contains(sig) && orphanedMothraIds.Contains(kvp.Key))) | |
| { | |
| isOrphaned = true; | |
| break; | |
| } | |
| isOrphaned = legacyMothraIdToSignatures | |
| .Any(kvp => kvp.Value.Contains(sig) && orphanedMothraIds.Contains(kvp.Key)); |
| if (KnownValueMappings.TryGetValue(columnName, out var mappings)) | ||
| { | ||
| if (mappings.TryGetValue(legacyValue, out var expectedShadow)) | ||
| { | ||
| // "*" means any non-null value is acceptable | ||
| if ((expectedShadow == "*" && shadowValue != "NULL") || | ||
| string.Equals(expectedShadow, shadowValue, StringComparison.OrdinalIgnoreCase)) | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (mappings.TryGetValue(legacyValue, out var expectedShadow)) | ||
| { | ||
| // "*" means any non-null value is acceptable | ||
| if ((expectedShadow == "*" && shadowValue != "NULL") || | ||
| string.Equals(expectedShadow, shadowValue, StringComparison.OrdinalIgnoreCase)) | ||
| return true; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| catch (Exception ex) | ||
| { | ||
| Console.ForegroundColor = ConsoleColor.Red; | ||
| Console.WriteLine($"ERROR: {ex.Message}"); | ||
| Console.WriteLine($"Stack Trace: {ex.StackTrace}"); | ||
| Console.ResetColor(); | ||
| Environment.Exit(1); | ||
| } |
| catch | ||
| { | ||
| // Transaction already rolled back | ||
| } |
| catch (Exception rollbackEx) | ||
| { | ||
| Console.ForegroundColor = ConsoleColor.Yellow; | ||
| Console.WriteLine($"⚠ WARNING: Transaction rollback failed: {rollbackEx.Message}"); | ||
| Console.ResetColor(); | ||
| } |
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($" ✗ Cannot connect to legacy Efforts database: {ex.Message}"); | ||
| Console.WriteLine(" Check the 'Effort' connection string in appsettings"); | ||
| return false; | ||
| } |
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine(" ⚠ WARNING: Could not verify VIPER.users.Person table. MothraId mapping may fail."); | ||
| Console.WriteLine($" Exception: {ex.Message}"); | ||
| } |
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($" ⚠ WARNING: Could not check legacy data: {ex.Message}"); | ||
| } |
left a comment
There was a problem hiding this comment.
Copilot reviewed 36 out of 39 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```sql | ||
| USE dictionary; | ||
| GRANT SELECT TO [YourApplicationUser]; | ||
| ``` |
There was a problem hiding this comment.
These examples grant database-wide SELECT on the entire dictionary database to the Effort application user, which is broader than necessary and exposes all tables in that database if the app account is compromised (e.g., via SQL injection). With this level of access an attacker could exfiltrate unrelated, potentially sensitive data from dictionary, not just the specific lookup tables your stored procedures use. Instead of GRANT SELECT at the database level, grant SELECT only on the specific tables/views Effort needs (e.g., dictionary.dbo.dvtTitle and dictionary.dbo.dvtTitle_Ray), or a dedicated schema containing just those objects.
| ```sql | ||
| USE EvalHarvest; | ||
| GRANT SELECT TO [YourApplicationUser]; | ||
| ``` |
There was a problem hiding this comment.
Granting database-wide SELECT on the EvalHarvest database gives the Effort application user read access to all evaluation tables, not just the one or few actually referenced by the Effort stored procedures. If an attacker can run arbitrary queries using the app’s SQL credentials, this over-broad permission lets them pull any data from EvalHarvest, increasing blast radius for a compromise. Prefer granting SELECT only on the specific evaluation tables/views Effort needs rather than at the whole-database level.
| ```sql | ||
| USE idcards; | ||
| GRANT SELECT TO [YourApplicationUser]; | ||
| ``` |
There was a problem hiding this comment.
These GRANT SELECT statements on the entire idcards database give the Effort application user unrestricted read access to all ID card data, which is likely sensitive and far beyond what the Effort reports require. If the application is exploited or its SQL credentials are abused, an attacker could dump every table in idcards, not just the specific idcard table used by Effort. To reduce impact, grant SELECT only on the needed table(s)/view(s) such as idcards.dbo.idcard, or confine required objects to a dedicated schema and grant access at that narrower scope.
1st draft of planning docs
Technical Implementation
Execution & Reference