fix(validation): restore ValidationComponent accidentally deleted#444
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a new ValidationComponent (TS + HTML) and registers it in AppModule, introduces VALIDATION_STATUS_SEVERITY, and replaces per-status tag markup with severity-driven tags in project and deposit brief-view templates and components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@projects/sonar/src/app/record/validation/validation.component.html`:
- Around line 90-95: The template uses an undefined pipe "nl2br" (seen in
validation.component.html on the binding [innerHtml]="log.comment | nl2br"),
which will throw "Unknown pipe 'nl2br'"; either implement and export a reusable
Nl2BrPipe and add it to the relevant NgModule's declarations/exports so all
templates (validation.component.html, brief-view.component.html,
document.component.html, etc.) can use it, or remove the pipe and change the
template to a safe alternative: sanitize the transformed HTML via
DomSanitizer.bypassSecurityTrustHtml(...) in the component that exposes
log.comment (e.g., ValidationComponent) and bind that sanitized string to
[innerHtml], or bind plain text via [textContent]="log.comment" to avoid HTML
altogether — update the component(s) and module declarations accordingly.
In `@projects/sonar/src/app/record/validation/validation.component.ts`:
- Around line 106-128: The spinner is only hidden on success; wrap the update
flow (recordService.update(this.type, this.record.id, this.record)) with an RxJS
finalize so this.spinner.hide() always runs, and add error handling (subscribe
error handler or pipe catchError) to surface failures via
this.messageService.add({ severity: 'error', detail:
this.translateService.instant('...'), life: CONFIG.MESSAGE_LIFE }) so users get
notified; keep setting this.validation.action/this.validation.comment before the
call but ensure any UI state cleanup (spinner.hide) and error notification
happen in the finalize/error path rather than only inside the success subscribe
callback.
- Around line 66-71: The component currently subscribes to userService.user$
inside ngOnInit and calls recordService.update() without error handling or
teardown; update the component to implement OnDestroy (or use
takeUntilDestroyed) and track subscriptions (e.g., a private Subscription or
subject) so you unsubscribe in ngOnDestroy, replace raw subscriptions in
ngOnInit (userService.user$) and the recordService.update() call with versions
that are cleaned up, and add error handling to the recordService.update()
subscription to handle/report errors rather than ignoring them.
🧹 Nitpick comments (1)
projects/sonar/src/app/record/validation/validation.component.html (1)
78-79: Make the logs toggle label translatable and state-aware.Right now the label is static and not translated; it can also indicate the open/closed state.
💡 Suggested tweak
- <p-button [text]="true" [label]="'Show logs'" (onClick)="showLogs = !showLogs" /> + <p-button + [text]="true" + [label]="(showLogs ? 'Hide logs' : 'Show logs') | translate" + (onClick)="showLogs = !showLogs" + />
8a28726 to
a8ffb23
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@projects/sonar/src/app/record/validation/validation.component.ts`:
- Around line 88-90: The isModerator() method can throw if this.user is
undefined; update isModerator() to defensively check this.user (and its
is_moderator property) before returning, e.g. return false when this.user is
falsy, so calls before the user$ subscription emits won't crash. Locate the
isModerator() function in validation.component.ts and add a null/undefined guard
around this.user/is_moderator and return a safe default.
🧹 Nitpick comments (2)
projects/sonar/src/app/record/validation/validation.component.ts (1)
60-61: Typo: "Wether" → "Whether"- // Wether to show logs table or not. + // Whether to show logs table or not.projects/sonar/src/app/record/project/brief-view/brief-view.component.html (1)
24-42: Consider adding a@defaultcase for unexpected statuses.The switch handles five known statuses but has no fallback. If the backend ever returns an unexpected status, no tag renders and there's no indication of a problem. A default case improves defensiveness.
💡 Suggested addition
`@case`('rejected') { <p-tag severity="danger" [value]="(record.metadata.validation.status) | translate" /> } + `@default` { + <p-tag severity="secondary" [value]="(record.metadata.validation.status) | translate" /> + } }
b2d0fbc to
8f66099
Compare
projects/sonar/src/app/record/project/brief-view/brief-view.component.html
Outdated
Show resolved
Hide resolved
8f66099 to
ab66357
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@projects/sonar/src/app/record/validation/validation.component.html`:
- Around line 93-99: The template risks a runtime error when rendering
log.user.name if log.user is null/undefined; update the binding that references
log.user.name in the ng-template for the logs table to guard against missing
users by using optional chaining on the log.user reference and provide a
sensible fallback display (e.g., a dash or "Unknown User") so the row renders
safely when user is absent.
In `@projects/sonar/src/app/record/validation/validation.component.ts`:
- Around line 79-81: Guard the isOwner() method against a missing
validation.user by checking that this.validation.user exists before accessing
$ref; update isOwner() (the function that compares
userService.getUserRefEndpoint() to this.validation.user.$ref) to return false
when this.validation.user is undefined (use an explicit null/undefined check or
optional chaining) so you never dereference $ref on an undefined object.
🧹 Nitpick comments (1)
projects/sonar/src/app/record/project/brief-view/brief-view.component.html (1)
24-26: Minor inconsistency in translation key pattern.The deposit brief-view uses
('deposit_status_' + record.metadata.status) | translatewhile this template usesrecord.metadata.validation.status | translatedirectly. If this is intentional due to different i18n key structures for deposits vs projects, this is fine. Otherwise, consider aligning the patterns for maintainability.
c00ed22 to
1d50ba9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@projects/sonar/src/app/record/validation/validation.component.ts`:
- Around line 60-61: Fix the typo in the comment above the showLogs property in
validation.component.ts: change "Wether to show logs table or not." to "Whether
to show logs table or not." so the inline comment for the showLogs = false;
declaration is correct.
🧹 Nitpick comments (1)
projects/sonar/src/app/enum/validation.ts (1)
34-40: Consider stricter typing for compile-time completeness.Using
Record<validation_status, string>instead ofRecord<string, string>would ensure the compiler flags if a new enum member is added tovalidation_statuswithout a corresponding severity entry. That said, since consumers index with dynamicstringkeys from record metadata, the current typing works fine at runtime.♻️ Suggested type tightening
-export const VALIDATION_STATUS_SEVERITY: Record<string, string> = { +export const VALIDATION_STATUS_SEVERITY: Record<validation_status, string> = {
projects/sonar/src/app/record/validation/validation.component.ts
Outdated
Show resolved
Hide resolved
1d50ba9 to
3d1f710
Compare
3d1f710 to
c0e3380
Compare
c0e3380 to
84cecc7
Compare
The ValidationComponent was removed in commit ced084f during the signal refactoring. This component is required for the project validation workflow in HEPVS. - Restore validation.component.ts and validation.component.html - Update import path for enums (./constants -> ../../enum/validation) - Add ValidationComponent to app.module.ts declarations - Extract validation status to severity mapping into a shared constant VALIDATION_STATUS_SEVERITY in validation.ts. Use getter properties in both project and deposit brief-view components to simplify templates and centralize the status-to-severity logic. - Adds the current user organisation information when a new project is created. - Adds a new method to return the current user organisation to the user service. - Force ngx-formly to version 7.0.0 to prevent issues with arrays. Co-Authored-by: Pascal Repond <pascal.repond@rero.ch> Co-Authored-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>
84cecc7 to
5b2037e
Compare
The ValidationComponent was removed in commit ced084f during
the signal refactoring. This component is required for the project
validation workflow in HEPVS.
VALIDATION_STATUS_SEVERITY in validation.ts. Use getter properties in
both project and deposit brief-view components to simplify templates
and centralize the status-to-severity logic.