Conversation
korelstar
added
feature: API
Henni
left a comment
Henni
left a comment
There was a problem hiding this comment.
Awesome stuff!
Just a single change request and afterwards this can be merged.
| } | ||
| } | ||
| return implode('/', $cleanedFolders); | ||
| } |
There was a problem hiding this comment.
We can drop this.
As discussed with @LukasReschke all paths containing /../ are denied by core.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay, I've removed the category name preparation and introduced an exception handling. Please review again, @Henni
Henni
left a comment
Henni
left a comment
There was a problem hiding this comment.
Final change request. Afterwards this will be merged.
| $file->move($newFilePath); | ||
| } | ||
| } catch(\Exception $e) { | ||
| } |
There was a problem hiding this comment.
Can we specify a stricter Exception than the generic \Exception?
Also it would be great to at least give some kind of feedback in form of a server log entry.
There was a problem hiding this comment.
Okay, that's reasonable. I will change this.
There was a problem hiding this comment.
done; ready for merging, now 😃
|
Awesome job @korelstar ! |
|
Great work as always! So i guess we can continue nextcloud/notes-android#210 as soon as it has been tested with combination old/new? |
|
Thanks :-) |
3 participants
Since #6 introduced notes from subdirectories and exposes this in the 3rd-party API as
category(see also #8), we've implemented read-only categories in the Android client (see nextcloud/notes-android#181). However, there is the demand to change the category from the client.Therefore, I extended the API to be capable of doing this (fully backward compatible). The implementation on the client-side is already finished (for now in a simple approach, this will be improved later) and the integration can be tested: nextcloud/notes-android#210
Of course, the web app should be able to display and change categories, too. But my AngularJS skills are limited and there is no agreement on how to layout this new feature. However, this implementation is realized in the
notesservice, so it can be used by the web app, too.