Open
Conversation
- Deleted `ActiveGroupService` implementation and its associated tests. - Removed references to `ActiveGroupService` in `abstract-navigation-tree` and related spec files. - Deleted `NextGroupService` test files and adjusted group-related base filter usage to rely on `UserService` instead. - Updated `Group` interface to include new fields such as `displayName`, `realmId`, `identifier`, etc. - Aligned `MockAuthenticationMethodService` to use `groupIds` instead of `nextGroups`. - Adjusted `group-view.component.ts` to utilize `UserService` for handling group-related functionality.
Modified test cases to adapt to updates in the user model, including new properties for `authorities` and a distinction between `groupIds` and `groups`. These changes ensure tests align with the adjusted model structure.
WalkthroughRemoves ActiveGroupService and NextGroupService along with related example components. Refactors User/UserResource model: renames nextGroups to groupIds, changes groups storage from string[] to Group[]. Updates dependent services and components to remove ActiveGroupService references and use updated data structures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
This update introduces the `group-view` component, defining its class, layout, access level, and routing path. The new configuration supports private navigation to the `group-view` path.
Deleted the GroupViewComponent, its styling, tests, and template. Removed all references from app module and view service to clean up unused code.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/netgrif-components-core/src/lib/authentication/models/user.transformer.ts (1)
17-17: 🛠️ Refactor suggestion | 🟠 MajorRemove unused variable or address TODO.
The
groupsvariable is declared but never used. The TODO comment suggests groups parsing was intended but remains unimplemented. Sinceuser.groupsis now passed directly to the constructor (line 29), this dead code should be removed.🧹 Proposed fix
public transform(user: UserResource): User { - const groups: Array<string> = []; // TODO groups parsing - return new User(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/netgrif-components-core/src/lib/authentication/models/user.transformer.ts` at line 17, Remove the unused local variable "groups: Array<string> = []" and its TODO comment from user.transformer.ts; since the code already passes user.groups into the constructor, delete the dead declaration (or replace it with actual parsing logic if you intend to implement groups parsing) so there are no unused variables and the constructor continues to receive user.groups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/authorization/group/group-guard.service.ts`:
- Line 40: The call to this._userService.user.groups.map(...) in
GroupGuardService (variable groupOfUser) can throw if user.groups is undefined;
update the logic that derives groupOfUser to safely handle a missing groups
array (use a null-coalescing or optional chaining fallback so groupOfUser
becomes an empty array when this._userService.user.groups is undefined) and keep
the rest of the guard logic unchanged.
In
`@projects/netgrif-components-core/src/lib/authorization/permission/permission.service.ts`:
- Around line 114-123: The code in the permission loop can throw when
loggedUser.groupIds is undefined; update the condition and push logic in the
forEach to safely handle optional groupIds: change the actor check to explicitly
guard groupIds (e.g., !loggedUser.groupIds ||
!loggedUser.groupIds.includes(actorId)) and when pushing to processedActorIds
only spread loggedUser.groupIds if it is defined and an array; locate the block
inside the loop in permission.service.ts where
Object.keys(users).forEach(actorId => { ... }) and adjust the conditional and
the loggedUser.groupIds !== undefined ? processedActorIds.push(actorId,
...loggedUser.groupIds) : processedActorIds.push(actorId) to a null-safe
variant.
In `@projects/netgrif-components-core/src/lib/resources/interface/group.ts`:
- Around line 1-2: Imports in
projects/netgrif-components-core/src/lib/resources/interface/group.ts use double
quotes instead of the project's single-quote convention; update the import
statements that reference ProcessRole and Authority to use single quotes (i.e.,
change "process-role" and "authority" imports to use single quotes) so they
match the Angular project style and lint rules.
In `@projects/netgrif-components-core/src/lib/user/models/user.ts`:
- Line 3: The import statement for Group uses double quotes while the rest of
the file uses single quotes; update the import of Group (the "Group" import
declaration) to use single quotes to match the project's quote style and keep
import formatting consistent.
---
Outside diff comments:
In
`@projects/netgrif-components-core/src/lib/authentication/models/user.transformer.ts`:
- Line 17: Remove the unused local variable "groups: Array<string> = []" and its
TODO comment from user.transformer.ts; since the code already passes user.groups
into the constructor, delete the dead declaration (or replace it with actual
parsing logic if you intend to implement groups parsing) so there are no unused
variables and the constructor continues to receive user.groups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4edeb3bc-406a-4ce8-8479-cd5af2913b10
📒 Files selected for processing (32)
nae.jsonprojects/nae-example-app/src/app/app.module.tsprojects/nae-example-app/src/app/doc/active-group/active-group.component.htmlprojects/nae-example-app/src/app/doc/active-group/active-group.component.scssprojects/nae-example-app/src/app/doc/active-group/active-group.component.spec.tsprojects/nae-example-app/src/app/doc/active-group/active-group.component.tsprojects/nae-example-app/src/app/doc/group-view/group-view.component.htmlprojects/nae-example-app/src/app/doc/group-view/group-view.component.scssprojects/nae-example-app/src/app/doc/group-view/group-view.component.spec.tsprojects/nae-example-app/src/app/doc/group-view/group-view.component.tsprojects/nae-example-app/src/app/nae-example-app-view.service.tsprojects/netgrif-components-core/src/lib/authentication/models/user.transformer.spec.tsprojects/netgrif-components-core/src/lib/authentication/models/user.transformer.tsprojects/netgrif-components-core/src/lib/authorization/group/group-guard.service.tsprojects/netgrif-components-core/src/lib/authorization/permission/permission.service.tsprojects/netgrif-components-core/src/lib/groups/public-api.tsprojects/netgrif-components-core/src/lib/groups/services/active-group.service.spec.tsprojects/netgrif-components-core/src/lib/groups/services/active-group.service.tsprojects/netgrif-components-core/src/lib/groups/services/next-group-service.spec.tsprojects/netgrif-components-core/src/lib/groups/services/next-group.service.tsprojects/netgrif-components-core/src/lib/impersonation/services/impersonation.service.spec.tsprojects/netgrif-components-core/src/lib/navigation/navigation-tree/abstract-navigation-tree.component.spec.tsprojects/netgrif-components-core/src/lib/navigation/navigation-tree/abstract-navigation-tree.component.tsprojects/netgrif-components-core/src/lib/resources/interface/authority.tsprojects/netgrif-components-core/src/lib/resources/interface/group.tsprojects/netgrif-components-core/src/lib/resources/interface/user-resource.tsprojects/netgrif-components-core/src/lib/search/models/category/user-autocomplete.spec.tsprojects/netgrif-components-core/src/lib/user/models/user.tsprojects/netgrif-components-core/src/lib/utility/tests/mocks/mock-authentication-method-service.tsprojects/netgrif-components-core/src/lib/utility/tests/mocks/mock-user-resource.service.tsprojects/netgrif-components-core/src/lib/view/case-view/abstract-case-view.tsprojects/netgrif-components/src/lib/navigation/navigation-tree/navigation-tree.component.ts
💤 Files with no reviewable changes (18)
- projects/netgrif-components-core/src/lib/groups/public-api.ts
- projects/netgrif-components-core/src/lib/groups/services/next-group.service.ts
- projects/nae-example-app/src/app/doc/active-group/active-group.component.html
- projects/netgrif-components-core/src/lib/groups/services/next-group-service.spec.ts
- projects/nae-example-app/src/app/doc/group-view/group-view.component.html
- projects/nae-example-app/src/app/app.module.ts
- projects/netgrif-components-core/src/lib/navigation/navigation-tree/abstract-navigation-tree.component.ts
- projects/nae-example-app/src/app/doc/group-view/group-view.component.ts
- projects/netgrif-components-core/src/lib/groups/services/active-group.service.spec.ts
- projects/nae-example-app/src/app/doc/active-group/active-group.component.ts
- nae.json
- projects/netgrif-components-core/src/lib/groups/services/active-group.service.ts
- projects/netgrif-components-core/src/lib/navigation/navigation-tree/abstract-navigation-tree.component.spec.ts
- projects/nae-example-app/src/app/doc/active-group/active-group.component.spec.ts
- projects/nae-example-app/src/app/doc/group-view/group-view.component.scss
- projects/netgrif-components/src/lib/navigation/navigation-tree/navigation-tree.component.ts
- projects/nae-example-app/src/app/nae-example-app-view.service.ts
- projects/nae-example-app/src/app/doc/group-view/group-view.component.spec.ts
| allowedGroups = [view.access.group]; | ||
| } | ||
| const groupOfUser = this._nextGroupService.groupOfUser.map(group => group.title); | ||
| const groupOfUser = this._userService.user.groups.map(group => group.displayName); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check User construction to verify groups is always an array
ast-grep --pattern $'new User($$$)'Repository: netgrif/components
Length of output: 12371
🏁 Script executed:
# Find and examine the User class definition
fd User.ts projects/netgrif-components-core/src/lib/authentication/models/Repository: netgrif/components
Length of output: 44
🏁 Script executed:
# Check the User class constructor
cat -n projects/netgrif-components-core/src/lib/authentication/models/user.model.ts | head -100Repository: netgrif/components
Length of output: 170
🏁 Script executed:
# Get complete view of user.transformer.ts to see full context
cat -n projects/netgrif-components-core/src/lib/authentication/models/user.transformer.tsRepository: netgrif/components
Length of output: 1361
🏁 Script executed:
# Find the User model file
fd -e ts User projects/netgrif-components-core/src/lib/user/models/Repository: netgrif/components
Length of output: 44
🏁 Script executed:
# Search for User class definition in user directory
rg "class User" projects/netgrif-components-core/src/lib/user/Repository: netgrif/components
Length of output: 771
🏁 Script executed:
# Get the User class definition
cat -n projects/netgrif-components-core/src/lib/user/models/user.tsRepository: netgrif/components
Length of output: 2050
Potential runtime error: groups may be undefined.
The User class defines groups as an optional parameter (line 20 of user.ts: public groups?: Group[]), which means it can be undefined. While the emptyUser initializes it as an empty array, the UserTransformer passes user.groups directly from the API response without guarantee it's defined.
The code on line 40 calls .map() directly without null-checking:
const groupOfUser = this._userService.user.groups.map(group => group.displayName);
This will throw a runtime error if groups is undefined. Add a null-coalescing operator or defensive check:
const groupOfUser = this._userService.user.groups?.map(group => group.displayName) ?? [];
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@projects/netgrif-components-core/src/lib/authorization/group/group-guard.service.ts`
at line 40, The call to this._userService.user.groups.map(...) in
GroupGuardService (variable groupOfUser) can throw if user.groups is undefined;
update the logic that derives groupOfUser to safely handle a missing groups
array (use a null-coalescing or optional chaining fallback so groupOfUser
becomes an empty array when this._userService.user.groups is undefined) and keep
the rest of the guard logic unchanged.
Comment on lines
114
to
+123
| Object.keys(users).forEach(actorId => { | ||
| if (userPermValue === false || processedActorIds.includes(actorId) | ||
| || actorId !== loggedUser.id && !loggedUser.nextGroups?.includes(actorId)) { | ||
| || actorId !== loggedUser.id && !loggedUser.groupIds.includes(actorId)) { | ||
| return; | ||
| } | ||
| let currentUserPermission: boolean = this.getPermissionByUserOrGroup(users, permission, loggedUser); | ||
| if (currentUserPermission !== undefined) { | ||
| userPermValue = userPermValue === undefined ? currentUserPermission : userPermValue && currentUserPermission; | ||
| } | ||
| loggedUser.nextGroups !== undefined ? processedActorIds.push(actorId, ...loggedUser.nextGroups) : processedActorIds.push(actorId); | ||
| loggedUser.groupIds !== undefined ? processedActorIds.push(actorId, ...loggedUser.groupIds) : processedActorIds.push(actorId); |
There was a problem hiding this comment.
Potential TypeError if groupIds is undefined.
On line 116, loggedUser.groupIds.includes(actorId) will throw a TypeError if groupIds is undefined. The User model defines groupIds as optional (groupIds?: Array<string>), so this needs a null-safety check.
🛡️ Proposed fix to add null-safety check
Object.keys(users).forEach(actorId => {
if (userPermValue === false || processedActorIds.includes(actorId)
- || actorId !== loggedUser.id && !loggedUser.groupIds.includes(actorId)) {
+ || actorId !== loggedUser.id && !(loggedUser.groupIds?.includes(actorId) ?? false)) {
return;
}📝 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
| Object.keys(users).forEach(actorId => { | |
| if (userPermValue === false || processedActorIds.includes(actorId) | |
| || actorId !== loggedUser.id && !loggedUser.nextGroups?.includes(actorId)) { | |
| || actorId !== loggedUser.id && !loggedUser.groupIds.includes(actorId)) { | |
| return; | |
| } | |
| let currentUserPermission: boolean = this.getPermissionByUserOrGroup(users, permission, loggedUser); | |
| if (currentUserPermission !== undefined) { | |
| userPermValue = userPermValue === undefined ? currentUserPermission : userPermValue && currentUserPermission; | |
| } | |
| loggedUser.nextGroups !== undefined ? processedActorIds.push(actorId, ...loggedUser.nextGroups) : processedActorIds.push(actorId); | |
| loggedUser.groupIds !== undefined ? processedActorIds.push(actorId, ...loggedUser.groupIds) : processedActorIds.push(actorId); | |
| Object.keys(users).forEach(actorId => { | |
| if (userPermValue === false || processedActorIds.includes(actorId) | |
| || actorId !== loggedUser.id && !(loggedUser.groupIds?.includes(actorId) ?? false)) { | |
| return; | |
| } | |
| let currentUserPermission: boolean = this.getPermissionByUserOrGroup(users, permission, loggedUser); | |
| if (currentUserPermission !== undefined) { | |
| userPermValue = userPermValue === undefined ? currentUserPermission : userPermValue && currentUserPermission; | |
| } | |
| loggedUser.groupIds !== undefined ? processedActorIds.push(actorId, ...loggedUser.groupIds) : processedActorIds.push(actorId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@projects/netgrif-components-core/src/lib/authorization/permission/permission.service.ts`
around lines 114 - 123, The code in the permission loop can throw when
loggedUser.groupIds is undefined; update the condition and push logic in the
forEach to safely handle optional groupIds: change the actor check to explicitly
guard groupIds (e.g., !loggedUser.groupIds ||
!loggedUser.groupIds.includes(actorId)) and when pushing to processedActorIds
only spread loggedUser.groupIds if it is defined and an array; locate the block
inside the loop in permission.service.ts where
Object.keys(users).forEach(actorId => { ... }) and adjust the conditional and
the loggedUser.groupIds !== undefined ? processedActorIds.push(actorId,
...loggedUser.groupIds) : processedActorIds.push(actorId) to a null-safe
variant.
Comment on lines
+1
to
+2
| import {ProcessRole} from "./process-role"; | ||
| import {Authority} from "./authority"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent quote style in imports.
These imports use double quotes while Angular projects typically use single quotes by convention.
🔧 Proposed fix
-import {ProcessRole} from "./process-role";
-import {Authority} from "./authority";
+import {ProcessRole} from './process-role';
+import {Authority} from './authority';📝 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
| import {ProcessRole} from "./process-role"; | |
| import {Authority} from "./authority"; | |
| import {ProcessRole} from './process-role'; | |
| import {Authority} from './authority'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/netgrif-components-core/src/lib/resources/interface/group.ts` around
lines 1 - 2, Imports in
projects/netgrif-components-core/src/lib/resources/interface/group.ts use double
quotes instead of the project's single-quote convention; update the import
statements that reference ProcessRole and Authority to use single quotes (i.e.,
change "process-role" and "authority" imports to use single quotes) so they
match the Angular project style and lint rules.
| @@ -1,5 +1,6 @@ | |||
| import {ProcessRole} from '../../resources/interface/process-role'; | |||
| import {IUser} from './iuser'; | |||
| import {Group} from "../../resources/interface/group"; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent quote style in import.
This import uses double quotes while other imports in the file use single quotes. Consider using single quotes for consistency.
🔧 Proposed fix
-import {Group} from "../../resources/interface/group";
+import {Group} from '../../resources/interface/group';📝 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
| import {Group} from "../../resources/interface/group"; | |
| import {Group} from '../../resources/interface/group'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/netgrif-components-core/src/lib/user/models/user.ts` at line 3, The
import statement for Group uses double quotes while the rest of the file uses
single quotes; update the import of Group (the "Group" import declaration) to
use single quotes to match the project's quote style and keep import formatting
consistent.
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
1 participant
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.
Description
Implements NAE-2286
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
groupIdsinstead ofnextGroupsfor improved clarity.