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

API: deprecate toggle of superuser status in favor of new idempotent endpoint, update setup-all.sh #10440

Merged
merged 22 commits into from
Apr 18, 2024

Conversation

zearaujo25
Copy link
Contributor

@zearaujo25 zearaujo25 commented Mar 27, 2024

What this PR does / why we need it:
Adds a new endpoint to make the superuser status change idempotent.
Which issue(s) this PR closes:

Special notes for your reviewer:
none

Suggestions on how to test this:
Integration test

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
no
Is there a release notes update needed for this change?:
New endpoint addition
Additional documentation:

Preview at https://dataverse-guide--10440.org.readthedocs.build/en/10440/api/native-api.html#set-superuser-status

@zearaujo25 zearaujo25 marked this pull request as ready for review March 27, 2024 21:42
Copy link
Member

@pdurbin pdurbin 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! Please see my comments.

src/main/java/edu/harvard/iq/dataverse/api/Admin.java Outdated Show resolved Hide resolved
src/main/java/edu/harvard/iq/dataverse/api/Admin.java Outdated Show resolved Hide resolved
@pdurbin
Copy link
Member

pdurbin commented Mar 27, 2024

I wasn't able to run the integrations tests locally as i keep receiving the java connection refused error

Huh. Please hit me up in https://chat.dataverse.org and I can try to help. I'll point you toward running Dataverse in Docker: https://preview.guides.gdcc.io/en/develop/developers/dev-environment.html#quickstart

Anyway, I know you can't see the results of Jenkins testing this pull request...

... but here's a screenshot from https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10440/1/testReport/edu.harvard.iq.dataverse.api/AdminIT/ that shows your tests (all tests, really) are passing:

Screenshot 2024-03-27 at 7 19 16 PM

@pdurbin pdurbin added Type: Feature a feature request Feature: API User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh Size: 0.5 A percentage of a sprint. 0.35 hours labels Mar 27, 2024
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

A comment on deprecation.

src/main/java/edu/harvard/iq/dataverse/api/Admin.java Outdated Show resolved Hide resolved
@pdurbin pdurbin changed the title 9887: Adding new endpoint to make superuser call idempotent API: deprecate togged of superuser status in favor of new idempotent endpoint Mar 29, 2024
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

This pull request is really coming together! Thanks, @zearaujo25 !

I left some more feedback for you.

doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java Outdated Show resolved Hide resolved
doc/release-notes/9887-new-superuser-status-endpoint.md Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/native-api.rst Show resolved Hide resolved
@pdurbin pdurbin changed the title API: deprecate togged of superuser status in favor of new idempotent endpoint API: deprecate toggle of superuser status in favor of new idempotent endpoint Mar 29, 2024
zearaujo25 and others added 5 commits March 29, 2024 12:51
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

A little more feedback.

doc/sphinx-guides/source/api/changelog.rst Outdated Show resolved Hide resolved
src/test/java/edu/harvard/iq/dataverse/api/AdminIT.java Outdated Show resolved Hide resolved
@pdurbin pdurbin self-assigned this Apr 11, 2024
@coveralls
Copy link

coveralls commented Apr 11, 2024

Coverage Status

coverage: 20.705% (-0.002%) from 20.707%
when pulling 80ae69b on zearaujo25:feat/9887
into a0d7cd2 on IQSS:develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Quick question about curl.

Also, I merged develop into this branch.

doc/sphinx-guides/source/api/native-api.rst Outdated Show resolved Hide resolved
@pdurbin
Copy link
Member

pdurbin commented Apr 12, 2024

@zearaujo25 and I chatted and we decided on the following:

  • For consistency with the settings API, we'll switch to -X PUT -d true.
  • We know the SPA will someday need the ability to promote and demote superusers (probably under /api/users) but we'll defer this for now.
  • Since the toggle API is deprecated as of this PR, we will switch scripts/api/setup-all.sh to use it.
  • Phil might switch an API test or two to the new PUT version.
  • At some point, we should open an issue to migrate away from the POST version (tech debt).

@zearaujo25
Copy link
Contributor Author

zearaujo25 commented Apr 12, 2024

We know the SPA will someday need the ability to promote and demote superusers (probably under /api/users) but we'll defer this for now.

I also would like to add the supersuer feature for the SPA probably will need a restructure of endpoints logic. Since its too early to predict such changes, we opt to defer this feature when its clearer the needs the front end team will need

@pdurbin
Copy link
Member

pdurbin commented Apr 12, 2024

defer this feature when its clearer the needs the front end team will need

Yes, exactly. I did ping @ekraffmiller @GPortas @MellyGray and @GermanSaracca about this in Slack: https://iqss.slack.com/archives/C04MJ76A34Z/p1712942297281699

Screenshot 2024-04-12 at 3 43 39 PM

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@zearaujo25 this is looking really good. Approved! Thank you!

To whoever does QA, I did a fair amount of testing and poking around. Please at least check that tests are passing and that the docs look ok.

@pdurbin pdurbin changed the title API: deprecate toggle of superuser status in favor of new idempotent endpoint API: deprecate toggle of superuser status in favor of new idempotent endpoint, update setup-all.sh Apr 12, 2024
@sekmiller sekmiller self-assigned this Apr 17, 2024
@sekmiller sekmiller merged commit 447d576 into IQSS:develop Apr 18, 2024
12 checks passed
@pdurbin pdurbin added this to the 6.3 milestone Apr 18, 2024
@zearaujo25 zearaujo25 deleted the feat/9887 branch April 19, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: API Size: 0.5 A percentage of a sprint. 0.35 hours Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

idempotent API call for making a user a superuser
4 participants