-
Notifications
You must be signed in to change notification settings - Fork 66
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
Resolve role sharing conflicts in app sharing #413
Resolve role sharing conflicts in app sharing #413
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
============================================
+ Coverage 29.01% 31.03% +2.01%
- Complexity 385 440 +55
============================================
Files 50 61 +11
Lines 4146 4386 +240
Branches 478 496 +18
============================================
+ Hits 1203 1361 +158
- Misses 2842 2913 +71
- Partials 101 112 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...main/java/org/wso2/carbon/identity/organization/management/handler/SharedRoleMgtHandler.java
Outdated
Show resolved
Hide resolved
...main/java/org/wso2/carbon/identity/organization/management/handler/SharedRoleMgtHandler.java
Outdated
Show resolved
Hide resolved
...main/java/org/wso2/carbon/identity/organization/management/handler/SharedRoleMgtHandler.java
Outdated
Show resolved
Hide resolved
Add unit tests |
c5cb4aa
to
e6b551c
Compare
Unit tests added to the new improvement |
...main/java/org/wso2/carbon/identity/organization/management/handler/SharedRoleMgtHandler.java
Outdated
Show resolved
Hide resolved
...main/java/org/wso2/carbon/identity/organization/management/handler/SharedRoleMgtHandler.java
Outdated
Show resolved
Hide resolved
auditData.put("parentOrganizationId", parentOrganizationId); | ||
auditData.put("parentApplicationId", parentApplicationId); | ||
auditData.put("sharedOrganizationId", sharedOrganizationId); | ||
auditData.put("roleId", roleId); | ||
auditData.put("roleName", roleName); | ||
auditData.put("failureReason", failureReason); |
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.
use the constant defined in role mgt side
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.
Once the PR [1] is merged we can use them here
...org/wso2/carbon/identity/organization/management/handler/tests/SharedRoleMgtHandlerTest.java
Show resolved
Hide resolved
...org/wso2/carbon/identity/organization/management/handler/tests/SharedRoleMgtHandlerTest.java
Show resolved
Hide resolved
thenReturn(PARENT_ORG_USER_ID); | ||
|
||
SharedRoleMgtHandler sharedRoleMgtHandler = new SharedRoleMgtHandler(); | ||
sharedRoleMgtHandler.handleEvent(event); |
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 are not asserting anything 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.
Here the assertion is the exception
expectedExceptions = IdentityEventException.class
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.
Yes this is correct, wanted to add this in last test method
...org/wso2/carbon/identity/organization/management/handler/tests/SharedRoleMgtHandlerTest.java
Outdated
Show resolved
Hide resolved
...org/wso2/carbon/identity/organization/management/handler/tests/SharedRoleMgtHandlerTest.java
Show resolved
Hide resolved
2773ef0
to
05a5b93
Compare
PR builder started |
PR builder completed |
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.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/11973410045
05a5b93
to
c470073
Compare
PR builder started |
PR builder completed |
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.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/12116587383
Purpose
role01
and an organization audience application will be shared from the root organization and root organization contains a role namedrole01
role01
and already an organization audienace app is shared. Now in the root organization a new organization role will be created with a namerole01
.role01
and already an organization audienace app is shared. Now in the root organization an existing role is renamed torole01
.[1] wso2/product-is#21208
Goals
Approach