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

Create infrastructure to allow files to be shared with circles #6959

Conversation

viniciuscb
Copy link
Member

This PR is necessary to nextcloud/circles#141 .

Proposal and discussion is on nextcloud/circles#137 .

This commit allows the user to view the calendars that are shared with any circle s/he belongs to.

Signed-off-by: Vinicius Cubas Brand <viniciuscb@gmail.com>
There is a proposal to allow users to filter files shared to circles. This commit is needed to provide the infrastucture for it.

Issue: nextcloud/circles#137

Signed-off-by: Vinicius Cubas Brand <viniciuscb@gmail.com>
…ivoEITA/server into add_circle_to_caldav_and_filepanel
Signed-off-by: Vinicius Cubas Brand <viniciuscb@gmail.com>
throw new TagNotFoundException('Tag with ids ' . implode(', ', $unknownTagIds) . ' not found');
}
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

Maybe cleaning those commented line

@ArtificialOwl
Copy link
Member

Result for anyone wondering:

selection_092

@ArtificialOwl
Copy link
Member

it might needs some unit tests

@ArtificialOwl ArtificialOwl added this to the Nextcloud 13 milestone Oct 25, 2017
@viniciuscb
Copy link
Member Author

@daita we were informed that nc13 will freeze the new features this friday. What can we make to have this PR accepted?

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I don't really like all this merging of circles into the dav app. We should instead add hooks, so the circles app can provide the functionality with code in it's own Namespace

@@ -45,6 +45,7 @@ class FilesReportPlugin extends ServerPlugin {
const NS_OWNCLOUD = 'http://owncloud.org/ns';
const REPORT_NAME = '{http://owncloud.org/ns}filter-files';
const SYSTEMTAG_PROPERTYNAME = '{http://owncloud.org/ns}systemtag';
const CIRCLE_PROPERTYNAME = '{http://owncloud.org/ns}circle';
Copy link
Member

Choose a reason for hiding this comment

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

Should be {http:/nextcloud.com/ns}circle

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, adding hooks in dav app would be more elegant. But since system tags are already core factored in dav, we thought a first approach would be to have both system tags and circles working similarly.

We would be happy to collaborate to implement hooks in dav in the future, and then migrate both circles and system tags to the hooks approach.

Maybe, for NC13, we could start the way we've implemented now... What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

There is another difference ;)
systemtags are shipped (therefor the code is not missing etc) while circles are totally optional

@ArtificialOwl
Copy link
Member

The hook thing could be a great improvement !

Signed-off-by: Vinicius Cubas Brand <viniciuscb@gmail.com>
@codecov
Copy link

codecov bot commented Nov 1, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@c05e8ac). Click here to learn what that means.
The diff coverage is 16.27%.

@@            Coverage Diff            @@
##             master    #6959   +/-   ##
=========================================
  Coverage          ?   29.49%           
  Complexity        ?    24490           
=========================================
  Files             ?     1581           
  Lines             ?    93622           
  Branches          ?     1359           
=========================================
  Hits              ?    27615           
  Misses            ?    66007           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/CalDAV/CalDavBackend.php 0% <0%> (ø) 219 <0> (?)
apps/dav/lib/Connector/Sabre/SharesPlugin.php 78.75% <100%> (ø) 20 <0> (?)
apps/dav/lib/Connector/Sabre/FilesReportPlugin.php 79.73% <23.07%> (ø) 50 <3> (?)
apps/dav/lib/Connector/Sabre/Principal.php 63.15% <4%> (ø) 30 <11> (?)
core/js/files/client.js 82.82% <66.66%> (ø) 0 <0> (?)

@@ -327,6 +342,13 @@ private function getSystemTagFileIds($systemTagIds) {
return $resultFileIds;
}

private function getCirclesFileIds($circlesIds) {
Copy link
Member

Choose a reason for hiding this comment

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

PHPDocs on the function parameters and the return value would be awesome.

Also is $circlesIds an array? If so you can typehint it with array $circleIds.

throw new Exception('Principal not found');
}

$userSession = \OC::$server->getUserSession();
Copy link
Member

Choose a reason for hiding this comment

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

Can we inject the current user in the constructor instead?

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl
Copy link
Member

@viniciuscb - I made the few changes requested by @LukasReschke
Can you test it on your side so we merge this PR before Friday.

@viniciuscb
Copy link
Member Author

Hi @daita , taking a look...

@viniciuscb
Copy link
Member Author

@daita Seems to be working fine! In my opinion, can merge.

$favoriteFilter = null;
foreach ($filterRules as $filterRule) {
if ($filterRule['name'] === $ns . 'systemtag') {
$systemTagIds[] = $filterRule['value'];
}
if ($filterRule['name'] === $ns . 'circle') {
Copy link
Member

Choose a reason for hiding this comment

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

it's CIRCLE_PROPERTYNAME now

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@@ -502,6 +503,9 @@
_.each(filter.systemTagIds, function(systemTagIds) {
body += ' <oc:systemtag>' + escapeHTML(systemTagIds) + '</oc:systemtag>\n';
});
_.each(filter.circlesIds, function(circlesIds) {
body += ' <oc:circle>' + escapeHTML(circlesIds) + '</oc:circle>\n';
Copy link
Member

Choose a reason for hiding this comment

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

nc?

@MorrisJobke
Copy link
Member

I moved this to 14

@MorrisJobke
Copy link
Member

@viniciuscb @daita Sorry that this somehow slipped through. What is the status of this?

@MorrisJobke MorrisJobke dismissed stale reviews from LukasReschke and nickvergessen April 11, 2018 13:20

Fixed

@MorrisJobke MorrisJobke added the stale Ticket or PR with no recent activity label Jun 19, 2018
@MorrisJobke
Copy link
Member

The unit tests in this one fail :/

@danxuliu @georgehrke @daita Mind to have a look at this one?

@nextcloud-bot nextcloud-bot removed the stale Ticket or PR with no recent activity label Jun 29, 2018
@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 9, 2018
@MorrisJobke
Copy link
Member

This was continued in #12119 -> let's close this one here

@MorrisJobke MorrisJobke closed this Nov 7, 2018
@MorrisJobke MorrisJobke removed this from the Nextcloud 15 milestone Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants