ERP5Security: make ERP5GroupManager more resistant in extreme cases

When accessing an ERP5 Site, we might have automatic migration of
tools. When this happens, we might not have yet access to scripts
like ERP5Type_asSecurityGroupId.

There was already try/except, so just move slightly more code
inside.

product/ERP5Security/ERP5GroupManager.py

@@ -131,7 +131,6 @@ class ERP5GroupManager(BasePlugin):
         # Fetch category values from defined scripts
         for (method_name, base_category_list) in security_definition_list:
           base_category_list = tuple(base_category_list)
-          method = getattr(self, method_name)
           security_category_list = security_category_dict.setdefault(
                                             base_category_list, [])
           try:
@@ -139,6 +138,7 @@ class ERP5GroupManager(BasePlugin):
             # from here or from _updateLocalRolesOnSecurityGroups.
             # Currently, passing portal_type='' (instead of 'Person')
             # is the only way to make the difference.
+            method = getattr(self, method_name)
             security_category_list.extend(
               method(base_category_list, user_id, person_object, '')
             )
@@ -155,7 +155,7 @@ class ERP5GroupManager(BasePlugin):
         group_id_list_generator = getattr(self, generator_name, None)
         if group_id_list_generator is None:
           generator_name = ERP5TYPE_SECURITY_GROUP_ID_GENERATION_SCRIPT
-          group_id_list_generator = getattr(self, generator_name)
+          group_id_list_generator = getattr(self, generator_name, None)
         for base_category_list, category_value_list in \
             security_category_dict.iteritems():
           for category_dict in category_value_list:

[Vincent Pelletier]

If this statement is moved inside the try..except block, then the method variable is pointless.

[Vincent Pelletier]

This means the loop below will still run, and fail on each iteration. Wouldn't it be better to detect a final None and skip it altogether ?

[Vincent Pelletier]

Reading more of this method, it looks like it needs more work if we consider failure cases: currently, it will cache the value returned when it fails, and that value will survive current transaction. That's bad.

I believe the log statements need to raise, these exception must get through and get caught outside the caching wrapper. This way, incomplete values will not be cached.

If this becomes a performance issue, then a transactional cache should be used (maybe in addition to the short cache, maybe it could even replace it entirely... this would need more performance checking). If both caches are preserved, transactional cache will almost certainly be faster than the current one, so it should be checked first.

[Sebastien Robin]

Hello. Thanks for your review. Your suggestion looks good. Though, I slightly improved a case that was not working with the minimal possible change (and this avoid other people having failing automatic update).

The case I had is really exceptional, it should only happens right after update of products.

It's impossible for me to do more on this at the moment due to some deadlines. If anyone else would like to improve further, I'm all ok with it.