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']:


-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to