Feature/userinfo#231
Open
JasonRobertFrancis wants to merge 15 commits into
Open
Conversation
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
==========================================
+ Coverage 44.49% 52.91% +8.41%
==========================================
Files 895 1090 +195
Lines 51655 73655 +22000
Branches 4812 5126 +314
==========================================
+ Hits 22983 38973 +15990
- Misses 28108 34046 +5938
- Partials 564 636 +72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Contributor
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Comment on lines
+475
to
+481
| foreach (var format in _formats.Where(f => DateTime.TryParseExact(value, f, null, System.Globalization.DateTimeStyles.None, out _))) | ||
| { | ||
| if (DateTime.TryParseExact(value, format, null, System.Globalization.DateTimeStyles.None, out var result)) | ||
| { | ||
| return result; | ||
| } | ||
| } |
Comment on lines
+1704
to
+1711
| foreach (var middlePart in middleParts.Where(middlePart => middlePart.Length > 0)) | ||
| { | ||
| var variation = $"{name} {middlePart[0]}"; | ||
| if (!nameVariations.Contains(variation)) | ||
| { | ||
| nameVariations.Add(variation); | ||
| } | ||
| } |
| { | ||
| if (firstNameParts[i].Length > 0) | ||
| { | ||
| accum += " " + firstNameParts[i][0]; |
Comment on lines
+364
to
+366
| catch | ||
| { | ||
| } |
Comment on lines
+1404
to
+1406
| catch | ||
| { | ||
| } |
| public class UserInfoController : AreaController | ||
| { | ||
| public Classes.SQLContext.AAUDContext _aaud; | ||
| private UserInfoService _userInfo; |
Comment on lines
+123
to
+127
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Warning: GetUserByIamIdAsync failed: {ex.Message}"); | ||
| return null; | ||
| } |
Comment on lines
+150
to
+154
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Warning: GetUserByMothraIdAsync failed: {ex.Message}"); | ||
| return null; | ||
| } |
Comment on lines
+171
to
+174
| catch | ||
| { | ||
| return new List<string>(); | ||
| } |
Comment on lines
+228
to
+231
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"Warning: PopulateDirectoryInfoAsync LDAP failed: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Completes the Directory-area UserInfo experience by wiring up additional data sources (PPS/ID Cards/Keys/Equipment Loan), updating directory navigation to the new UserInfo route, and adding page-specific styling.
Changes:
- Registered additional EF Core DbContexts against the VIPER connection string and added a large set of scaffolded entity models for PPS/IDCards/Keys/EquipmentLoan.
- Updated the Directory card view/controller to pass a view-model that drives permission-based UI toggles and links to the new UserInfo endpoint.
- Added/adjusted utilities to support UserInfo lookups (LDAP lookup by MothraID, IAM DateTime JSON parsing).
Reviewed changes
Copilot reviewed 206 out of 208 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/wwwroot/css/userinfo.css | Adds UserInfo page styling. |
| web/wwwroot/css/directory.css | Minor formatting change. |
| web/Views/Shared/_VIPERLayout.cshtml | Loads extra CSS based on request path (directory/userinfo). |
| web/Properties/launchSettings.json | Changes dev launch behavior (launchBrowser). |
| web/Program.cs | Registers additional DbContexts for UserInfo data sources. |
| web/Models/PPS/VwStipend.cs | New PPS EF entity. |
| web/Models/PPS/VwRankStepEthnicityPayRate.cs | New PPS EF entity. |
| web/Models/PPS/VwPersonAccrual.cs | New PPS EF entity. |
| web/Models/PPS/VwPerson.cs | New PPS EF entity. |
| web/Models/PPS/VwLoa.cs | New PPS EF entity. |
| web/Models/PPS/VwJobCodeAndGroup.cs | New PPS EF entity. |
| web/Models/PPS/VwEmployee.cs | New PPS EF entity. |
| web/Models/PPS/VwAccrualsCdm.cs | New PPS EF entity. |
| web/Models/PPS/VwAccrual.cs | New PPS EF entity. |
| web/Models/PPS/VisaPermitDataDV.cs | New PPS EF entity. |
| web/Models/PPS/UpdateLog.cs | New PPS EF entity. |
| web/Models/PPS/UnionDV.cs | New PPS EF entity. |
| web/Models/PPS/UcpathVerificationItem.cs | New PPS EF entity. |
| web/Models/PPS/UcpathOverride.cs | New PPS EF entity. |
| web/Models/PPS/UcpathmissingpersonBk.cs | New PPS EF entity. |
| web/Models/PPS/UcpathMissingPerson20190821.cs | New PPS EF entity. |
| web/Models/PPS/UcpathMissingPerson.cs | New PPS EF entity. |
| web/Models/PPS/UcpathItemNote.cs | New PPS EF entity. |
| web/Models/PPS/UcdOrganizationDV.cs | New PPS EF entity. |
| web/Models/PPS/UcdFauDV.cs | New PPS EF entity. |
| web/Models/PPS/UcdEmployeeFlagsDV.cs | New PPS EF entity. |
| web/Models/PPS/UcdEmployeeFlagsDOverride.cs | New PPS EF entity. |
| web/Models/PPS/TitlecodeGroup.cs | New PPS EF entity. |
| web/Models/PPS/TimeDailyDV.cs | New PPS EF entity. |
| web/Models/PPS/ServiceCreditUnit.cs | New PPS EF entity. |
| web/Models/PPS/ServiceCredit.cs | New PPS EF entity. |
| web/Models/PPS/PsxlatitemV.cs | New PPS EF entity. |
| web/Models/PPS/PsVisaPmtDataV.cs | New PPS EF entity. |
| web/Models/PPS/PsUnionTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcSsDisclosurV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcJobGrpDescV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcJobCodeTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcJobCodesV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcFundAttribV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcExtSystemV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcdDmPsNamesPrefVnamesV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcCtoOscV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcAmSsTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsUcAmSsRcdV.cs | New PPS EF entity. |
| web/Models/PPS/PsPrimaryJobsV.cs | New PPS EF entity. |
| web/Models/PPS/PsPersonV.cs | New PPS EF entity. |
| web/Models/PPS/PsPersonalPhoneV.cs | New PPS EF entity. |
| web/Models/PPS/PsPersDataEffdtV.cs | New PPS EF entity. |
| web/Models/PPS/PsPerOrgInstV.cs | New PPS EF entity. |
| web/Models/PPS/PsJpmProfileV.cs | New PPS EF entity. |
| web/Models/PPS/PsJpmCatTypesV.cs | New PPS EF entity. |
| web/Models/PPS/PsJpmCatItemsV.cs | New PPS EF entity. |
| web/Models/PPS/PsGpRsltAcumV.cs | New PPS EF entity. |
| web/Models/PPS/PsGpAbsReasonV.cs | New PPS EF entity. |
| web/Models/PPS/PsGpAbsEaV.cs | New PPS EF entity. |
| web/Models/PPS/PsGpAbsEaStaV.cs | New PPS EF entity. |
| web/Models/PPS/PsEthnicGrpTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsErnProgramTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsEmplClassTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsEmailAddressesV.cs | New PPS EF entity. |
| web/Models/PPS/PsEarningsTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsEarningsBalV.cs | New PPS EF entity. |
| web/Models/PPS/PsDiversEthnicV.cs | New PPS EF entity. |
| web/Models/PPS/PsDeptTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsDeptBudgetErnV.cs | New PPS EF entity. |
| web/Models/PPS/PsDeptBudgetDtV.cs | New PPS EF entity. |
| web/Models/PPS/PsCountryTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsCompRatecdTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsCompensationV.cs | New PPS EF entity. |
| web/Models/PPS/PsCitizenStsTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsCitizenshipV.cs | New PPS EF entity. |
| web/Models/PPS/PsAddressTypTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsAddressesV.cs | New PPS EF entity. |
| web/Models/PPS/PsAddlPayDataV.cs | New PPS EF entity. |
| web/Models/PPS/PsActnReasonTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsActionTblV.cs | New PPS EF entity. |
| web/Models/PPS/PsAcctCdTblV.cs | New PPS EF entity. |
| web/Models/PPS/Prj2EmpidToPpsid.cs | New PPS EF entity. |
| web/Models/PPS/PrimaryJobsDV.cs | New PPS EF entity. |
| web/Models/PPS/PayItemNameDV.cs | New PPS EF entity. |
| web/Models/PPS/OrganizationDV.cs | New PPS EF entity. |
| web/Models/PPS/OdsEmployeeFlagsDV.cs | New PPS EF entity. |
| web/Models/PPS/JpmFV.cs | New PPS EF entity. |
| web/Models/PPS/JobStatusDV.cs | New PPS EF entity. |
| web/Models/PPS/JobOverride.cs | New PPS EF entity. |
| web/Models/PPS/JobHistoryDetail.cs | New PPS EF entity. |
| web/Models/PPS/JobHistory.cs | New PPS EF entity. |
| web/Models/PPS/JobActionDV.cs | New PPS EF entity. |
| web/Models/PPS/HistServiceCredit.cs | New PPS EF entity. |
| web/Models/PPS/HistloaV.cs | New PPS EF entity. |
| web/Models/PPS/HistappdisV.cs | New PPS EF entity. |
| web/Models/PPS/GoAnywhereLog.cs | New PPS EF entity. |
| web/Models/PPS/FurloughTarget.cs | New PPS EF entity. |
| web/Models/PPS/Export.cs | New PPS EF entity. |
| web/Models/PPS/EthnicityGender20210201.cs | New PPS EF entity. |
| web/Models/PPS/EthnicityGender.cs | New PPS EF entity. |
| web/Models/PPS/EmployeeHistory.cs | New PPS EF entity. |
| web/Models/PPS/EdbeffrptLog.cs | New PPS EF entity. |
| web/Models/PPS/Edbeffrpt.cs | New PPS EF entity. |
| web/Models/PPS/Dvtloa.cs | New PPS EF entity. |
| web/Models/PPS/DiversEthnicityDV.cs | New PPS EF entity. |
| web/Models/PPS/DepartmentDV.cs | New PPS EF entity. |
| web/Models/PPS/DepartmentBudgetEarnFV.cs | New PPS EF entity. |
| web/Models/PPS/Ctvhme.cs | New PPS EF entity. |
| web/Models/PPS/Ctltci.cs | New PPS EF entity. |
| web/Models/PPS/Ctlcad.cs | New PPS EF entity. |
| web/Models/PPS/CompensationFV.cs | New PPS EF entity. |
| web/Models/PPS/CompensationDV.cs | New PPS EF entity. |
| web/Models/PPS/CompensationDRateV.cs | New PPS EF entity. |
| web/Models/PPS/AuditOverride.cs | New PPS EF entity. |
| web/Models/PPS/AccountCodeDV.cs | New PPS EF entity. |
| web/Models/PPS/AbsenceResultFV.cs | New PPS EF entity. |
| web/Models/PPS/AbsenceResultDV.cs | New PPS EF entity. |
| web/Models/PPS/AbsenceEventFV.cs | New PPS EF entity. |
| web/Models/PPS/AbsenceEventDV.cs | New PPS EF entity. |
| web/Models/PPS/AbsenceCalendarDV.cs | New PPS EF entity. |
| web/Models/Keys/VwUserinfo.cs | New Keys EF entity (UserInfo view). |
| web/Models/Keys/KeyManager.cs | New Keys EF entity. |
| web/Models/Keys/KeyBuilding.cs | New Keys EF entity. |
| web/Models/Keys/KeyAssignment.cs | New Keys EF entity. |
| web/Models/Keys/Key.cs | New Keys EF entity. |
| web/Models/Keys/Import3.cs | New Keys EF entity. |
| web/Models/Keys/Import2.cs | New Keys EF entity. |
| web/Models/Keys/Import.cs | New Keys EF entity. |
| web/Models/Keys/Disposition.cs | New Keys EF entity. |
| web/Models/Keys/Building.cs | New Keys EF entity. |
| web/Models/IDCards/VwLatestIdcard.cs | New IDCards EF entity. |
| web/Models/IDCards/VwDelimitedSpecialApprover.cs | New IDCards EF entity. |
| web/Models/IDCards/VwApproverMothraId.cs | New IDCards EF entity. |
| web/Models/IDCards/PrintQueue.cs | New IDCards EF entity. |
| web/Models/IDCards/PhotoExport.cs | New IDCards EF entity. |
| web/Models/IDCards/LenelBadge.cs | New IDCards EF entity. |
| web/Models/IDCards/IgnoreList.cs | New IDCards EF entity. |
| web/Models/IDCards/IdCardToPrintQueue.cs | New IDCards EF entity. |
| web/Models/IDCards/IdCard.cs | New IDCards EF entity. |
| web/Models/IDCards/ExtVisit.cs | New IDCards EF entity. |
| web/Models/IDCards/EcoTimeBadgeExclusion.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtSvmUnit.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtSpecialty.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtSpecialApprover.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtReason.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtOverseer.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtClient.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtCardStatus.cs | New IDCards EF entity. |
| web/Models/IDCards/DvtApprover.cs | New IDCards EF entity. |
| web/Models/IDCards/Defuncted.cs | New IDCards EF entity. |
| web/Models/IDCards/DbIdCard.cs | New IDCards EF entity. |
| web/Models/IDCards/BulkLoadResult.cs | New IDCards EF entity. |
| web/Models/IDCards/Audit.cs | New IDCards EF entity. |
| web/Models/IDCards/AccessLevel.cs | New IDCards EF entity. |
| web/Models/IDCards/AccessExpiration.cs | New IDCards EF entity. |
| web/Models/EquipmentLoan/VwLoan.cs | New EquipmentLoan EF entity (view). |
| web/Models/EquipmentLoan/Reason.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/O.cs | New EquipmentLoan EF entity (OS table). |
| web/Models/EquipmentLoan/LoanNote.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/LoanItem.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/Loan.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/EmailTemplate.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/EmailSent.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/Audit.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/AssetType.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/AssetNote.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/Asset.cs | New EquipmentLoan EF entity. |
| web/Models/EquipmentLoan/AppSetting.cs | New EquipmentLoan EF entity. |
| web/Classes/Utilities/LdapService.cs | Adds LDAP lookup by MothraID. |
| web/Classes/Utilities/IamApi.cs | Adds IAM DateTime JSON conversion and response DTO tweaks. |
| web/Areas/Directory/Views/Card.cshtml | Uses view-model permissions + updates UserInfo link. |
| web/Areas/Directory/Models/UCPathResult.cs | New DTO for UCPath portion of UserInfo. |
| web/Areas/Directory/Models/LoanResult.cs | New DTO for loan portion of UserInfo. |
| web/Areas/Directory/Models/KeyResult.cs | New DTO for keys portion of UserInfo. |
| web/Areas/Directory/Models/InstinctResult.cs | New DTO for Instinct portion of UserInfo. |
| web/Areas/Directory/Models/IDCardResult.cs | New DTO for ID card portion of UserInfo. |
| web/Areas/Directory/Models/DirectoryUser.cs | New view-model encapsulating directory page permissions. |
| web/Areas/Directory/Controllers/DirectoryController.cs | Passes DirectoryUser view-model into the view. |
| .gitignore | Minor formatting change. |
Comments suppressed due to low confidence (2)
web/Views/Shared/_VIPERLayout.cshtml:20
- The layout condition for "/userinfo" currently only injects directory.css, so the new userinfo.css will never be loaded. Also, calling ToLower() repeatedly allocates and can be avoided by caching a single lowercase path value.
web/Areas/Directory/Views/Card.cshtml:7 - The view no longer uses
rapsContextorUserHelper, but they are still created in the Razor code block (and require an extra @using). Removing the unused variables reduces noise and avoids misleading future readers into thinking these objects are involved in rendering.
@using Viper.Classes.SQLContext;
@model Viper.Areas.Directory.Models.DirectoryUser;
@{
RAPSContext? rapsContext = (RAPSContext?)Context.RequestServices.GetService(typeof(RAPSContext));
IUserHelper UserHelper = new UserHelper();
ViewData["Title"] = "Directory";
}
| .userinfo ul + ul { margin-top: 0.5rem; } | ||
| .userinfo ul li { padding: 0.2rem; margin-left: -0.2rem; } | ||
| .userinfo ul li:nth-child(odd) { background-color: #f1f3f5; } | ||
| .userinfo ul li ul li { padding-left: -0.2rem; margin-left: 2.2rem; } |
Comment on lines
+120
to
+129
| /// <summary> | ||
| /// Look up User by its MothraID | ||
| /// </summary> | ||
| /// <param name="id">iamID for looking up user</param> | ||
| /// <returns>LdapUserContact</returns> | ||
| public static LdapUserContact? GetUserByMothraID(string? id) | ||
| { | ||
| if (id == null) return null; | ||
| string filter = string.Format("(ucdpersonuuid = {0})", id); | ||
| var results = SearchLdap(filter); |
Comment on lines
+467
to
+489
| public override DateTime? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| var value = reader.GetString(); | ||
| if (string.IsNullOrEmpty(value)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| foreach (var format in _formats.Where(f => DateTime.TryParseExact(value, f, null, System.Globalization.DateTimeStyles.None, out _))) | ||
| { | ||
| if (DateTime.TryParseExact(value, format, null, System.Globalization.DateTimeStyles.None, out var result)) | ||
| { | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| if (DateTime.TryParse(value, out var dt)) | ||
| { | ||
| return dt; | ||
| } | ||
|
|
||
| return null; | ||
| } |
Comment on lines
35
to
38
| public async Task<ActionResult> Index(string? useExample) | ||
| { | ||
| return await Task.Run(() => View("~/Areas/Directory/Views/Card.cshtml")); | ||
| return await Task.Run(() => View("~/Areas/Directory/Views/Card.cshtml", new DirectoryUser())); | ||
| } |
Comment on lines
+6
to
+10
| public partial class O | ||
| { | ||
| public int OsId { get; set; } | ||
|
|
||
| public string OsDescription { get; set; } = null!; |
rlorenzo
reviewed
Jun 24, 2026
rlorenzo
reviewed
Jun 24, 2026
| public UserInfoServiceTests(ITestOutputHelper output) | ||
| { | ||
| _output = output; | ||
| Console.SetOut(new ConsoleRedirector(output)); |
Contributor
There was a problem hiding this comment.
This Console.SetOut is process-global and never restored, so after this test ends the redirector still points at its now-inactive ITestOutputHelper. Later tests that call Console.WriteLine then throw InvalidOperationException: There is no currently active test (seen while merging to Development: EmailNotificationTest.RemoveInstructorScheduleAsync_* failed this way, stack-tracing back into ConsoleRedirector.WriteLine). It's order-dependent, so it can present as flaky.
Minimal fix, make the class IDisposable (xUnit calls Dispose per test):
private readonly TextWriter _originalOut = Console.Out;
public void Dispose() => Console.SetOut(_originalOut);Console.Out is shared and classes run in parallel, so a [Collection] to serialize these (or dropping the global redirect) fully closes it.
rlorenzo
reviewed
Jun 24, 2026
| userInfo.CanViewADGroups = UserHelper.HasPermission(_rapsContext, currentUser, "SVMSecure.UserInfo.ADGroups"); | ||
|
|
||
| userInfo.CanViewDirectoryDetail = true; | ||
| userInfo.CanViewStudentID = true; |
Contributor
There was a problem hiding this comment.
@JasonRobertFrancis Is this right? You are overriding the permissions checks done above to all true? Was this for debugging left over?
rlorenzo
requested changes
Jun 24, 2026
rlorenzo
left a comment
rlorenzo
left a comment
Contributor
There was a problem hiding this comment.
Also, look at the Copilot reviews. They seem valid to me.
| @@ -0,0 +1,34 @@ | |||
| .userinfo div, .userinfo ul, .userinfo p, .userinfo h1, .userinfo h2, .userinfo h3, .userinfo h4 { padding: 0; margin: 0; list-style: none; } | |||
Contributor
There was a problem hiding this comment.
How is this file being loaded? I do not see it referenced anywhere
| public IUserHelper UserHelper; | ||
| private readonly RAPSContext _rapsContext; | ||
|
|
||
| public UserInfoController( |
Contributor
There was a problem hiding this comment.
UserInfoService is built by hand from the service locator (HttpHelper.HttpContext?.RequestServices.GetService(...)) and isn't registered in Program.cs.
Register it as scoped and inject it through the constructor like the contexts already are. That drops the ! null-forgiving on the resolved services (which genuinely can be null) and makes the service testable without an HttpContext.
Also, _aaud and UserHelper should be private rather than public fields.
| @@ -0,0 +1,2186 @@ | |||
| using System.Linq; | |||
Contributor
There was a problem hiding this comment.
Two things here.
- ~25 of the catch blocks are silent (catch {} or catch-and-return-null/false/empty), so a broken stored proc or a PPS schema drift shows up as a blank section instead of an error anyone can see. At minimum log these; better, let infrastructure failures bubble and only swallow the genuine "data absent" cases.
- This file is 2,185 lines across ~25 Populate*/Get* methods. Splitting it along the data-source seams (Student / Employee / UCPath / IDCards / Keys / Loans / Instinct) would make both the error handling and the testing far more tractable.
| return null; | ||
| } | ||
|
|
||
| Console.WriteLine($"[INSTINCT SERVICE] mothraId: '{mothraId}', iamId: '{iamId}', result.MothraId: '{result.MothraId}'"); |
Contributor
There was a problem hiding this comment.
Logging potential PII info with names and IDs.
Also, should use logging levels.
Look at using nlog instead of console logs.
| public async Task<ActionResult<IEnumerable<NavMenuItem>>> Nav() | ||
| { | ||
| var nav = new List<NavMenuItem>(); | ||
| return await Task.Run(() => nav); |
Contributor
There was a problem hiding this comment.
Task.Run(() => nav) bounces trivial synchronous work onto a thread-pool thread for no benefit. Make these non-async or use Task.FromResult.
| /// <param name="altphoto">Use alternative photo</param> | ||
| /// <returns></returns> | ||
| [Route("/userPhoto")] | ||
| public async Task<ActionResult> UserPhoto(string mailID, bool altphoto = false) |
Contributor
There was a problem hiding this comment.
Declared async with no await (will warn) and stubbed to NotFound(). Add a TODO or remove it until it's implemented.
| // Effort tables are in the VIPER database's [effort] schema. | ||
| RegisterDbContext<EffortDbContext>("VIPER"); | ||
| RegisterDbContext<EvalHarvestDbContext>("EvalHarvest"); | ||
| RegisterDbContext<EquipmentLoanContext>("VIPER"); |
Contributor
There was a problem hiding this comment.
We're scaffolding 134 PPS entity classes (plus full IDCards/Keys/EquipmentLoan schemas), but UserInfo only reads a few. Every scaffolded table joins the EF model permanently, including sensitive ones we don't use (visa, citizenship, ethnicity/gender). Consider scaffolding only the views we actually query rather than the whole schema.
Not a blocker, just flagging it as long-term maintenance cost.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the UserInfo page. Ready for your review