Feature/userinfo - #273
Conversation
Bundle ReportChanges will increase total bundle size by 50.15kB (2.19%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
===========================================
+ Coverage 42.38% 63.40% +21.01%
===========================================
Files 994 159 -835
Lines 49877 6222 -43655
Branches 5887 1330 -4557
===========================================
- Hits 21142 3945 -17197
+ Misses 27798 2049 -25749
+ Partials 937 228 -709
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![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.
This comment was marked as resolved.
This comment was marked as resolved.
…modal) and responds to code-quality bot userinfo recommendations
22567dc to
694ecd4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 219 out of 221 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/Areas/Directory/Models/IndividualSearchResultWithIDs.cs:49
- This branch removes the null-guard on
PostalAddress, butLdapUserContact.PostalAddressis declared= null!and is only assigned when the LDAP entry actually contains apostalAddressattribute, so it can be null at runtime. For a contact without a postal address this will throw aNullReferenceException. Note the base classIndividualSearchResulthandles the same field null-safely (ldapUserContact.PostalAddress?.Replace(...) ?? ""), so the two paths are now inconsistent. Please restore the null-safe access here.
PostalAddress = ldapUserContact.PostalAddress.Replace("$", '\n'.ToString());
|
@JasonRobertFrancis Three threads still open after c18d9c8:
|
rlorenzo
left a comment
rlorenzo
left a comment
There was a problem hiding this comment.
Medium and low items from the same pass, inline.
| catch (Exception ex) when (ex is DbException || ex is InvalidOperationException) | ||
| { | ||
| // Exceptions during student info retrieval are caught and ignored to allow other directory details to load. | ||
| _logger.LogWarning(ex, "PopulateStudentInfoAsync failed"); |
There was a problem hiding this comment.
@JasonRobertFrancis 27 catch blocks in this file and none rethrow, so a SIS or UCPath outage renders a page that looks complete with sections silently missing. Worth surfacing a partial-data notice.
There was a problem hiding this comment.
@JasonRobertFrancis Half done. RecordSectionFailure:298 populates UnavailableSections from 24 call sites and its own comment says "so the view can show a partial-data notice", but nothing ever reads the list. UserInfo.cshtml is the only view bound to UserInfoResult and it never references the property, so a failed section still renders as merely empty. Please render the notice.
There was a problem hiding this comment.
@JasonRobertFrancis Still open at d21de551. RecordSectionFailure fills UnavailableSections from 24 call sites and UserInfo.cshtml never reads the property, so a SIS or UCPath outage renders a page that looks complete with sections quietly missing. Instinct failures do reach the list at line 1255, but ErrorMessage itself goes nowhere, so the reason disappears with it. A banner listing Model.UnavailableSections at the top of the view covers both.
|
@JasonRobertFrancis I was testing this on TEST: https://secure-test.vetmed.ucdavis.edu/2/UserInfo/02725606 and comparing against VIPER1: https://secure-test.vetmed.ucdavis.edu/default.cfm?page=userinfo&id=1000610632&mothraID=02725606
|
For the effective date, it looks like this is pulling the effective date of the last change to the position. It might make more sense to show the effdt on the job record (the effective date of the last change to the job) in the summary and history views. Employees have an original hire date, but not a hire date to a specific job. I would also suggest removing the reports to column from the uc path history. I removed this from the uc path view on the current directory because it can be ambiguous when looking at historical data (which is why it looks like Rex is reporting to Dan). I checked Rex's permissions on test and the counts look good. The prod user info page is double counting some permissions. |
| { | ||
| } | ||
|
|
||
| public virtual DbSet<AbsenceCalendarDV> AbsenceCalendarDVs { get; set; } |
There was a problem hiding this comment.
@JasonRobertFrancis The four new contexts scaffold 182 entities and the service queries 9 of them: PPS has 134 DbSets and uses VwPeople, VwPersonJobPositionAlls and VwPersonJobPositions; IDCards has 25 and uses IdCards, DvtCardStatuses and DvtReasons; EquipmentLoan has 13 and uses Loans; Keys has 10 and uses Keys and KeyAssignments. That is 10,531 of the 30,584 lines this PR adds, and it pulls in tables no one is querying right now, like UcpathMissingPerson20190821 and UcpathmissingpersonBk. Every one of them now has to track its database by hand. Please cut each context down to what it queries; re-scaffolding a view later takes a minute.
There was a problem hiding this comment.
I'm guessing these tables were pulled in by the EF scaffolding tool (our pre-AI method of automating the creation of context classes). I'm OK with leaving these in because it's likely things will need them in the future, except for tables with names indicating they are a backup or copy, which may or may not exist in production and could probably be deleted from the DB altogether.
| }; | ||
|
|
||
| await _aaudContext.Database.ExecuteSqlRawAsync( | ||
| "EXEC AAUD.dbo.usp_get_CurrentOrFutureTermForUser @pidm = @pidm, @loginID = NULL, @termCode = @termCode OUTPUT", |
There was a problem hiding this comment.
@JasonRobertFrancis UserInfoService makes 10 SqlQueryRaw/ExecuteSqlRawAsync calls against _sisContext and _aaudContext, and both contexts also serve EF entity queries in this same file. CLAUDE.md keeps raw SQL to non-EF tables reached through GetConnectionString(), because mixing the two causes auth failures. The SIS stored procs are a fair reason to reach for raw SQL, so a dedicated context or a plain connection for them would settle it. One more on line 471: EXEC AAUD.dbo.usp_get_CurrentOrFutureTermForUser hardcodes the database name, which ignores whatever database the connection string actually points at.
| { | ||
| try | ||
| { | ||
| var ldapUser = LdapService.GetUserByID(result.IamId); |
There was a problem hiding this comment.
@JasonRobertFrancis PopulateDirectoryInfoAsync calls the synchronous LdapService.GetUserByID on line 315, so every page load parks a thread-pool thread for the LDAP round trip. DirectoryController already does this, so it predates your branch and I am happy for it to go to its own ticket. Raising it because you just batched the N+1 in this request path and this is the blocking call left on it.
| <title>@ViewData["Title"] - VIPER(2.0)</title> | ||
| <link rel="stylesheet" href="~/css/site.css" asp-append-version="true" /> | ||
| @if (HttpHelper.HttpContext != null && HttpHelper.HttpContext.Request.Path.ToString().ToLower().Contains("/directory")) | ||
| @if (HttpHelper.HttpContext != null && (HttpHelper.HttpContext.Request.Path.ToString().ToLower().Contains("/directory") || HttpHelper.HttpContext.Request.Path.ToString().ToLower().Contains("/userinfo"))) |
There was a problem hiding this comment.
@JasonRobertFrancis Two files carry edits this feature does not need. launchSettings.json flips launchBrowser to false and strips the file's BOM, which diverges from main for everyone who runs the project. This layout renames UserHelper to userHelper in five places and drops its BOM alongside the one change it does need, the /userinfo check that loads directory.css. Every MVC page renders this file, so holding its diff to that css line makes it easier to review now and to revert later.
| if (string.IsNullOrWhiteSpace(apiUrl)) | ||
| { | ||
| const string errMsg = "Instinct:ApiUrl is not configured"; | ||
| _logger.LogWarning("Instinct API: {ErrorMessage}", errMsg); |
There was a problem hiding this comment.
@JasonRobertFrancis The switch to ILogger reads well. One gap: LogSanitizer shows up once, on line 720, and _logger.LogWarning("Instinct API: {ErrorMessage}", errMsg) here writes a raw upstream HTTP response body to the log. The structured placeholder keeps this out of log-forging range, so it is minor, but sending the Instinct and IAM error strings through SanitizeString() would match the rest of the codebase.
| || new[] { u.MailId, u.LoginId, u.SpridenId, u.Pidm, u.MothraId, u.EmployeeId, u.IamId } | ||
| .Any(id => id != null && id.Contains(search))) | ||
| .Where(u => u.Current != 0) | ||
| .Where(u => u.Current != 0 || u.Future != 0) |
There was a problem hiding this comment.
@JasonRobertFrancis CmsUserPhotoService.ResolveIdsAsync still filters u.Current != 0, so the future-only users this adds to the results never resolve there. Table.cshtml:41 requests /api/cms/photos/by-mothra/{mothraId}?altPhoto=true, and with no mailId or iamId to fall back on, the miss returns (null, null) and the student gets the generic no-picture placeholder from PhotoService:49 instead of their own photo. Card.cshtml:79 goes through the by-mail route, so the supplied mailId survives the miss and the ID card photo still loads, but iamId stays null and the alternate photo never resolves. Matching that filter to Current != 0 || Future != 0 keeps the two definitions of a directory person in step.
There was a problem hiding this comment.
I'm having a hard time figuring out what the proposed solution is from this text - is it supplying the mail id when looking up a photo?
There was a problem hiding this comment.
@bsedwards No, the mail id is a symptom, not the fix. The change is one line in CmsUserPhotoService.ResolveIdsAsync, line 90:
- var query = _aaudContext.AaudUsers.AsNoTracking().Where(u => u.Current != 0);
+ var query = _aaudContext.AaudUsers.AsNoTracking().Where(u => u.Current != 0 || u.Future != 0);That filter is the photo service's definition of a directory person, and this PR just widened the search's definition to Current != 0 || Future != 0. Until the two match, a future-only student shows up in results and then fails to resolve for their photo.
No description provided.