Skip to content
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

Fixes updateAttributes endpoint #10958

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Fixes updateAttributes endpoint #10958

wants to merge 3 commits into from

Conversation

GPortas
Copy link
Contributor

@GPortas GPortas commented Oct 23, 2024

This comment has been minimized.

@GPortas GPortas marked this pull request as ready for review October 23, 2024 13:41
@GPortas GPortas added SPA.Q4 Not related to any specific Q4 feature Size: 10 A percentage of a sprint. 7 hours. GREI Re-arch Issues related to the GREI Dataverse rearchitecture Original size: 10 Type: Bug a defect labels Oct 23, 2024
@cmbz cmbz added the FY25 Sprint 8 FY25 Sprint 8 (2024-10-09 - 2024-10-23) label Oct 23, 2024
@GPortas GPortas added Size: 3 A percentage of a sprint. 2.1 hours. and removed Size: 10 A percentage of a sprint. 7 hours. labels Oct 23, 2024
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:fix-updateAttributes
ghcr.io/gdcc/configbaker:fix-updateAttributes

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@cmbz cmbz added the FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) label Oct 23, 2024
@GPortas GPortas added the SPA These changes are required for the Dataverse SPA label Oct 24, 2024
@sekmiller sekmiller self-assigned this Oct 24, 2024
}
if (!ctxt.settings().isTrueForKey(SettingsServiceBean.Key.AllowEnablingFilePIDsPerCollection, false)) {
throw new PermissionException("Changing File PID policy per collection is not enabled on this server",
this, Collections.singleton(Permission.EditDataset), dataverse);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point, but if we get here we have a super user so wouldn't it be an illegal command exception - not a permission exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought the same, but I chose to use the PermissionException since that was the original exception thrown before moving this logic into the command. It's not a significant backward compatibility issue, but that was the reason behind my decision. However, I can switch to the IllegalCommandException if you think it fits better.

changeAttributeResp = UtilIT.setCollectionAttribute("root", "name", newCollectionName, apiToken);
changeAttributeResp.then().assertThat()
.statusCode(UNAUTHORIZED.getStatusCode());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not asserting anything that is particular to Root here, correct? Any random user without Edit Dataverse permissions would get the same Unauthorized on any Dataverse

Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for responding to my questions.

@sekmiller sekmiller removed their assignment Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 8 FY25 Sprint 8 (2024-10-09 - 2024-10-23) FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) GREI Re-arch Issues related to the GREI Dataverse rearchitecture Original size: 10 Size: 3 A percentage of a sprint. 2.1 hours. SPA.Q4 Not related to any specific Q4 feature SPA These changes are required for the Dataverse SPA Type: Bug a defect
Projects
Status: Ready for QA ⏩
Development

Successfully merging this pull request may close these issues.

3 participants