Add OCS sharing info to capabilities #13692

rullzer · 2015-01-26T20:30:14Z

Patch for #13673

PVince81 · 2015-01-26T20:34:54Z

I wonder if this should be in core instead. Some of these settings apply to sharing in general and might affect calendar/contacts.

CC @schiesbn @icewind1991 @MorrisJobke for additional feedback

PVince81 · 2015-01-26T20:40:28Z

I wonder if we should also expose the other settings that enforce the date to be only within a given number of days. This way clients could also enforce it that way.
On the other hand this could be done through validation.

Disclosing too much isn't always a good thing, isn't it @LukasReschke ?

PVince81 · 2015-01-26T20:41:24Z

Forgot to say: thanks a lot for your PR 😄

DeepDiver1975 · 2015-01-26T20:47:09Z

as defined in the contribution guidelines - we require unit test - see b533aad#diff-6a3371457528722a734f3c51d9238c13

Therefore I'd welcome to have the capabilities class not static where an instance to the config object is injected via the ctor. Feel free to ping me in case of questions.

DeepDiver1975 · 2015-01-26T20:47:38Z

Forgot to say: thanks a lot for your PR 😄

indeed - thanks a lot! I tend to forget this as well! Much appreciated!

MorrisJobke · 2015-01-26T22:47:25Z

apps/files_sharing/lib/capabilities.php

+		$config = \OC::$server->getConfig();
+
+		$res = array();
+		if ($config->getAppValue('core', 'shareapi_allow_links', 'yes') === "yes") {


please use single quotes

PVince81 · 2015-01-27T09:01:52Z

@DeepDiver1975 I'm afraid it might not be possible to make this class non-static / make it use the app framework because it uses a special feature of the OCS routine that merges the responses together, gathered from multiple apps. Something to be considered as part of #12454

It might be possible to mock the config currently using static injection (static setConfig() method) or by replacing the service with a mock (see https://github.com/owncloud/core/blob/master/tests/lib/app.php#L493)

Or do you guys have a better idea ?

DeepDiver1975 · 2015-01-27T11:05:12Z

I'm afraid it might not be possible to make this class non-static

there is a way to make this work

class Capabilities {
    public static function getCapabilities() {
        $config = \OC::$server->getConfig();
        $cap = new Capabilities($config);
        return $cap->getCaps();
    }

    public function getCaps() {
        $res = array();
        if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') === "yes") {
            $res["allow_links"] = true;

            /* ..... */
        }
        return new \OC_OCS_Result(/* ... */);
    }
    }
}

rullzer · 2015-01-27T20:12:55Z

@DeepDiver1975 unit tests are on the way, however why should the class be non static for that?

DeepDiver1975 · 2015-01-27T22:38:06Z

however why should the class be non static for that?

to break the dependencies between objects - you want to test the class Capabilities and not the class Config. Therefore a test double/mock is to be injected.

This fixes #13673. It now lists link sharing, passwords enforced, and if public uploads are allowed.

rullzer · 2015-02-04T14:31:03Z

If I have time this evening I'll split up the unit test so each funtion tests once case.

@PVince81 any update on where this should be placed? I get your concern about those settings not being limit to files sharing. However where should they go then?

Extending the PR to include default expire stuff. Resharing etc. Is easy. Probably there is not really a thing as to much sharing since it is only available for users anyway.

DeepDiver1975 · 2015-02-05T09:19:00Z

apps/files_sharing/lib/capabilities.php

