-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AAE-6389] Remove duplicated identity user service mock #7373
Conversation
lib/core/mock/identity-group.mock.ts
Outdated
mockIdentityGroup1, mockIdentityGroup2, mockIdentityGroup3, mockIdentityGroup4, mockIdentityGroup5 | ||
]; | ||
|
||
export const mockApplicationDetails = {id: 'mock-app-id', name: 'mock-app-name'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it shouldn't exist here, it is not related to identity or groups but related to apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now it's ok to have this mock here we have a unit test depending on it. we need this mock because IdentityGroups has a method that gets clientId based on the appName and it is used in the GroupCloud component to fetch application-level groups. If we consider the place of the mock it would be nice if we start refactoring the service then move the mock accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
Edit: i put value of mockApplicationDetails
directly to applicationDetailsMockApi
change
{ id: 'role-id-1', name: 'role-name-1' }, { id: 'role-id-2', name: 'role-name-2' } | ||
]; | ||
getTotalGroupsCount(): Observable<IdentityGroupCountModel> { | ||
return of({ count: mockIdentityGroups.length }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need mockIdentityGroupsCount
if you are not using it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need it, it is used in unit testing
spyOn(service, 'getTotalGroupsCount').and.returnValue(of(mockIdentityGroupsCount)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to use mockIdentityGroupsCount
, because it also used here.
checkGroupHasRole(groupId: string, roleNames: string[]): Observable<boolean> { | ||
return this.getGroupRoles(groupId).pipe(map((groupRoles) => { | ||
let hasRole = false; | ||
if (groupRoles && groupRoles.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the typescript ?. which does the same but without adding many && conditions
if (groupRoles?.length) {
It is equivalent to if (groupRoles && groupRoles.length).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed, i also changed it in 'real' identity-group service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The > 0 is not needed because if(0) is not gonna go through the if. But that's fine no need to change it
lib/core/mock/identity-user.mock.ts
Outdated
|
||
export const mockJoinGroupRequest: IdentityJoinGroupRequestModel = {userId: 'mock-hser-id', groupId: 'mock-group-id', realm: 'mock-realm-name'}; | ||
|
||
export const mockGroup1 = <IdentityGroupModel> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have another file called identity-group.mock.ts
You probably need to move the group related stuff from this file to that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mocks moved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added one comment
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
What is the new behaviour?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: