Feature/userinfo - #273
Conversation
Bundle ReportChanges will increase total bundle size by 280 bytes (0.01%) ⬆️. 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
==========================================
- Coverage 45.36% 43.73% -1.63%
==========================================
Files 948 1108 +160
Lines 49529 53518 +3989
Branches 6696 7034 +338
==========================================
+ Hits 22467 23408 +941
- Misses 26096 29067 +2971
- Partials 966 1043 +77
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.
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. |
|
@JasonRobertFrancis, I've resolved all my open comment threads. Some minor issues:
|
rlorenzo
left a comment
rlorenzo
left a comment
There was a problem hiding this comment.
@JasonRobertFrancis c3de7d7 "Represses Instinct error if API is not available" is on Development only, not on feature/userinfo. It is the commit that actually suppresses the Instinct error on TEST (IsDevelopmentEnvironment to IsInstinctOptionalEnvironment, covering Test as well as Development). Since Development is never a base and never merges to main, that change disappears when this PR merges. Please cherry-pick it onto feature/userinfo and re-merge into Development.
| string? mothraId, bool altPhoto, CancellationToken ct) | ||
| { | ||
| var photo = await _photoService.GetUserPhotoAsync(mailId, loginId, iamId, mothraId, altPhoto, ct); | ||
| CmsUserPhotoResult? photo = altPhoto |
There was a problem hiding this comment.
altPhoto=true used to mean "prefer the alternate photo, fall back to the ID card, then the placeholder". After this split it means "alternate photo or 404", and the two list views still send it, so every person without an uploaded profile photo now renders the #error template instead of their ID-card photo. Confirmed on TEST: /2/Directory search results have no photos, while PROD's /2/Directory still shows them (it goes through getbase64image.cfm?...&altphoto=1, which keeps the fallback).
GetAlternatePhotoAsync returning null is right for UserInfo's dedicated "Alternative Photo" slot, but it needs to be a distinct mode rather than a redefinition of altPhoto. Suggest keeping altPhoto=true as prefer-with-fallback and adding a separate strict flag for UserInfo:
CmsUserPhotoResult? photo = altPhotoOnly
? await _photoService.GetAlternatePhotoAsync(mailId, loginId, iamId, mothraId, ct)
: altPhoto
? await _photoService.GetAlternatePhotoAsync(mailId, loginId, iamId, mothraId, ct)
?? await _photoService.GetUserPhotoAsync(mailId, loginId, iamId, mothraId, ct)
: await _photoService.GetUserPhotoAsync(mailId, loginId, iamId, mothraId, ct);The alternative is dropping altPhoto=true from Table.cshtml:41 and Card.cshtml:79, but that also drops the alternate-photo preference for people who did upload one, which the legacy page honors.
| <template v-slot:body-cell-photo="props"> | ||
| <q-td :props="props"> | ||
| <q-img :src="'@HttpHelper.GetOldViperRootURL()' + '/public/utilities/getbase64image.cfm?mothraID=' + props.row.mothraId + '&altphoto=1'" style="width:87px; height:111px" fit="cover" :alt="'Photo of ' + props.row.displayFirstName + ' ' + props.row.displayLastName"> | ||
| <q-img :src="'@HttpHelper.GetRootURL()' + '/api/cms/photos/by-mothra/' + props.row.mothraId + '?altPhoto=true'" style="width:87px; height:111px" fit="cover" :alt="'Photo of ' + props.row.displayFirstName + ' ' + props.row.displayLastName"> |
There was a problem hiding this comment.
This ?altPhoto=true now 404s for anyone without an uploaded profile photo, so the #error template's grey person icon replaces what used to be their ID-card photo. This is why search results on TEST show no photos. See the note on CMSUserPhotoController.cs:58.
| <q-card-section class="col photo"> | ||
| <q-img | ||
| :src="'@HttpHelper.GetOldViperRootURL()/public/utilities/getbase64image.cfm?mailid=' + user.mailId + '&altphoto=1'" | ||
| <q-img :src="'@HttpHelper.GetRootURL()/api/cms/photos/by-iam/' + user.iamId + '?altphoto=true'" |
There was a problem hiding this comment.
Same as Table.cshtml:41: ?altphoto=true 404s without an uploaded profile photo and falls through to the grey person icon. See CMSUserPhotoController.cs:58.
| "commandName": "Project", | ||
| "dotnetRunMessages": true, | ||
| "launchBrowser": false, | ||
| "launchBrowser": true, |
There was a problem hiding this comment.
Still flipped false to true versus main, on the https profile. npm run dev opens the browser already, so this gives you two tabs. Unrelated to the feature.
| </span> | ||
| <span class="photo2"> | ||
| <b>Alternative Photo</b> | ||
| @* Not every person has an alternate photo; the endpoint 404s when there isn't |
There was a problem hiding this comment.
This comment documents an implementation that isn't here: it describes an error listener attached via addEventListener before .src is set, but the script at line 615 uses fetch plus URL.createObjectURL. Also URL.createObjectURL(blob) at line 668 is never revoked.
If the controller keeps a strict alt-only mode (see CMSUserPhotoController.cs:58), a Model.HasAlternatePhoto resolved server-side plus an @if around the <span class="photo2"> block replaces all 60 lines of this script and sidesteps the Vue-remount problem it exists to work around.
| public async Task GetByMailId_AltPhotoMissing_ReturnsNotFound() | ||
| { | ||
| _photoService.GetAlternatePhotoAsync("mail@example.com", null, null, null, Arg.Any<CancellationToken>()) | ||
| .Returns((CmsUserPhotoResult?)null); |
Resolves ReSharper S8969 warnings flagged by the PR-scoped gate: the compiler already narrows InstinctInfo.ErrorMessage to non-null after the preceding Assert.NotNull check, so the ! was redundant.
|
|
||
| Assert.NotNull(result); | ||
| Assert.NotNull(result.InstinctInfo?.ErrorMessage); | ||
| Assert.Contains("Downstream service unavailable", result.InstinctInfo.ErrorMessage); |
|
|
||
| Assert.NotNull(result); | ||
| Assert.NotNull(result.InstinctInfo?.ErrorMessage); | ||
| Assert.Contains("Instinct:ApiUrl is not configured", result.InstinctInfo.ErrorMessage); |
No description provided.