+	public function getCaps() {
+		$res = array();
+		if ($this->config->getAppValue('core', 'shareapi_allow_links', 'yes') === 'yes') {
+			$res['allow_links'] = true;


I was thinking a bit more about this and from my understanding it would be better to also add false in case a capability is not available. Makes it easier from an api usage pov.

Furthermore I'd like to come up with a naming scheme to be used for the capabilities.

More an OO style of naming like

~~sharing.allow-links .... still thinking about this~~

Sure that easy enough to do. I'll add that to my list.
I agree the naming is not ideal. So suggestions are welcome.

Moved to files_sharing Added more capabilities. Tried to order to capabilities more OO style

rullzer · 2015-02-06T10:00:20Z

I pushed a new commit (I did not update the unit tests just yet).

Now it basically has to following structure:

<files_sharing>
    <public>
        <password_enforced>1</password_enforced>
        <expire_date>
            <days>7</days>
            <enforce></enforce>
        </expire_date>
        <send_mail>1</send_mail>
    </public>
    <user>
        <send_mail>1</send_mail>
    </user>
    <resharing>1</resharing>
</files_sharing>

So an element it empty when set to false in json this looks better:

            "files_sharing":{
                "public":{
                    "password_enforced":true,
                    "expire_date":{
                        "days":"7",
                        "enforce":false
                    },
                    "send_mail":true
                },
                "user":{
                    "send_mail":true
                },
                "resharing":true
            }

Anyway I think this structure looks better and is easier to understand.
Any comments or suggestions? Else I'll start editing the unit tests.

scrutinizer-notifier · 2015-02-06T11:39:55Z

The inspection completed: 11 new issues, 20 updated code elements

ghost · 2015-02-06T12:08:28Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/8896/
Test PASSed.

karlitschek · 2015-02-06T21:44:48Z

👍

craigpg · 2015-02-06T21:46:27Z

👍 did pair testing with @karlitschek.

Add OCS sharing info to capabilities

LukasReschke · 2015-02-06T21:54:58Z

Please adjust at least the milestone when merging such critical bug fixes…

LukasReschke · 2015-02-06T21:58:43Z

Furthermore that code follows not the coding practises and is missing appropriate PHPDoc which was requested by me in another PR – this PR was just not getting reviewed because it was scheduled for 8.1…

LukasReschke · 2015-02-06T21:59:55Z

apps/files_sharing/lib/capabilities.php

+	public function getCaps() {
+		$res = array();
+
+		$public = false;;


rullzer · 2015-02-06T22:09:23Z

Mmm I was indeed not yet done with this PR. I'll try to fix them over the weekend.

LukasReschke · 2015-02-06T22:09:56Z

apps/files_sharing/tests/capabilities.php

+/**
+ * Class Test_Files_Sharing_Capabilties
+ */
+class Test_Files_Sharing_Capabilities extends \Test\TestCase {


Classname should be different. Those Test_ class names are legacy.

PVince81 · 2015-02-06T22:28:37Z

Please stop merging features so late before the release!

Maybe I should have kept that OCS capability idea to myself and waited for after the release to raise it to avoid such happenings ?

DeepDiver1975 · 2015-02-06T22:40:28Z

Please revert this - we did schedule this for 8.1! THX @karlitschek

LukasReschke · 2015-02-06T23:14:16Z

Reverted with c8e8456

@rullzer Sorry for the trouble. Can I ask you to reopen a new PR for this? Thanks a lot!

PVince81 added this to the 8.1-next milestone Jan 26, 2015

PVince81 mentioned this pull request Jan 26, 2015

Add OCS capability to advertise whether a server allows public links #13673

Closed

rullzer mentioned this pull request Jan 26, 2015

Add OCS capability to advertise server to server sharing support #12289

Closed

MorrisJobke reviewed Jan 26, 2015
View reviewed changes

rullzer mentioned this pull request Feb 4, 2015

When you enable the option of "Enforce password protection", you can't share by link from the desktop owncloud/client#2774

Closed

rullzer added 5 commits February 4, 2015 14:56

Added capabilities whether a server allows public links

b3ea849

This fixes #13673. It now lists link sharing, passwords enforced, and if public uploads are allowed.

Only return capabilities if they are set

ea1f726

Use single quotes

4801d9c

Converted getCapabilities to non static function

0452fde

Added unit test to test getCapabilities for files_sharing

083ebca

Split up unit tests

bcae79a

DeepDiver1975 reviewed Feb 5, 2015
View reviewed changes

rullzer added 2 commits February 6, 2015 10:33

Updated sharing capabilities

d4e361d

Moved to files_sharing Added more capabilities. Tried to order to capabilities more OO style

Bit better formatting when using json output

3be3e20

Updated unit tests

fe2aca5

karlitschek added a commit that referenced this pull request Feb 6, 2015

Merge pull request #13692 from rullzer/capabilities

0b421e8

Add OCS sharing info to capabilities

karlitschek merged commit 0b421e8 into owncloud:master Feb 6, 2015

LukasReschke reviewed Feb 6, 2015
View reviewed changes

apps/files_sharing/lib/capabilities.php

public function getCaps() {

$res = array();

$public = false;;

Copy link

Member

LukasReschke Feb 6, 2015

Choose a reason for hiding this comment

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

???

LukasReschke reviewed Feb 6, 2015
View reviewed changes

LukasReschke mentioned this pull request Feb 6, 2015

Revert "Add OCS sharing info to capabilities" #13960

Merged

MorrisJobke removed this from the 8.1-next milestone Feb 17, 2015

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OCS sharing info to capabilities #13692

Add OCS sharing info to capabilities #13692

rullzer commented Jan 26, 2015

PVince81 commented Jan 26, 2015

PVince81 commented Jan 26, 2015

PVince81 commented Jan 26, 2015

DeepDiver1975 commented Jan 26, 2015

DeepDiver1975 commented Jan 26, 2015

MorrisJobke Jan 26, 2015

PVince81 commented Jan 27, 2015

DeepDiver1975 commented Jan 27, 2015

rullzer commented Jan 27, 2015

DeepDiver1975 commented Jan 27, 2015

rullzer commented Feb 4, 2015

DeepDiver1975 Feb 5, 2015

rullzer Feb 5, 2015

rullzer commented Feb 6, 2015

scrutinizer-notifier commented Feb 6, 2015

ghost commented Feb 6, 2015

karlitschek commented Feb 6, 2015

craigpg commented Feb 6, 2015

LukasReschke commented Feb 6, 2015

LukasReschke commented Feb 6, 2015

LukasReschke Feb 6, 2015

rullzer commented Feb 6, 2015

LukasReschke Feb 6, 2015

PVince81 commented Feb 6, 2015

DeepDiver1975 commented Feb 6, 2015

LukasReschke commented Feb 6, 2015

Add OCS sharing info to capabilities #13692

Add OCS sharing info to capabilities #13692

Conversation

rullzer commented Jan 26, 2015

PVince81 commented Jan 26, 2015

PVince81 commented Jan 26, 2015

PVince81 commented Jan 26, 2015

DeepDiver1975 commented Jan 26, 2015

DeepDiver1975 commented Jan 26, 2015

MorrisJobke Jan 26, 2015

Choose a reason for hiding this comment

PVince81 commented Jan 27, 2015

DeepDiver1975 commented Jan 27, 2015

rullzer commented Jan 27, 2015

DeepDiver1975 commented Jan 27, 2015

rullzer commented Feb 4, 2015

DeepDiver1975 Feb 5, 2015

Choose a reason for hiding this comment

rullzer Feb 5, 2015

Choose a reason for hiding this comment

rullzer commented Feb 6, 2015

scrutinizer-notifier commented Feb 6, 2015

ghost commented Feb 6, 2015

karlitschek commented Feb 6, 2015

craigpg commented Feb 6, 2015

LukasReschke commented Feb 6, 2015

LukasReschke commented Feb 6, 2015

LukasReschke Feb 6, 2015

Choose a reason for hiding this comment

rullzer commented Feb 6, 2015

LukasReschke Feb 6, 2015

Choose a reason for hiding this comment

PVince81 commented Feb 6, 2015

DeepDiver1975 commented Feb 6, 2015

LukasReschke commented Feb 6, 2015