[Freeipa-devel] [TESTS][PATCH 0007] Multiple managers per user

[Martin Basti]

On 11.01.2016 12:47, Lenka Doudova wrote:
> Hi,
>
> everyone having time to take a look at this? It's been hanging for a 
> few weeks.
> Thanks,
> Lenka
>
> On 12/15/2015 12:42 PM, Lenka Doudova wrote:
>> Hi,
>>
>> I updated the (stage)user tests to reflect the multiple managers per 
>> user feature.
>> Corresponding ticket: https://fedorahosted.org/freeipa/ticket/5344
>>
>> Lenka
>>
>>
>
>
>
We enabled more checks in pylint recently, your patch currently does not 
pass on master.

./make-lint
************* Module ipatests.test_xmlrpc.tracker.user_plugin
ipatests/test_xmlrpc/tracker/user_plugin.py:380: 
[W0106(expression-not-assigned), UserTracker.track_add_manager] 
Expression "[self.attrs[u'manager'].append(item) for item in managers]" 
is assigned to nothing)
************* Module ipatests.test_xmlrpc.tracker.stageuser_plugin
ipatests/test_xmlrpc/tracker/stageuser_plugin.py:248: 
[W0106(expression-not-assigned), StageUserTracker.track_add_manager] 
Expression "[self.attrs[u'manager'].append(item) for item in managers]" 
is assigned to nothing)

1)
I suggest to use self.attrs[u'manager'].extend(managers)  instead of 
list comprehension (lint errors)

2)
if self.attrs[u'manager'] == []:

I suggest to use
if not self.attrs[u'manager']:
this is proper python usage.

*on several places in code

3)
+    def test_delete_all_managers(self, stageduser, manager1, manager2):
...
+
+        updates = {u'manager': ""}
+        expected_updates = {u'manager': ""}
...
+        stageduser.attrs.update(updates)
+        stageduser.attrs.update(expected_updates)

I do not understand this, you have 2 dictionaries that are the same, and 
you twice update stageuser object with the same value, What did I miss?

4)
+                self.attrs[u'managers_failed'][key] =\
+                    u'This entry is already a member'

Please try to avoid '\' in code if possible
                 self.attrs[u'managers_failed'][key] = (
                     u'This entry is already a member')

*on several places in code

5)
I do not have a feeling that the following code is right:

          if u'nsaccountlock' in expected:
              if expected[u'nsaccountlock'] == [u'true']:
                  expected[u'nsaccountlock'] = True
              elif expected[u'nsaccountlock'] == [u'false']:
                  expected[u'nsaccountlock'] = False
+        else:
+            expected[u'nsaccountlock'] = False

Why is there added 'nsaccountlock' into dictionary when it is not 
expected to be in results? Shouldn't be 'nsaccountlock' added to 
expected when needed?

6)
-            expected['result']['manager'] = [
- unicode(get_user_dn(expected['result']['manager'][0]))]
+            for i, item in enumerate(expected['result']['manager']):
+                expected['result'][u'manager'][i] = 
unicode(get_user_dn(item)

Please use list comprehension, it is safer than changing list that is 
being iterated.

expected['result']['manager'] = [unicode(get_user_dn(mgr)) for mgr in 
expected['result']['manager']]

7)
+        if len(self.attrs[u'managers_failed']) != 0:
This should be rewritten to

if self.attrs[u'managers_failed']:

*on several places in code

8)
+        if u'manager' in self.attrs and self.attrs[u'manager'] == []:
+            del self.attrs[u'manager']

IMO list == [] is bad, this can be rewritten to:

if u'manager' in self.attrs and not self.attrs[u'manager']:


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160111/31710762/attachment.htm>