Skip to content

SystemTags for everything not just files #394

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.

Sign up for GitHub

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

Merged
MorrisJobke merged 5 commits into from
Jul 19, 2016
Merged

SystemTags for everything not just files #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e13ff2
Fix TODO and bring in abstraction (similar to comments)
nickvergessen May 24, 2016
c2b077e
Fix doc blocks
nickvergessen May 24, 2016
7c039bc
Allow apps to register SystemTags plugins
nickvergessen May 24, 2016
54708f9
Fix non-existing exception class
nickvergessen May 24, 2016
9795a73
Update since version
nickvergessen Jul 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/dav/lib/RootCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public function __construct() {
\OC::$server->getSystemTagObjectMapper(),
\OC::$server->getUserSession(),
\OC::$server->getGroupManager(),
\OC::$server->getRootFolder()
\OC::$server->getEventDispatcher()
);
$commentsCollection = new Comments\RootCollection(
\OC::$server->getCommentsManager(),
Expand Down
28 changes: 14 additions & 14 deletions apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
use OCP\SystemTag\ISystemTagObjectMapper;
use OCP\IUserSession;
use OCP\IGroupManager;
use OCP\Files\IRootFolder;

/**
* Collection containing object ids by object type
Expand Down Expand Up @@ -64,9 +63,9 @@ class SystemTagsObjectTypeCollection implements ICollection {
private $userSession;

/**
* @var IRootFolder
* @var \Closure
**/
protected $fileRoot;
protected $childExistsFunction;

/**
* Constructor
Expand All @@ -76,27 +75,28 @@ class SystemTagsObjectTypeCollection implements ICollection {
* @param ISystemTagObjectMapper $tagMapper
* @param IUserSession $userSession
* @param IGroupManager $groupManager
* @param IRootFolder $fileRoot
* @param \Closure $childExistsFunction
*/
public function __construct(
$objectType,
ISystemTagManager $tagManager,
ISystemTagObjectMapper $tagMapper,
IUserSession $userSession,
IGroupManager $groupManager,
IRootFolder $fileRoot
\Closure $childExistsFunction
Copy link
Member

@icewind1991 icewind1991 Jul 18, 2016

Choose a reason for hiding this comment

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

I would prefer to inject something less generic (more "typed") than a closure, maybe something like a ``SystemTagsAdapterthat has achilldExists` method

If we do keep the closure than please use callable as type hint instead

Copy link
Member Author

@nickvergessen nickvergessen Jul 18, 2016

Choose a reason for hiding this comment

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

I'd like to keep the API of comments (already released in 9) and tags similar here.
The problem is I didn't want to set up all the stuff, if we don't need it, that's why I made the fallback to closures, but yes, I also don't like this a lot.

) {
$this->tagManager = $tagManager;
$this->tagMapper = $tagMapper;
$this->objectType = $objectType;
$this->userSession = $userSession;
$this->groupManager = $groupManager;
$this->fileRoot = $fileRoot;
$this->childExistsFunction = $childExistsFunction;
}

/**
* @param string $name
* @param resource|string $data Initial payload
* @return null|string
* @throws Forbidden
*/
function createFile($name, $data = null) {
Expand All @@ -105,13 +105,16 @@ function createFile($name, $data = null) {

/**
* @param string $name
* @throws Forbidden
*/
function createDirectory($name) {
throw new Forbidden('Permission denied to create collections');
}

/**
* @param string $objectId
* @return SystemTagsObjectMappingCollection
* @throws NotFound
*/
function getChild($objectId) {
// make sure the object exists and is reachable
Expand All @@ -133,17 +136,13 @@ function getChildren() {
}

/**
* Checks if a child-node with the specified name exists
*
* @param string $name
* @return bool
*/
function childExists($name) {
// TODO: make this more abstract
if ($this->objectType === 'files') {
// make sure the object is reachable for the current user
$userId = $this->userSession->getUser()->getUID();
$nodes = $this->fileRoot->getUserFolder($userId)->getById(intval($name));
return !empty($nodes);
}
return true;
return call_user_func($this->childExistsFunction, $name);
}

function delete() {
Expand All @@ -156,6 +155,7 @@ function getName() {

/**
* @param string $name
* @throws Forbidden
*/
function setName($name) {
throw new Forbidden('Permission denied to rename this collection');
Expand Down
26 changes: 22 additions & 4 deletions apps/dav/lib/SystemTag/SystemTagsRelationsCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@

use OCP\SystemTag\ISystemTagManager;
use OCP\SystemTag\ISystemTagObjectMapper;
use OCP\SystemTag\SystemTagsEntityEvent;
use Sabre\DAV\Exception\Forbidden;
use Sabre\DAV\SimpleCollection;
use OCP\IUserSession;
use OCP\IGroupManager;
use OCP\Files\IRootFolder;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;

class SystemTagsRelationsCollection extends SimpleCollection {

Expand All @@ -40,14 +41,14 @@ class SystemTagsRelationsCollection extends SimpleCollection {
* @param ISystemTagObjectMapper $tagMapper
* @param IUserSession $userSession
* @param IGroupManager $groupManager
* @param IRootFolder $fileRoot
* @param EventDispatcherInterface $dispatcher
*/
public function __construct(
ISystemTagManager $tagManager,
ISystemTagObjectMapper $tagMapper,
IUserSession $userSession,
IGroupManager $groupManager,
IRootFolder $fileRoot
EventDispatcherInterface $dispatcher
) {
$children = [
new SystemTagsObjectTypeCollection(
Expand All @@ -56,10 +57,27 @@ public function __construct(
$tagMapper,
$userSession,
$groupManager,
$fileRoot
function($name) {
$nodes = \OC::$server->getUserFolder()->getById(intval($name));
Copy link
Member

@icewind1991 icewind1991 Jul 18, 2016

Choose a reason for hiding this comment

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

shouldn't the user folder be injected

return !empty($nodes);
}
),
];

$event = new SystemTagsEntityEvent(SystemTagsEntityEvent::EVENT_ENTITY);
$dispatcher->dispatch(SystemTagsEntityEvent::EVENT_ENTITY, $event);

foreach ($event->getEntityCollections() as $entity => $entityExistsFunction) {
$children[] = new SystemTagsObjectTypeCollection(
$entity,
$tagManager,
$tagMapper,
$userSession,
$groupManager,
$entityExistsFunction
);
}

parent::__construct('root', $children);
}

Expand Down
2 changes: 1 addition & 1 deletion apps/dav/tests/unit/SystemTag/SystemTagMappingNodeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public function testDeleteTagExpectedException(ISystemTag $tag, $expectedExcepti
}

/**
* @expectedException Sabre\DAV\Exception\NotFound
* @expectedException \Sabre\DAV\Exception\NotFound
*/
public function testDeleteTagNotFound() {
// assuming the tag existed at the time the node was created,
Expand Down
8 changes: 4 additions & 4 deletions apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public function testGetters($isAdmin) {
}

/**
* @expectedException Sabre\DAV\Exception\MethodNotAllowed
* @expectedException \Sabre\DAV\Exception\MethodNotAllowed
*/
public function testSetName() {
$this->getTagNode()->setName('2');
Expand Down Expand Up @@ -196,7 +196,7 @@ public function testUpdateTagPermissionException($originalTag, $changedArgs, $ex
}

/**
* @expectedException Sabre\DAV\Exception\Conflict
* @expectedException \Sabre\DAV\Exception\Conflict
*/
public function testUpdateTagAlreadyExists() {
$tag = new SystemTag(1, 'tag1', true, true);
Expand All @@ -216,7 +216,7 @@ public function testUpdateTagAlreadyExists() {
}

/**
* @expectedException Sabre\DAV\Exception\NotFound
* @expectedException \Sabre\DAV\Exception\NotFound
*/
public function testUpdateTagNotFound() {
$tag = new SystemTag(1, 'tag1', true, true);
Expand Down Expand Up @@ -294,7 +294,7 @@ public function testDeleteTagPermissionException(ISystemTag $tag, $expectedExcep
}

/**
* @expectedException Sabre\DAV\Exception\NotFound
* @expectedException \Sabre\DAV\Exception\NotFound
*/
public function testDeleteTagNotFound() {
$tag = new SystemTag(1, 'tag1', true, true);
Expand Down
12 changes: 6 additions & 6 deletions apps/dav/tests/unit/SystemTag/SystemTagsByIdCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,14 @@ public function adminFlagProvider() {
}

/**
* @expectedException Sabre\DAV\Exception\Forbidden
* @expectedException \Sabre\DAV\Exception\Forbidden
*/
public function testForbiddenCreateFile() {
$this->getNode()->createFile('555');
}

/**
* @expectedException Sabre\DAV\Exception\Forbidden
* @expectedException \Sabre\DAV\Exception\Forbidden
*/
public function testForbiddenCreateDirectory() {
$this->getNode()->createDirectory('789');
Expand All @@ -107,7 +107,7 @@ public function testGetChild() {
}

/**
* @expectedException Sabre\DAV\Exception\BadRequest
* @expectedException \Sabre\DAV\Exception\BadRequest
*/
public function testGetChildInvalidName() {
$this->tagManager->expects($this->once())
Expand All @@ -119,7 +119,7 @@ public function testGetChildInvalidName() {
}

/**
* @expectedException Sabre\DAV\Exception\NotFound
* @expectedException \Sabre\DAV\Exception\NotFound
*/
public function testGetChildNotFound() {
$this->tagManager->expects($this->once())
Expand All @@ -131,7 +131,7 @@ public function testGetChildNotFound() {
}

/**
* @expectedException Sabre\DAV\Exception\NotFound
* @expectedException \Sabre\DAV\Exception\NotFound
*/
public function testGetChildUserNotVisible() {
$tag = new SystemTag(123, 'Test', false, false);
Expand Down Expand Up @@ -225,7 +225,7 @@ public function testChildExistsNotFound() {
}

/**
* @expectedException Sabre\DAV\Exception\BadRequest
* @expectedException \Sabre\DAV\Exception\BadRequest
*/
public function testChildExistsBadRequest() {
$this->tagManager->expects($this->once())
Expand Down
16 changes: 8 additions & 8 deletions apps/dav/tests/unit/SystemTag/SystemTagsObjectMappingCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public function testAssignTagNoPermission($userVisible, $userAssignable, $expect
}

/**
* @expectedException Sabre\DAV\Exception\PreconditionFailed
* @expectedException \Sabre\DAV\Exception\PreconditionFailed
*/
public function testAssignTagNotFound() {
$this->tagManager->expects($this->once())
Expand All @@ -140,7 +140,7 @@ public function testAssignTagNotFound() {
}

/**
* @expectedException Sabre\DAV\Exception\Forbidden
* @expectedException \Sabre\DAV\Exception\Forbidden
*/
public function testForbiddenCreateDirectory() {
$this->getNode()->createDirectory('789');
Expand Down Expand Up @@ -193,7 +193,7 @@ public function testGetChildNonVisible() {
}

/**
* @expectedException Sabre\DAV\Exception\NotFound
* @expectedException \Sabre\DAV\Exception\NotFound
*/
public function testGetChildRelationNotFound() {
$this->tagMapper->expects($this->once())
Expand All @@ -205,7 +205,7 @@ public function testGetChildRelationNotFound() {
}

/**
* @expectedException Sabre\DAV\Exception\BadRequest
* @expectedException \Sabre\DAV\Exception\BadRequest
*/
public function testGetChildInvalidId() {
$this->tagMapper->expects($this->once())
Expand All @@ -217,7 +217,7 @@ public function testGetChildInvalidId() {
}

/**
* @expectedException Sabre\DAV\Exception\NotFound
* @expectedException \Sabre\DAV\Exception\NotFound
*/
public function testGetChildTagDoesNotExist() {
$this->tagMapper->expects($this->once())
Expand Down Expand Up @@ -321,7 +321,7 @@ public function testChildExistsTagNotFound() {
}

/**
* @expectedException Sabre\DAV\Exception\BadRequest
* @expectedException \Sabre\DAV\Exception\BadRequest
*/
public function testChildExistsInvalidId() {
$this->tagMapper->expects($this->once())
Expand All @@ -333,14 +333,14 @@ public function testChildExistsInvalidId() {
}

/**
* @expectedException Sabre\DAV\Exception\Forbidden
* @expectedException \Sabre\DAV\Exception\Forbidden
*/
public function testDelete() {
$this->getNode()->delete();
}

/**
* @expectedException Sabre\DAV\Exception\Forbidden
* @expectedException \Sabre\DAV\Exception\Forbidden
*/
public function testSetName() {
$this->getNode()->setName('somethingelse');
Expand Down
Loading