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.

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

Part 1 of LDAP Backend OCS Api #3162

Merged
merged 12 commits into from Jan 26, 2017
Merged

Part 1 of LDAP Backend OCS Api #3162

merged 12 commits into from Jan 26, 2017

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jan 19, 2017

Contributes to #2694

It brings OCS Api methods for creating, reading, modifying and deleting an LDAP configuration. Including integration tests (Configuration manipulation is against DB only, no LDAP server interaction).

General appconfig does not suffice here, because some values are not written to the DB as is, but need to be transformed first (i.e. multiline values, password). Also some sanity-checking is happening.

Please review @nextcloud/ldap

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member Author

blizzz commented Jan 19, 2017

Hm, actually, unlike the CLI thing, we could set not only one but all any config keys with one request. Will improve this tomorrow.

@blizzz blizzz added the 2. developing Work in progress label Jan 19, 2017
@codecov-io
Copy link

codecov-io commented Jan 19, 2017

Current coverage is 53.76% (diff: 2.67%)

Merging #3162 into master will decrease coverage by 0.17%

@@             master      #3162   diff @@
==========================================
  Files          1299       1303     +4   
  Lines         80135      81003   +868   
  Methods        7909       7974    +65   
  Messages          0          0          
  Branches       1248       1303    +55   
==========================================
+ Hits          43227      43555   +328   
- Misses        36908      37447   +539   
- Partials          0          1     +1   
Diff Coverage File Path
0% apps/user_ldap/lib/Helper.php
0% apps/user_ldap/lib/Command/CreateEmptyConfig.php
0% apps/user_ldap/lib/Configuration.php
0% new ...pps/user_ldap/lib/Controller/ConfigAPIController.php
••• 37% apps/user_ldap/appinfo/routes.php

Powered by Codecov. Last update e9badb9...31a0821

@blizzz
Copy link
Member Author

blizzz commented Jan 19, 2017

  • And integration show up in drone's list, but are still not properly executed.

@@ -413,6 +413,15 @@ pipeline:
when:
matrix:
TESTS: integration-transfer-ownership-features
integration-ldap-features:
Copy link
Member

Choose a reason for hiding this comment

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

indentation is wrong. Let me fix that 😉

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member

And integration show up in drone's list, but are still not properly executed.

Should be fixed now 😃

Copy link
Contributor

@GitHubUser4234 GitHubUser4234 left a comment

Choose a reason for hiding this comment

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

Typo

Then the OCS status code should be "400"
And the HTTP status code should be "400"

Scenario: Modiying a non-existing configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo "Modifying"

Copy link
Member Author

Choose a reason for hiding this comment

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

ty :)

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 20, 2017
Also move ensureConfigIDExists checks into try, it might throw DB
related exceptions

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
covered by "Delete a non-existing configuration"

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member Author

blizzz commented Jan 20, 2017

Hm, actually, unlike the CLI thing, we could set not only one but all any config keys with one request. Will improve this tomorrow.

And done! 😹

Deleted my config and restored it using the API 🚀 🎸

@blizzz
Copy link
Member Author

blizzz commented Jan 23, 2017

Final reviews? 😸

@blizzz blizzz mentioned this pull request Jan 23, 2017
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants