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

fix: allow a maximum of one invite at a time #8019

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Dec 19, 2023

Description

as described in #7962 multi invites could get problematic, specially the error handling.
Therefore we've decided to restrict invitations to a maximum of one!

It contains validation (allow only user and group types, id), error handling and tests for that.

  • already shared to the user // group -> 409
  • wrong or empty libre.graph.recipient.type (group && user allowed) -> 400
  • unknown user -> 400
  • unknown group -> 400
  • disabled user -> TODO not part of this pr, will be handled globally via reva

Related Issue

Motivation and Context

explicit error handling

How Has This Been Tested?

  • unit tests
  • manual tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Copy link

update-docs bot commented Dec 19, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@mmattel
Copy link
Contributor

mmattel commented Dec 20, 2023

Wouldn't your list of 4xx cases be valuable in some dev docs description?

@kulmann
Copy link
Contributor

kulmann commented Dec 20, 2023

Wouldn't your list of 4xx cases be valuable in some dev docs description?

The PR is still in draft mode, isn't it?!

@individual-it
Copy link
Member

I think this also needs a change in the libregraph repo, the example there shows multi recipient invite: https://github.com/owncloud/libre-graph-api/blob/main/api/openapi-spec/v1.0.yaml#L514-L519

@fschade
Copy link
Contributor Author

fschade commented Dec 22, 2023

recipient

I think this also needs a change in the libregraph repo, the example there shows multi recipient invite: https://github.com/owncloud/libre-graph-api/blob/main/api/openapi-spec/v1.0.yaml#L514-L519

thanks for the hint, done: owncloud/libre-graph-api#155

@fschade
Copy link
Contributor Author

fschade commented Dec 22, 2023

Wouldn't your list of 4xx cases be valuable in some dev docs description?

thanks for the hint too, one of the benefits of oapi is the self documenting character, e.g. https://swagger.io/resources/articles/documenting-apis-with-swagger/

or do is miss some point?

@fschade fschade force-pushed the graph-beta-fix-invite branch from f2f3b72 to 4c42b1e Compare December 22, 2023 13:34
@fschade fschade self-assigned this Dec 22, 2023
@fschade fschade marked this pull request as ready for review December 22, 2023 13:35
Copy link

@rhafer rhafer merged commit 465c9e3 into owncloud:master Jan 9, 2024
3 checks passed
@ScharfViktor
Copy link
Contributor

I tested it, looks good

  • resource from personal/project space already shared to the user // group -> 409
  • space already shared to the user // group -> 409
  • share disabled/deleted space -> 404
  • share deleted resource -> 404
  • wrong or empty libre.graph.recipient.type - 404
  • unknown user/group -400

@grgprarup could you please adapt your tests for new code expectation

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.

SharingNg. sharing invitation
7 participants