Skip to content

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:

@update-docs
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 self-assigned this Dec 22, 2023
@fschade fschade marked this pull request as ready for review December 22, 2023 13:35
@sonarqubecloud
Copy link

@rhafer rhafer merged commit 465c9e3 into owncloud:master Jan 9, 2024
@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