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 OCS public link share update values logic #930

Merged
merged 6 commits into from
Jul 3, 2020
Merged

Fix OCS public link share update values logic #930

merged 6 commits into from
Jul 3, 2020

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Jul 2, 2020

Only update a value if it was actually set in the form.

Added support for clearing expiration date and password by
distinguishing between an empty string and whether the value is actually
set in the request.

Refactored public permission parsing to use permissionFromRequest for
both creation and update of public shares.

For owncloud/ocis-reva#252

Makes almost all tests from tests/acceptance/features/apiSharePublicLink2/multilinkSharing.feature pass now.

  • run more API tests to confirm that there are no regressions => CI will help with that!

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

and yes, OCS is not REST... one does not pass the whole object to a PUT request...

Only update a value if it was actually set in the form.

Added support for clearing expiration date and password by
distinguishing between an empty string and whether the value is actually
set in the request.

Refactored public permission parsing to use permissionFromRequest for
both creation and update of public shares.
@PVince81 PVince81 marked this pull request as ready for review July 2, 2020 12:37
@PVince81 PVince81 requested a review from labkode as a code owner July 2, 2020 12:37
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

/drone/src/tmp/testrunner/tests/acceptance/features/apiSharePublicLink1/createPublicLinkShare.feature:566 and others

I'll have a look

Vincent Petry added 2 commits July 2, 2020 15:31
Make it 15 chars instead of 12 to comply with the old implementation.
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

seems the refactoring of permission logic broke the permission conversion from OC to CS3.
but even after verifying almost all locations I can't see where it breaks.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

problem solved. it was a nasty colon again...

I've also added two extra fixes for expiration date and token length as they were failing in tests I'm trying to enable...

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

hmm some other failures in the same file: /drone/src/tmp/testrunner/tests/acceptance/features/apiSharePublicLink1/createPublicLinkShare.feature:603

Adjust the public link permission mappings to make sure that we get the
same "permission" values out that were put in through the API.
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

right, the old code ignored the error and I made it more strict...

I've adjusted the roles just a bit to make the tests pass. I added some comments as there will be more to adjust/align later in the roles and perms (see https://github.com/owncloud/ocis-reva/issues/292)

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

All green now!

More tests will be enabled later in owncloud/core#37633

Only reset the password when it was explicitly set to an empty string.

This fix copies the old data structure and keeps the password in place
if no explicit update was requested.
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 2, 2020

I've added yet another fix: setting any attribute for a public link share would clear its password. This was making tests/acceptance/features/apiSharePublicLink2/updatePublicLinkShare.feature:71 in owncloud/core#37633 fail

@labkode
Copy link
Member

labkode commented Jul 3, 2020

@PVince81 once you're happy with @refs comments I'll merge.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 3, 2020

@labkode solved now, thanks

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.

3 participants