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.

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

add API to change the category #87

Merged
merged 3 commits into from
Jun 6, 2017
Merged

add API to change the category #87

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a4f368
add API to change the category (move note to subdirectory)
korelstar Apr 10, 2017
3b56e4e
don't prepare category name
korelstar May 19, 2017
8f2ba89
log (specific) exception on file movement
korelstar Jun 6, 2017
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
18 changes: 10 additions & 8 deletions controller/notesapicontroller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,15 @@ public function get($id, $exclude='') {
* @NoCSRFRequired
*
* @param string $content
* @param string $category
* @param int $modified
* @param boolean $favorite
* @return DataResponse
*/
public function create($content, $modified=0, $favorite=null) {
return $this->respond(function () use ($content, $modified, $favorite) {
public function create($content, $category=null, $modified=0, $favorite=null) {
return $this->respond(function () use ($content, $category, $modified, $favorite) {
$note = $this->service->create($this->userId);
return $this->updateData($note->getId(), $content, $modified, $favorite);
return $this->updateData($note->getId(), $content, $category, $modified, $favorite);
});
}

Expand All @@ -127,13 +128,14 @@ public function create($content, $modified=0, $favorite=null) {
*
* @param int $id
* @param string $content
* @param string $category
* @param int $modified
* @param boolean $favorite
* @return DataResponse
*/
public function update($id, $content=null, $modified=0, $favorite=null) {
return $this->respond(function () use ($id, $content, $modified, $favorite) {
return $this->updateData($id, $content, $modified, $favorite);
public function update($id, $content=null, $category=null, $modified=0, $favorite=null) {
return $this->respond(function () use ($id, $content, $category, $modified, $favorite) {
return $this->updateData($id, $content, $category, $modified, $favorite);
});
}

Expand All @@ -145,14 +147,14 @@ public function update($id, $content=null, $modified=0, $favorite=null) {
* @param boolean $favorite
* @return Note
*/
private function updateData($id, $content, $modified, $favorite) {
private function updateData($id, $content, $category, $modified, $favorite) {
if($favorite!==null) {
$this->service->favorite($id, $favorite, $this->userId);
}
if($content===null) {
return $this->service->get($id, $this->userId);
} else {
return $this->service->update($id, $content, $this->userId, $modified);
return $this->service->update($id, $content, $this->userId, $category, $modified);
}
}

Expand Down
37 changes: 34 additions & 3 deletions service/notesservice.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public function create ($userId) {
* @throws NoteDoesNotExistException if note does not exist
* @return \OCA\Notes\Db\Note the updated note
*/
public function update ($id, $content, $userId, $mtime=0) {
public function update ($id, $content, $userId, $category=null, $mtime=0) {
$notesFolder = $this->getFolderForUser($userId);
$file = $this->getFileById($notesFolder, $id);
$folder = $file->getParent();
Expand All @@ -140,7 +140,15 @@ public function update ($id, $content, $userId, $mtime=0) {

// generate filename if there were collisions
$currentFilePath = $file->getPath();
$basePath = pathinfo($file->getPath(), PATHINFO_DIRNAME);
if($category===null) {
$basePath = pathinfo($file->getPath(), PATHINFO_DIRNAME);
} else {
$basePath = $notesFolder->getPath();
$categoryPath = $this->getCategoryPath($category);
if(!empty($categoryPath))
$basePath .= '/'.$categoryPath;
$this->getOrCreateFolder($basePath);
}
$fileExtension = pathinfo($file->getName(), PATHINFO_EXTENSION);
$newFilePath = $basePath . '/' . $this->generateFileName($folder, $title, $fileExtension, $id);

Expand Down Expand Up @@ -179,7 +187,7 @@ public function favorite ($id, $favorite, $userId){
$tagger->removeFromFavorites($id);

$tags = $tagger->getTagsForObjects([$id]);
return in_array(\OC\Tags::TAG_FAVORITE, $tags[$id]);
return array_key_exists($id, $tags) && in_array(\OC\Tags::TAG_FAVORITE, $tags[$id]);
}


Expand Down Expand Up @@ -219,6 +227,16 @@ private function getFileById ($folder, $id) {
*/
private function getFolderForUser ($userId) {
$path = '/' . $userId . '/files/Notes';
return $this->getOrCreateFolder($path);
}


/**
* Finds a folder and creates it if non-existent
* @param string $path path to the folder
* @return Folder
*/
private function getOrCreateFolder($path) {
if ($this->root->nodeExists($path)) {
$folder = $this->root->get($path);
} else {
Expand Down Expand Up @@ -262,6 +280,19 @@ private function generateFileName (Folder $folder, $title, $extension, $id) {
}
}

private function getCategoryPath($category) {
$category = str_replace('\\', '/', $category);
$folders = explode('/', $category);
$cleanedFolders = [];
foreach($folders as $folder) {
$folder = trim($folder);
$folder = preg_replace('/^\.+\s*/', '', $folder);
if(!empty($folder)) {
$cleanedFolders[] = $folder;
}
}
return implode('/', $cleanedFolders);
}
Copy link
Member

Choose a reason for hiding this comment

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

We can drop this.
As discussed with @LukasReschke all paths containing /../ are denied by core.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just tested removing this method. Then, the core throws a NotPermittedException. This currently results in not accepting the whole change (i.e. also content changes) and in creating two (!) empty notes. In order to improve the user-experience (a respective category name could be entered by accident), we should handle this exception.
Of course, this can be easily done, but we have to think about, which behavior is desired:

a) Remove problematic parts from the new category name and perform changing the category to this adjusted name (my current approach).
b) Don't prepare category name; check for Exception by core; and ignore category change if Exception happens.

From the user perspective, I would prefer a). 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.

This should only occur with a comment containing /../ right? So I really doubt that any user would enter such a category. I would probably go with b.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've removed the category name preparation and introduced an exception handling. Please review again, @Henni


/**
* gather note files in given directory and all subdirectories
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/controller/NotesApiControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function testUpdate() {
$this->assertEquals($note->getId(), $note2->getId());
$this->assertNotEquals($t, $note2->getModified());

$note3 = $this->controller->update($note->getId(), 'test3', $t)->getData();
$note3 = $this->controller->update($note->getId(), 'test3', null, $t)->getData();
$this->assertEquals('test3', $note3->getContent());
$this->assertEquals($note->getId(), $note3->getId());
$this->assertEquals($t, $note3->getModified());
Expand Down