-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add note to dav endpoint #12978
Add note to dav endpoint #12978
Conversation
d1e6f2c
to
a08da9c
Compare
Anyone up for a review? @schiessle @rullzer |
do you have a curl example for quickly testing it? |
|
||
$notes = ""; | ||
foreach ($shares as $share) { | ||
$notes .= $share->getNote() . " "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of getting all share notes returned only concatenated by a space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@blizzz curl:
|
@blizzz I am out here somehow… |
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 '';
}
/** |
@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? |
Nevermind this is on my side… |
So this is fine from my side. |
@tobiasKaminsky mind rebasing? I suppose that should make CI run. |
@tobiasKaminsky and, if the changes work for you, please commit them. I cannot since it is in your fork. |
a72024e
to
89ba11b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after formatting ;)
another reviewer, please? |
89ba11b
to
b3de7a7
Compare
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me otherwise
Co-Authored-By: tobiasKaminsky <tobias@nextcloud.com>
Status of 16667: failure
Show full log
Show full log
Show full log
Show full log
Show full log
|
Failures are unrelated -> merge |
]; | ||
|
||
foreach ($types as $shareType) { | ||
$shares = $this->shareManager->getSharedWith($user, $shareType, $this, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this works... The third argument expects a node for sure... but this is a different note that the sharing one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -359,6 +361,12 @@ public function handleGetProperties(PropFind $propFind, \Sabre\DAV\INode $node) | |||
$propFind->handle(self::MOUNT_TYPE_PROPERTYNAME, function () use ($node) { | |||
return $node->getFileInfo()->getMountPoint()->getMountType(); | |||
}); | |||
|
|||
$propFind->handle(self::SHARE_NOTE, function() use ($node, $httpRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -70,6 +70,7 @@ class FilesPlugin extends ServerPlugin { | |||
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also share-note
maybe... as just note
is a bit generic?
Just for client side: will this be in NC16 (according to milestone) or NC17 as feature freeze was on friday? |
|
Fix #11702
It is my first PR, @schiessle helped me a bit on how to do this.