Conversation
| res = self.nxc.edit_ldap_config(config_id, data={param_to_change: new_param_value}) | ||
| assert res['ocs']['meta']['statuscode'] == self.SUCCESS_CODE | ||
| new_config_data = self.nxc.get_ldap_config(config_id)['ocs']['data'] | ||
| assert str(new_config_data[param_to_change]) == str(new_param_value) |
There was a problem hiding this comment.
That's interesting, what is the point of the conversion? You treat new_config_data[param_to_change] as integer earlier in the test.
There was a problem hiding this comment.
You right, didn't notice that at first it was integer.
After request it set ldapPagingSize as string, even though I sent integer there. Looks like it happens because of x-www-form-urlencoded Content-type which is required.
(Pdb) config_data[param_to_change]
500
(Pdb) new_param_value
777
(Pdb) new_config_data[param_to_change]
'777'
| return self.requester.post(url) | ||
|
|
||
|
|
||
| class UserLDAP(WithRequester): |
There was a problem hiding this comment.
It would be great to provide methods for configuration s.a. edit_ldap_password and get_ldap_password instead of the edit_ldap_config accepting a opcode argument. It is possible to go the manual way of reading the documentation and creating those methods by hand, or if they are very similar, it may be worth considering to define a list of e.g. (name_stem, argname, docstring) tuples in the module, and creating those methods dynamically in the class constructor using setattr.
Think about that, it is not a requirement for this PR to be merged, it can be added later.
There was a problem hiding this comment.
Great idea, thanks! I've added it as a TODO comment below to refactor it like this later.
|
Please rebase. I have some questions and suggestion, but nothing that could be called a merge blocker. |
danil-topchiy
force-pushed
the
user_ldap
branch
2 times, most recently
from
56e3eba to
6df7799
Compare
|
Rebased it |
danil-topchiy
force-pushed
the
user_ldap
branch
from
6df7799 to
537d5c8
Compare
|
Rebased. Checked notes at #4 issue, agree, will start doing refactoring in separate branch when will have all the current features merged. |
|
The inspection completed: 4 new issues, 7 updated code elements |
|
Sure, agree! |
3 participants
Add class for User LDAP api, tests