fix(organsation): add the organisation to a new project#446
Closed
jma wants to merge 2 commits intorero:stagingfrom
Closed
fix(organsation): add the organisation to a new project#446jma wants to merge 2 commits intorero:stagingfrom
jma wants to merge 2 commits intorero:stagingfrom
Conversation
Contributor
jma
commented
Feb 5, 2026
jma
commented
- 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.
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. Co-Authored-by: Pascal Repond <pascal.repond@rero.ch>
WalkthroughAdds a per-resource Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@projects/sonar/src/app/app-routing.module.ts`:
- Around line 400-412: preCreateRecord currently assumes record.metadata exists
and will throw if it's undefined; initialize record.metadata (e.g., if
(!record.metadata) record.metadata = {}) before assigning
record.metadata.organisation, then proceed to set the $ref using
this.apiService.getRefEndpoint('organisations',
this.userService.getOrganisationCode()) only when organisationCode is truthy,
and finally return record; update the preCreateRecord implementation to
defensively create metadata to avoid runtime errors.
In `@projects/sonar/src/app/record/validation/validation.component.ts`:
- Around line 66-72: The component currently subscribes to userService.user$ in
ngOnInit without storing the Subscription and also assumes
this.record.metadata.validation exists; update the component to implement
OnDestroy, add an instance property like userSub: Subscription, import
Subscription and OnDestroy, assign the subscription returned from
this.userService.user$.subscribe(...) to userSub in ngOnInit, call
this.userSub.unsubscribe() inside ngOnDestroy, and guard the validation
assignment (e.g., use optional chaining or a fallback) when setting
this.validation from this.record.metadata.validation to avoid null/undefined
access.
🧹 Nitpick comments (1)
projects/sonar/src/app/user.service.ts (1)
138-140: Consider adding optional chaining for defensive consistency.While
getOrganisationCode()is called only in thepreCreateRecordhook (line 402 of app-routing.module.ts), and the application'sAPP_INITIALIZERensures the user is loaded before any routing occurs, the method could be more defensive. The caller checksif (organisationCode), suggesting it handles falsy returns, but the current implementation will throw rather than return a falsy value iforganisationorcodeis missing.Adding optional chaining aligns with defensive coding practices seen in other similar methods like
getPublicInterfaceLink()at line 148, which also accessesorganisationwithout checks:Suggested improvement
getOrganisationCode(): string { - return this._user.organisation.code; + return this._user?.organisation?.code ?? ''; }
Comment on lines
+400
to
+412
| preCreateRecord: (record: any) => { | ||
| // add organisation reference to the new record | ||
| const organisationCode = this.userService.getOrganisationCode(); | ||
| if (organisationCode) { | ||
| record.metadata.organisation = { | ||
| $ref: this.apiService.getRefEndpoint( | ||
| 'organisations', | ||
| organisationCode | ||
| ) | ||
| }; | ||
| } | ||
| return record; | ||
| }, |
There was a problem hiding this comment.
Ensure record.metadata exists before assignment.
The code assumes record.metadata already exists. If preCreateRecord is called with a record where metadata is undefined, this will throw a runtime error on line 404.
🛡️ Proposed defensive fix
preCreateRecord: (record: any) => {
// add organisation reference to the new record
const organisationCode = this.userService.getOrganisationCode();
if (organisationCode) {
+ record.metadata = record.metadata || {};
record.metadata.organisation = {
$ref: this.apiService.getRefEndpoint(
'organisations',
organisationCode
)
};
}
return record;
},🤖 Prompt for AI Agents
In `@projects/sonar/src/app/app-routing.module.ts` around lines 400 - 412,
preCreateRecord currently assumes record.metadata exists and will throw if it's
undefined; initialize record.metadata (e.g., if (!record.metadata)
record.metadata = {}) before assigning record.metadata.organisation, then
proceed to set the $ref using this.apiService.getRefEndpoint('organisations',
this.userService.getOrganisationCode()) only when organisationCode is truthy,
and finally return record; update the preCreateRecord implementation to
defensively create metadata to avoid runtime errors.
Comment on lines
+66
to
+72
| ngOnInit(): void { | ||
| this.validation = this.record.metadata.validation; | ||
|
|
||
| this.userService.user$.subscribe((user) => { | ||
| this.user = user; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Memory leak: Subscription to user$ is never unsubscribed.
The subscription created on line 69 is not stored and will not be cleaned up when the component is destroyed. This causes a memory leak. The ReviewComponent in this codebase handles this correctly by storing the subscription and calling unsubscribe() in ngOnDestroy.
Additionally, line 67 assumes this.record.metadata.validation exists without a null check, which could cause a runtime error.
🔒️ Proposed fix: Store and clean up subscription
-export class ValidationComponent implements OnInit {
+export class ValidationComponent implements OnInit, OnDestroy {
private userService: UserService = inject(UserService);
private recordService: RecordService = inject(RecordService);
private translateService: TranslateService = inject(TranslateService);
private messageService: MessageService = inject(MessageService);
private confirmationService: ConfirmationService = inject(ConfirmationService);
private spinner: NgxSpinnerService = inject(NgxSpinnerService);
+ // Subscription holder
+ private subscription: Subscription = new Subscription();
+
// ... other properties ...
ngOnInit(): void {
- this.validation = this.record.metadata.validation;
+ this.validation = this.record?.metadata?.validation;
- this.userService.user$.subscribe((user) => {
+ this.subscription.add(this.userService.user$.subscribe((user) => {
this.user = user;
- });
+ }));
}
+ ngOnDestroy(): void {
+ this.subscription.unsubscribe();
+ }Also add OnDestroy and Subscription to the imports:
-import { Component, ElementRef, inject, Input, OnInit, ViewChild } from '@angular/core';
+import { Component, ElementRef, inject, Input, OnDestroy, OnInit, ViewChild } from '@angular/core';
+import { Subscription } from 'rxjs';🤖 Prompt for AI Agents
In `@projects/sonar/src/app/record/validation/validation.component.ts` around
lines 66 - 72, The component currently subscribes to userService.user$ in
ngOnInit without storing the Subscription and also assumes
this.record.metadata.validation exists; update the component to implement
OnDestroy, add an instance property like userSub: Subscription, import
Subscription and OnDestroy, assign the subscription returned from
this.userService.user$.subscribe(...) to userSub in ngOnInit, call
this.userSub.unsubscribe() inside ngOnDestroy, and guard the validation
assignment (e.g., use optional chaining or a fallback) when setting
this.validation from this.record.metadata.validation to avoid null/undefined
access.
* 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. Co-Authored-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>
jma
force-pushed
the
maj-add-org-to-project
branch
from
213b436 to
abe5258
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/sonar/src/app/app-routing.module.ts (1)
662-669:⚠️ Potential issue | 🟡 MinorProtect against missing route children in
_documentAggregationsOrder.Line 662 assumes
this.route.snapshot.children[0]always exists; if not, this will throw on initial load or non-search routes.Proposed fix
- const { view } = this.route.snapshot.children[0].params; + const firstChild = this.route.snapshot.children[0]; + if (!firstChild) { + return of(null); + } + const { view } = firstChild.params ?? {}; @@ - if (this.route.snapshot.children[0].queryParams.collection_view) { + if (firstChild.queryParams?.collection_view) { params = params.set('collection', '1'); }
🤖 Fix all issues with AI agents
In `@projects/sonar/src/app/app-routing.module.ts`:
- Around line 400-414: preCreateRecord currently dereferences
this.userService.currentUser() and user.organisation without checks; update the
handler to guard against null/undefined by ensuring record.metadata is
initialized (e.g. if (!record.metadata) record.metadata = {}), retrieve const
user = this.userService.currentUser() and only access user.organisation?.code
and user.pid if user exists (use optional chaining or an explicit if (user)
check), and only call this.apiService.getRefEndpoint('organisations',
organisationCode) when organisationCode is truthy; apply these checks around the
existing assignments in preCreateRecord to avoid runtime crashes.
Comment on lines
+400
to
+414
| preCreateRecord: (record: any) => { | ||
| const user = this.userService.currentUser(); | ||
| // add organisation reference to the new record | ||
| const organisationCode = user.organisation.code; | ||
| if (organisationCode) { | ||
| record.metadata.organisation = { | ||
| $ref: this.apiService.getRefEndpoint( | ||
| 'organisations', | ||
| organisationCode | ||
| ) | ||
| }; | ||
| } | ||
| const userPid = user.pid; | ||
| if (userPid) { | ||
| record.metadata.user = { |
There was a problem hiding this comment.
Guard null currentUser() / organisation before dereference.
Line 403 assumes user and user.organisation are always present; if currentUser() isn’t ready, this will throw. Consider optional chaining (and still initialize record.metadata as per the earlier comment).
Proposed fix
- const user = this.userService.currentUser();
+ const user = this.userService.currentUser();
// add organisation reference to the new record
- const organisationCode = user.organisation.code;
+ const organisationCode = user?.organisation?.code;
if (organisationCode) {
record.metadata.organisation = {
$ref: this.apiService.getRefEndpoint(
'organisations',
organisationCode
)
};
}
- const userPid = user.pid;
+ const userPid = user?.pid;
if (userPid) {
record.metadata.user = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| preCreateRecord: (record: any) => { | |
| const user = this.userService.currentUser(); | |
| // add organisation reference to the new record | |
| const organisationCode = user.organisation.code; | |
| if (organisationCode) { | |
| record.metadata.organisation = { | |
| $ref: this.apiService.getRefEndpoint( | |
| 'organisations', | |
| organisationCode | |
| ) | |
| }; | |
| } | |
| const userPid = user.pid; | |
| if (userPid) { | |
| record.metadata.user = { | |
| preCreateRecord: (record: any) => { | |
| const user = this.userService.currentUser(); | |
| // add organisation reference to the new record | |
| const organisationCode = user?.organisation?.code; | |
| if (organisationCode) { | |
| record.metadata.organisation = { | |
| $ref: this.apiService.getRefEndpoint( | |
| 'organisations', | |
| organisationCode | |
| ) | |
| }; | |
| } | |
| const userPid = user?.pid; | |
| if (userPid) { | |
| record.metadata.user = { |
🤖 Prompt for AI Agents
In `@projects/sonar/src/app/app-routing.module.ts` around lines 400 - 414,
preCreateRecord currently dereferences this.userService.currentUser() and
user.organisation without checks; update the handler to guard against
null/undefined by ensuring record.metadata is initialized (e.g. if
(!record.metadata) record.metadata = {}), retrieve const user =
this.userService.currentUser() and only access user.organisation?.code and
user.pid if user exists (use optional chaining or an explicit if (user) check),
and only call this.apiService.getRefEndpoint('organisations', organisationCode)
when organisationCode is truthy; apply these checks around the existing
assignments in preCreateRecord to avoid runtime crashes.
Contributor
|
Merged in #444 |
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
2 participants
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.