Directly use WorkflowTool to access time value.

Mixed commit:

  43b1ae1b
  2f05fe1b

product/ERP5Type/Base.py

@@ -1823,7 +1823,7 @@ class Base( CopyContainer,
   def _setValue(self, id, target, spec=(), filter=None, portal_type=(), keep_default=1,
                                   checked_permission=None):
     getRelativeUrl = self.getPortalObject().portal_url.getRelativeUrl
-        
+    
     def cleanupCategory(path):
       # prevent duplicating base categories and storing "portal_categories/"
       for start_string in ("portal_categories/", "%s/" % id):
@@ -3138,13 +3138,14 @@ class Base( CopyContainer,
       pass
     else:
       max_date = None
-      for history in history_list.itervalues():
-        try:
-          date = history[-1]['time']
-        except (IndexError, KeyError, TypeError):
-          continue
-        if date > max_date:
-          max_date = date
+      portal_workflow = getToolByName(self.getPortalObject(), 'portal_workflow')
+      getStatusOf = portal_workflow.getStatusOf
+      for key, _ in history_list.iteritems():
+        status = getStatusOf(key, self)
+        if status is not None and status.has_key('time'):
+          date = status['time']
+          if date > max_date:
+            max_date = date
       if max_date:
         # Return a copy of history time, to prevent modification
         return DateTime(max_date)

[Rafael Monnerat]

@nexedi, Does anyone objects to merge this old change from SlapOS Master on master branch of erp5?

[Romain Courteaud]

@vpelletier if I remember correctly, the goal was to prevent loading the full workflow history in RAM (which makes ERP5 unusable in case of huge history).

@rafael is it possible to replace iteritems by iterkeys?

[Kazuhiko Shiozaki]

Even with the original implementation, we do not load FULL workflow history.

Here, history_list is a persistent mapping whose key is workflow id and value is small WorkflowHistoryList instance, that is a chain of small WorkflowHistoryList instances. And it is optimised for accessing the LAST value. And here we only access the last item in each workflow list, thus data loading is minimum.

I just checked the DCWorkflow implementation and I believe that 'what is accessed' is quite same in the original implementation and the modified version.

[Vincent Pelletier]

@rafael is it possible to replace iteritems by iterkeys?

Actually, there are 2 things which can be improved if we try to keep this patch:

• see if it is possible to get rid of history_list altogether: when using workflow tool API it should not have to be provided
• do not use iteritems or iterkeys, just iterate: for x in some_dict: is just the same as for x in some_dict.iterkeys():, but the former is valid both for 2.x and 3.x, while the latter works only in 2.x (and is more verbose).

I like a lot the removal of try..except block in this patch, as intent is much clearer than catching a bunch of exceptions (Should we tolerate that status suddenly became a list ? This is the only reason I see for a TypeError, and it's misleading.).

Then again, as @kazuhiko points out, [-1] should not cause the whole history list to be loaded (or there is a serious performance issue). On a side note, workflow history should be easy to optimise for both backward and forward traversals, instead of just backward, by also linking oldest item to chained list's root object. I believe it would benefit Base_viewHistory for large workflow histories.

[EDIT]: s/large workflows/large workflow histories/