OCS Share API to appframework by rullzer · Pull Request #445 · nextcloud/server

rullzer · 2016-07-19T14:09:49Z

First OCS API to be ported.

TODO:

mention-bot · 2016-07-19T14:09:51Z

@rullzer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @schiessle, @nickvergessen and @icewind1991 to be potential reviewers

rullzer · 2016-07-19T14:19:55Z

The thing here is that we can't use all (we can use some) of the standard AppFramework responses. Since here we want to add a message often as well (to not change the output of the API).

Should we add OCSNotFoundResponse etc?
Other idea's?

@BernhardPosselt @LukasReschke @icewind1991 @nickvergessen @MorrisJobke

BernhardPosselt · 2016-07-19T14:23:23Z

Depends really. You could use a trait like https://github.com/owncloud/app-tutorial/blob/master/controller/errors.php#L13 or implement an afterException hook in a middleware that responds to an OCSNotFoundException, you could add a shortcut function, custom response type etc etc :D

rullzer · 2016-07-19T14:30:04Z

Fair enough.
But I'm more looking for something that we could use in all cases where the OCS API is in use.

Maybe an OCSMiddleware that lives in core would not be so bad. Then it just catches OCS***Exceptions.

BernhardPosselt · 2016-07-19T14:43:44Z

There's definitely a benefit in adding a type for these well defined cases since it makes it harder to make mistakes. Types and Exceptions are sometimes hard to distinguish and other languages actually only have types (e.g. Result<String, Error> where String is the success case and Error the error case)

Ultimately I like the exception approach a little bit more than the custom response because I think its more idiomatic. However both approaches can and should work fine :)

rullzer · 2016-07-19T16:16:12Z

After thinking about it I think I prefer the exceptions as well.

I'll create a PR soonish and let you know.

rullzer · 2016-07-20T14:59:48Z

Rebase on #475 once in...

rullzer · 2016-07-20T20:11:49Z

Needs: #480

rullzer · 2016-07-22T17:37:16Z

Ok finally time to review. A lot is rewriting the tests. But I think this cleaned up the code alread pretty good.

CC: @schiessle @nickvergessen @MorrisJobke @LukasReschke

MorrisJobke · 2016-07-25T07:37:52Z

