-
Notifications
You must be signed in to change notification settings - Fork 490
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
Allow merging of accounts that are members of the same group #9909
Allow merging of accounts that are members of the same group #9909
Conversation
Response createGroup = UtilIT.createGroup(dataverseAlias, aliasInOwner, displayName, superuserApiToken); | ||
createGroup.prettyPrint(); | ||
createGroup.then().assertThat() | ||
.statusCode(CREATED.getStatusCode()); | ||
|
||
String groupIdentifier = JsonPath.from(createGroup.asString()).getString("data.identifier"); | ||
// String groupIdentifier = JsonPath.from(createGroup.asString()).getString("data.identifier"); |
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 suggest to remove code that is no longer needed, instead of leaving it commented out. The codebase has lots of commented code already, which is very rarely removed.
Added an additional line to restart Payara after changing settings in Permalinks section
This reverts commit f71274e.
For unknown reasons, in 2009 several files from the JDK were copied into the Dataverse codebase, instead of referenced. It appears that these classes weren't really used.
|
||
List<String> roleAssigneesToAdd = new ArrayList<>(); | ||
roleAssigneesToAdd.add(user2identifier); | ||
List<String> roleAssigneesToAdd = Arrays.asList(user2identifier, target2identifier); |
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.
@stevenferey does it make sense to leave in the original test (that is, create another group and only add the "consumed" user) - just so we don't lose the original test?
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 original test have been remove because if the test is valid for merged accounts in the same group, I think it's difficult to not to be valid with accounts not in the same group. However, I don't think the reverse is true.
But I can keep the original test and add a specific test for accounts in the same group.
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.
No, it's OK - thanks. Just wanted to make sure it was considered.
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: