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

6915-feature-builtin-users-sendEmailNotification-new-optionalParameter #7033

Conversation

RightInTwo
Copy link
Contributor

@RightInTwo RightInTwo commented Jun 29, 2020

What this PR does / why we need it:
Allow to disable sending an email for confirmation after creating a user.

Which issue(s) this PR closes:
Closes none, but solves part of #6915

Special notes for your reviewer:
We don't have a dev server, so there was no local testing yet.

Suggestions on how to test this:
Create and edit users with the builtin-users API with and without explicitly submitting PathParam with
@post
@path("{password}/{key}/{sendEmailNotification}")

and as QueryParam with

@post
...and &sendEmailNotification=...

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?:
could be a new small feature to mention

Additional documentation:
see doc/sphinx-guides/source/api/native-api.rst

…arameter to create(...) and internalSave(...)
@coveralls
Copy link

coveralls commented Jun 29, 2020

Coverage Status

Coverage increased (+0.09%) to 19.652% when pulling b5204a7 on WZBSocialScienceCenter:6915-feature-builtin-users-sendEmailNotification-new-optionalParameter into b97185a 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.

Please add some docs.

@djbrooke
Copy link
Contributor

@RightInTwo, moving this back to Community Dev. I saw your note about it not being ready yet, but it looks like you already have one review suggestion! :)

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.

I think there's some confusion over path parameter vs query parameter but I'm not sure. @RightInTwo can you please look at my comment?

*/
@POST
@Path("{password}/{key}/{sendEmailNotification}")
public Response create(BuiltinUser user, @PathParam("password") String password, @PathParam("key") String key, @PathParam("sendEmailNotification") Boolean sendEmailNotification) {
Copy link
Member

Choose a reason for hiding this comment

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

sendEmailNotification appears to be a path parameter rather than a query parameter, as documented above. @RightInTwo can you provide a full curl example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @pdurbin, we added both ways (query and path), just as the create() function is implemented for both currently. The docs at http://guides.dataverse.org/en/latest/api/native-api.html#assign-default-role-to-user-creating-a-dataset-in-a-dataverse seem to omit the path variant completely, which is why we left it out as well.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I get it. The query parameter version is called "save". Thanks.

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.

There's a slight mismatch of the docs vs the code but I'm moving this to QA. The docs say to use a query parameter. In truth, both the query parameter and the path parameter should work but I haven't tested the code myself.

I'm not sure how popular this feature will be but it seems to solve a problem for @RightInTwo.

*/
@POST
@Path("{password}/{key}/{sendEmailNotification}")
public Response create(BuiltinUser user, @PathParam("password") String password, @PathParam("key") String key, @PathParam("sendEmailNotification") Boolean sendEmailNotification) {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I get it. The query parameter version is called "save". Thanks.

@RightInTwo
Copy link
Contributor Author

  • Improve docs (path vs. query params)
  • Improve code comments

@kcondon kcondon self-assigned this Jul 13, 2020
@kcondon kcondon merged commit c1a3834 into IQSS:develop Jul 14, 2020
@djbrooke djbrooke added this to the Dataverse 5 milestone Jul 14, 2020
@RightInTwo
Copy link
Contributor Author

Wonderful! Thank you @pdurbin, @kcondon and @djbrooke!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants