-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Appconfig endpoint #1452
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
Appconfig endpoint #1452
Conversation
|
@nickvergessen, thanks for your PR! By analyzing the annotation information on this pull request, we identified @icewind1991, @LukasReschke and @bartv2 to be potential reviewers |
|
df52a19 to
529162d
Compare
|
@nickvergessen Fails ;) |
|
I would prefer to return a promise from |
|
Can you just take care of fixing the failing phpunit tests which I didn't touch? We can discuss promises later, although I don't feel a lot like breaking the current js API. |
ce9ccc9 to
0aa3e92
Compare
Drone messing up? |
I restarted it. Let's see |
950d457 to
496984d
Compare
496984d to
7da6e6a
Compare
|
I think I found the issue, will fix myself |
7da6e6a to
4743fbc
Compare
Current coverage is 57.26% (diff: 71.18%)@@ master #1452 diff @@
==========================================
Files 1074 1075 +1
Lines 61308 61661 +353
Methods 6848 6892 +44
Messages 0 0
Branches 0 0
==========================================
+ Hits 35084 35312 +228
- Misses 26224 26349 +125
Partials 0 0
|
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
4743fbc to
0d878f9
Compare
|
Rebased for #1822 and added php unit tests @icewind1991 do you want to do the promise stuff in here, or can we do it in a different PR? I have no idea what that is and how it works. |
core/js/public/appconfig.js
Outdated
| data: options.data || {}, | ||
| success: options.success, | ||
| error: options.error | ||
| }) |
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.
Semicolon
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.
Fixed
| * @param {string} key | ||
| * @param {Object} [options] | ||
| * @param {function} [options.success] | ||
| * @param {function} [options.error] |
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.
Proper documentation for the parameters would be nice
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.
Well no documentation means nothing you should use?
Feel free to add something if you think it should be.
Signed-off-by: Joas Schilling <coding@schilljs.com>
| ['name' => 'AppConfig#getKeys', 'url' => '/api/v1/config/apps/{app}', 'verb' => 'GET'], | ||
| ['name' => 'AppConfig#getValue', 'url' => '/api/v1/config/apps/{app}/{key}', 'verb' => 'GET'], | ||
| ['name' => 'AppConfig#setValue', 'url' => '/api/v1/config/apps/{app}/{key}', 'verb' => 'POST'], | ||
| ['name' => 'AppConfig#deleteKey', 'url' => '/api/v1/config/apps/{app}/{key}', 'verb' => 'DELETE'], |
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.
Do we really want to name this /api/ again?
This then results in stuff like http://localhost/server/ocs/v2.php/apps/provisioning_api/api/v1/config/apps/files_sharing/outgoing_server2server_share_enabled
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.
this is what all ocs routes use
|
Beside the naming comment this is fine by me and works 👍 |
|
LGTM |
@LukasReschke @MorrisJobke