- $this->assertFalse($result->succeeded());
+ try {
+ $ocs->createShare();
+ $this->fail();


I like that one 👍

MorrisJobke · 2016-07-25T07:41:23Z

I get following when opening the share sidebar and it logs me out, because it thinks the session has timed out:

rullzer · 2016-07-25T07:44:42Z

@MorrisJobke ah mmm I get that to. Let me check why.

MorrisJobke · 2016-07-30T07:54:31Z

Rebased on master.

@LukasReschke @rullzer Is the above mentioned issue fixed with this then?

rullzer · 2016-07-30T11:24:00Z

@MorrisJobke it is now.
@LukasReschke any other remarks?

rullzer · 2016-08-01T06:29:21Z

Ah joy this breaks the intergration tests... because we did not set the header there.

rullzer · 2016-08-01T14:23:22Z

All fixed.

@LukasReschke @schiessle @nickvergessen final review?

MorrisJobke · 2016-08-02T15:03:13Z

This branch has conflicts that must be resolved

@rullzer

rullzer · 2016-08-03T09:41:46Z

Conflicts resolved.
Review time!!!!

LukasReschke · 2016-08-04T17:43:20Z

 * The getShares function.
 *
+ * @NoAdminRequired
+d *


* This will cleanup the locks after each request * Move check for enabled share api to the middleware

MorrisJobke · 2016-08-05T14:33:05Z

Still works 👍

LukasReschke · 2016-08-08T12:59:55Z

👍

…work OCS Share API to appframework

rullzer added the 2. developing Work in progress label Jul 19, 2016

rullzer force-pushed the ocs_share_to_appframework branch from 3e9d360 to 78f4a8eCompare July 20, 2016 13:27

rullzer force-pushed the ocs_share_to_appframework branch 2 times, most recently from a4f8e90 to 21a140bCompare July 20, 2016 19:49

rullzer force-pushed the ocs_share_to_appframework branch 7 times, most recently from 6ef5ec8 to e067f73Compare July 22, 2016 14:10

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 22, 2016

rullzer added this to the Nextcloud 11.0 milestone Jul 22, 2016

MorrisJobke reviewed Jul 25, 2016
View reviewed changes

rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 25, 2016

rullzer force-pushed the ocs_share_to_appframework branch from e067f73 to c2c58d2Compare July 25, 2016 19:19

rullzer force-pushed the ocs_share_to_appframework branch 2 times, most recently from 7c328f0 to 55a4d53Compare July 30, 2016 11:23

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 30, 2016

rullzer force-pushed the ocs_share_to_appframework branch 2 times, most recently from 2b216d4 to 6b64109Compare August 1, 2016 12:38

rullzer mentioned this pull request Aug 2, 2016
Allow ocs/v2.php/cloud/... routes #691
Merged

rullzer force-pushed the ocs_share_to_appframework branch from 6b64109 to b62510dCompare August 3, 2016 08:17

LukasReschke reviewed Aug 4, 2016
View reviewed changes

rullzer added 9 commits August 5, 2016 14:17

Convert Share API to use the AppFramework
a32caa4

Remove unneeded wrapper
54f21bc

Throw OCSNotFoundExceptions
8b16007

Add OCSShareAPIMiddleware
3a31b28
* This will cleanup the locks after each request * Move check for enabled share api to the middleware

More exceptions
7fdc2f3

DataResponse required ['data' => DATA]
4504d7f

Add OCSShareAPIMiddleware tests
d9418c1

Fix tests
0556e40

Add OCS-APIREQUEST header to intergration test calls
dd9f195

rullzer force-pushed the ocs_share_to_appframework branch from b62510d to dd9f195Compare August 5, 2016 12:17

LukasReschke merged commit 70eef2a into masterAug 8, 2016

LukasReschke deleted the ocs_share_to_appframework branch August 8, 2016 13:00

GitHubUser4234 pushed a commit to GitHubUser4234/server that referenced this pull request Aug 30, 2016
Merge pull requestnextcloud#445from nextcloud/ocs_share_to_appframe…
cdb2c87
…work OCS Share API to appframework

Uh oh!

Uh oh!

Conversation

rullzer commented Jul 19, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jul 19, 2016

Uh oh!

rullzer commented Jul 19, 2016

Uh oh!

BernhardPosselt commented Jul 19, 2016

Uh oh!

rullzer commented Jul 19, 2016

Uh oh!

BernhardPosselt commented Jul 19, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer commented Jul 19, 2016

Uh oh!

rullzer commented Jul 20, 2016

Uh oh!

rullzer commented Jul 20, 2016

Uh oh!

rullzer commented Jul 22, 2016

Uh oh!

MorrisJobkeJul 25, 2016

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Jul 25, 2016

Uh oh!

rullzer commented Jul 25, 2016

Uh oh!

MorrisJobke commented Jul 30, 2016

Uh oh!

rullzer commented Jul 30, 2016

Uh oh!

rullzer commented Aug 1, 2016

Uh oh!

rullzer commented Aug 1, 2016

Uh oh!

MorrisJobke commented Aug 2, 2016

Uh oh!

rullzer commented Aug 3, 2016• edited by LukasReschke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukasReschkeAug 4, 2016

Choose a reason for hiding this comment

Uh oh!

rullzerAug 5, 2016

Choose a reason for hiding this comment

Uh oh!

MorrisJobke commented Aug 5, 2016

Uh oh!

LukasReschke commented Aug 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rullzer commented Jul 19, 2016•
edited
Loading

BernhardPosselt commented Jul 19, 2016•
edited
Loading

rullzer commented Aug 3, 2016•
edited by LukasReschke
Loading