Repository navigation
Conversation
There was a problem hiding this comment.
Thank you @hammadxcm for the PR.
I have created #48279 for this PR. Please reference it in the PR.
Overall LGTM, but please revert the comments removal.
@lhanusov I'd like to hear your thoughts about replacing OrganizationAttributeUpdater with private static Runnable setOrganizationEnabled(OrganizationResource org, boolean enabled), thank you.
|
|
||
| /** | ||
| * Integration tests for Organization Invitation Management functionality |
There was a problem hiding this comment.
please, revert this change, I don't see any benefit of it
| private OrganizationResource organization; | ||
| private String organizationId; | ||
|
|
||
| @Before | ||
| @BeforeEach |
There was a problem hiding this comment.
please, remove it and replace it with RealmConfig implementation
| @@ -86,14 +78,14 @@ public void testCreateAndListInvitations() { | |||
|
|
|||
| // List invitations | |||
| List<OrganizationInvitationRepresentation> invitations = organization.invitations().list(); | |||
|
|
|||
There was a problem hiding this comment.
please, revert this change, I don't see any benefit of it
| assertThat(invitations, hasSize(2)); | ||
|
|
| public void testSendInvitationToDisabledOrganization() throws Exception { | ||
| try (OrganizationAttributeUpdater oau = new OrganizationAttributeUpdater(organization).setEnabled(false).update()) { | ||
| public void testSendInvitationToDisabledOrganization() { | ||
| Runnable restore = setOrganizationEnabled(organization, false); |
There was a problem hiding this comment.
please use managedRealm and updateWithCleanup instead
There was a problem hiding this comment.
Hi @hammadxcm , thanks for contribution. I just want to point out the issue was created by @vramik and he planned to work on it. Your contribution is fine, but it lacks some context and those indentation changes doesn't make sense. Next time, please, start with issues marked with help-wanted, or good-first-issue.
I see, you are probably using some LLM tool or agent, for finding the easy looking issue and trying to deliver a PR that's fine, but if the contributor doesn't know the context, it adds additional work on reviewer, which is time consuming and at the end waste of time.
Please, check for comments and apply those changes
| public void testInvitationWorksAfterReEnablingOrganization() throws Exception { | ||
| try (OrganizationAttributeUpdater oau = new OrganizationAttributeUpdater(organization).setEnabled(false).update()) { | ||
| public void testInvitationWorksAfterReEnablingOrganization() { | ||
| Runnable restore = setOrganizationEnabled(organization, false); |
There was a problem hiding this comment.
please use managedRealm and updateWithCleanup instead
| sendInvitation("user@test-org.com", "John", "Doe"); | ||
|
|
||
| List<OrganizationInvitationRepresentation> invitations = organization.invitations().list(); | ||
| assertThat(invitations, hasSize(1)); | ||
| String invitationId = invitations.get(0).getId(); | ||
|
|
||
| try (OrganizationAttributeUpdater oau = new OrganizationAttributeUpdater(organization).setEnabled(false).update()) { | ||
| Runnable restore = setOrganizationEnabled(organization, false); |
There was a problem hiding this comment.
please use managedRealm and updateWithCleanup instead
| private void sendInvitationToOrganization(OrganizationResource org, String email, String firstName, String lastName) { | ||
| try (Response response = org.members().inviteUser(email, firstName, lastName)) { | ||
| assertThat(response.getStatus(), equalTo(204)); | ||
| } | ||
| } | ||
|
|
||
| private static Runnable setOrganizationEnabled(OrganizationResource org, boolean enabled) { |
There was a problem hiding this comment.
this runnable task doesn't add any value, please use: managedRealm and updateWithCleanup instead,
or call managedRealm directly
|
@vramik adding auto-expire |
|
@hammadxcm there is also missing sign-off by you as a contributor, please add it as well |
7c91b33 to
e928105
Compare
|
@lhanusov added the sign-off — amended the restore-comments commit and force-pushed. Thanks! |
|
Thanks for the thorough review @lhanusov — point taken on context, I'll pick from I've addressed the inline comments in 378abf5:
Let me know if the managedRealm-based approach you had in mind was different — |
Signed-off-by: hammadxcm <hammadkhanxcm@gmail.com>
Closes keycloak#48279 Signed-off-by: hammadxcm <hammadkhanxcm@gmail.com>
- Remove @BeforeEach/setUp and per-class fields; each test now creates its own OrganizationResource inline, matching the idiom in OrganizationGroupsTest and other new-framework organization tests. - Drop the setOrganizationEnabled helper; inline the enable/disable calls directly in the three disabled-organization tests. The per-test organization is auto-cleaned by AbstractOrganizationTest, so an explicit restore Runnable is no longer needed. - Revert the cosmetic Javadoc and chained-method indentation changes from the original arquillian file so the migration diff is limited to framework-only changes. Closes keycloak#48279 Signed-off-by: hammadxcm <hammadkhanxcm@gmail.com>
378abf5 to
ccc2cc0
Compare
Preserves the blank-line whitespace from the original arquillian file per reviewer feedback, keeping the migration diff limited to framework changes. Closes keycloak#48279 Signed-off-by: hammadxcm <hammadkhanxcm@gmail.com>
|
@lhanusov final pass in 743f658 — restored trailing whitespace on blank lines inside method bodies and the javadoc |
|
@hammadxcm thank you for your updates and effort, but still there are lot of places which are not correct, besides emails are sent to non-existent users. I suggest to close this PR and let @vramik to do it, since he is the author of the issue and he plans to work on it in a near future. JFYI reviewing and commenting this simple PR already took me more then 30 minutes, which doesn't make sense. If you want to try LLM and contribute to Keycloak, please, focus on issues with label help-wanted or good-first-issue. Thank you for understanding. |
Closes #48279
Parent tracking issue: #47926
Part of #48178 (reopened after #48203 — closed under the 2-PR-for-new-contributors rule while four PRs were briefly open. I've since closed #48207 too, so this + #48204 are the only two open).
Migrates
OrganizationInvitationManagementTestfromtestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/to the new test framework undertests/base/src/test/java/org/keycloak/tests/organization/admin/.All 15 test methods are pure organization admin-API checks (invitation CRUD, pagination, filtering, search, cross-org isolation, disabled-org behaviour) and required no logic changes — only the framework wiring differs:
AbstractOrganizationTest(JUnit 5,@InjectRealm).@KeycloakIntegrationTestadded at class level.@InjectMailServer MailServer— theGreenMailSupplieralready intercepts the realm config, so the manual SMTP override fromconfigureTestRealmis no longer needed.setTimeOffset(int)→@InjectTimeOffSet TimeOffSet.OrganizationAttributeUpdater(not ported to the new framework) is replaced with a localsetOrganizationEnabledhelper that returns a restoreRunnable— same disable/re-enable semantics. (Open question from review: whether to keep this local helper or portOrganizationAttributeUpdater— pending @lhanusov input.)The remaining admin tests in #48178 (
OrganizationTest,OrganizationInvitationLinkTest,OrganizationThemeTest,IdentityProviderThemeConfigTest) still depend on broker/UI infrastructure (legacybc/BrokerConfiguration,@Page/ Graphene) that has no direct equivalent in the new framework yet, so they are not part of this PR.