Uh oh!
There was an error while loading. Please reload this page.
Add note to dav endpoint - #12978
Merged
Merged
Conversation
tobiasKaminsky
requested review from
icewind1991, rullzer and schiessle
and removed request for
icewind1991, rullzer and schiessleDecember 10, 2018 14:32
tobiasKaminskyforce-pushed
the
noteOnDav
branch
from
December 10, 2018 14:33
d1e6f2c to
a08da9cCompare
This was referenced Dec 11, 2018
tobiasKaminsky
commented
Dec 11, 2018
MemberAuthor
Anyone up for a review? @schiessle@rullzer |
blizzz
commented
Jan 3, 2019
Member
do you have a curl example for quickly testing it? |
blizzz
requested changes
Jan 3, 2019
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| $notes = ""; | ||
| foreach ($shares as $share) { | ||
| $notes .= $share->getNote() . " "; |
Member
There was a problem hiding this comment.
what is the purpose of getting all share notes returned only concatenated by a space?
MemberAuthor
There was a problem hiding this comment.
I guess only one share type will have a note, so this is just a quick and ugly way not to deal with checking.
Hints are more than welcome ;-)
Member
There was a problem hiding this comment.
I share one file with a "personal note" to a user and the same with a "group note" to a group he is member of. The web interface only displays the personal note. @nextcloud/sharing is this intentional or undefined behaviour?
The request returns two elements with a note each:
<?xml version="1.0"?>
<ocs>
<meta>
<status>ok</status>
<statuscode>200</statuscode>
<message>OK</message>
</meta>
<data>
<element>
<id>14</id>
<share_type>0</share_type>
<.../>
<note>personal note</note>
<path>/php.ini</path>
<item_type>file</item_type>
<file_target>/php.ini</file_target>
<share_with>person_15</share_with>
<.../>
</element>
<element>
<id>15</id>
<share_type>1</share_type>
<.../>
<note>group note</note>
<path>/php.ini</path>
<item_type>file</item_type>
<file_target>/php.ini</file_target>
<share_with>barfoo</share_with>
<.../>
</element>
</data>
</ocs>
Now I shared to another group the target user is member of and added a different note again. This creates another element in the result set. No concatenation takes place. Internally the file is represented by different node instances, therefore you get multiple element sets for the entry.
Therefore, just return the first note you encounter. There won't be others (just leave a comment about that).
I wonder whether there is a short cut to get the share data from the node itself, to avoid asking the share manager about everything. This would improve maintainability, as of new we'd probably forget to query a new sharing type in case it'll be added at some point.
tobiasKaminsky
commented
Jan 4, 2019
MemberAuthor
@blizzz curl: |
tobiasKaminsky
commented
Jan 15, 2019
MemberAuthor
@blizzz I am out here somehow… |
AndyScherzinger
commented
Feb 14, 2019
Member
blizzz
commented
Feb 15, 2019
Member
Um, the provided curl call does not help as it does not got via webdav. If i remember correctly, I was using a dav call back then, but that's too late now. @tobiasKaminsky @nextcloud/sharing what do you think about? (untested) diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php
index fd895c291d..b6433e6e0b 100644
--- a/apps/dav/lib/Connector/Sabre/Node.php+++ b/apps/dav/lib/Connector/Sabre/Node.php@@ -298,25 +298,27 @@ abstract class Node implements \Sabre\DAV\INode {
*/
public function getNoteFromShare($user) {
if ($user == null) {
- return "";+ return '';
}
- $userShares = $this->shareManager->getSharedWith($user, Share::SHARE_TYPE_USER, $this, -1);- $groupShares = $this->shareManager->getSharedWith($user, Share::SHARE_TYPE_GROUP, $this, -1);- $circleShares = $this->shareManager->getSharedWith($user, Share::SHARE_TYPE_CIRCLE, $this, -1);- $roomShares = $this->shareManager->getSharedWith($user, Share::SHARE_TYPE_ROOM, $this, -1);-- $shares = array_merge($userShares, $groupShares, $circleShares, $roomShares);- $shares = array_filter($shares, function (IShare $share) use ($user) {- return $share->getShareOwner() !== $user;- });-- $notes = "";- foreach ($shares as $share) {- $notes .= $share->getNote() . " ";+ $types = [+ Share::SHARE_TYPE_USER,+ Share::SHARE_TYPE_GROUP,+ Share::SHARE_TYPE_CIRCLE,+ Share::SHARE_TYPE_ROOM+ ];++ foreach ($types as $shareType) {+ $shares = $this->shareManager->getSharedWith($user, $shareType, $this, -1);+ foreach ($shares as $share) {+ $note = $share->getNote();+ if($share->getShareOwner() !== $user && !empty($note)) {+ return $note;+ }+ }
}
- return $notes;+ return '';
}
/** |
tobiasKaminsky
commented
Feb 15, 2019
MemberAuthor
@blizzz thanks for taking care of it. and with filled note: On first, I do get null back on android, but it should be "" (empty string), or? |
tobiasKaminsky
commented
Feb 15, 2019
MemberAuthor
Nevermind this is on my side… |
tobiasKaminsky
commented
Feb 25, 2019
MemberAuthor
So this is fine from my side. |
blizzz
commented
Feb 25, 2019
Member
@tobiasKaminsky mind rebasing? I suppose that should make CI run. |
blizzz
commented
Feb 25, 2019
Member
@tobiasKaminsky and, if the changes work for you, please commit them. I cannot since it is in your fork. |
tobiasKaminskyforce-pushed
the
noteOnDav
branch
from
February 25, 2019 11:09
a72024e to
89ba11bCompareblizzz
reviewed
Feb 26, 2019
Uh oh!
There was an error while loading. Please reload this page.
blizzz
commented
Feb 26, 2019
Member
another reviewer, please? |
tobiasKaminskyforce-pushed
the
noteOnDav
branch
from
February 27, 2019 06:00
89ba11b to
b3de7a7CompareSigned-off-by: tobiasKaminsky <tobias@kaminsky.me>
ChristophWurst
approved these changes
Mar 1, 2019
ChristophWurst
left a comment
Member
There was a problem hiding this comment.
Looks good to me otherwise
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: tobiasKaminsky <tobias@nextcloud.com>
MorrisJobke
commented
Mar 4, 2019
Member
Status of 16667: failure
Show full log
Show full log
Show full log
Show full log
Show full log |
MorrisJobke
commented
Mar 4, 2019
Member
Failures are unrelated -> merge |
rullzer
reviewed
Mar 4, 2019
| ]; | ||
| foreach ($types as $shareType) { | ||
| $shares = $this->shareManager->getSharedWith($user, $shareType, $this, -1); |
Member
There was a problem hiding this comment.
I doubt this works... The third argument expects a node for sure... but this is a different note that the sharing one
Member
There was a problem hiding this comment.
Ok so it works because the actual function e use int he share amanger is also present. But this will do 💥 at some point.
Either we revert this or we fix it. Because this is not really something we can leave around and have do 💥 when we move to more strict typing. Especially since this isn't covered by any tests.
Member
There was a problem hiding this comment.
We can fix it by putting an instanceof check against $this beforehand, in line 300. Would be the easiest thing to do, but perhaps not cleanest one.
Alternative would be to retrieve always correct Node instance, if possible. There's no straight way from View or FileInfo, and I did not look deeper by now.
| return $node->getFileInfo()->getMountPoint()->getMountType(); | ||
| }); | ||
| $propFind->handle(self::SHARE_NOTE, function() use ($node, $httpRequest) { |
Member
There was a problem hiding this comment.
Similar comments as to #14429
I think it would make sense to move this to the share plugin we have.
Because in it current state. If you have N entries in the folder you propfind. This will fire off N*4 queries. This won't scale.
rullzer
reviewed
Mar 4, 2019
| const HAS_PREVIEW_PROPERTYNAME = '{http://nextcloud.org/ns}has-preview'; | ||
| const MOUNT_TYPE_PROPERTYNAME = '{http://nextcloud.org/ns}mount-type'; | ||
| const IS_ENCRYPTED_PROPERTYNAME = '{http://nextcloud.org/ns}is-encrypted'; | ||
| const SHARE_NOTE = '{http://nextcloud.org/ns}note'; |
Member
There was a problem hiding this comment.
also share-note maybe... as just note is a bit generic?
tobiasKaminsky
commented
Mar 4, 2019
MemberAuthor
Just for client side: will this be in NC16 (according to milestone) or NC17 as feature freeze was on friday? |
MorrisJobke
commented
Mar 4, 2019
Member
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix#11702
It is my first PR, @schiessle helped me a bit on how to do this.