WIP: New ERP5-style workflows
These changes will migrate the Workflow Tool automatically when restarting Zope and will allow to convert old DC Workflows and use both workflows in the same instance. More details on the Technical note.
TODO:
-
fix broken tests:
- testDms
- testERP5Workflow
- testKM (in compatibility with DC Workflow test suite)
-
Added 6 commits:
- 39870065 - ERP5Workflow : initial work on moving from DC Workflows to ERP5 Workflows
- fc19cd6c - ERP5Workflow: refactoring of ERP5 Workflow
- aea163cd - ERP5Workflow: changes related to the configurators
- bb579846 - ERP5Workflow: changes on configurator
- 25e5288f - ERP5Workflow: testDms: filter event with None action in getWorkflowEventInfoList
- 08867eee - ERP5Workflow: always sort workflow list associated to portal types
Toggle commit list -
Marked the task fix broken tests: as completed
Toggle commit list -
Marked the task fix broken tests: as incomplete
Toggle commit list -
Marked the task testDms as completed
Toggle commit list -
Marked the task fix broken tests: as completed
Toggle commit list -
Marked the task fix broken tests: as incomplete
Toggle commit list -
Marked the task testERP5Workflow as completed
Toggle commit list -
Marked the task testERP5Workflow as incomplete
Toggle commit list -
Marked the task testDms as incomplete
Toggle commit list -
Marked the task testDms as completed
Toggle commit list -
1 import unittest 2 from erp5.component.test.testWorkflowMixin import testWorkflowMixin 3 4 class TestWorklist(testWorkflowMixin): -
Isn't this a copy (and not move) of
product/ERP5/tests/testWorklist.py?
-
-
bt5/erp5_graph_editor/SkinTemplateItem/portal_skins/erp5_graph_editor/Workflow_resetGraph.py 0 → 100644
1 # after calling this script, positions of the states will be chosen by jsplumb 2 # and manually changed positions which were saved by a user will be erased 3 context.setProperty('jsplumb_graph', None) 4 context.Base_redirect(form_id='Workflow_viewGraph') -
this dialog action does not seem to comply with http://www.erp5.org/HowToCreateActionDialogs
-
-
bt5/erp5_administration/ExtensionTemplateItem/portal_components/extension.erp5.ERP5Administration.py
186 186 def indent(text): 187 187 return ''.join((" " + line) for line in text.splitlines(True)) 188 188 189 is_python_script = 'bound_names' in data -
Why renaming this variable ?
Also, this change should be done in a separate commit, properly explained on its own and pushed in master without aiting for this merge request.
-
-
26 26 wf = getattr(portal_workflow, wf_id) 27 27 if getattr(wf, "interactions", marker) is marker: # only way to make sure it is not an interaction workflow ? 28 28 result.append((wf, wf_id, 'workflow')) 29 for state_id, state in wf.states.items(): 29 for state_id, state in wf.getStateValueList().items(): -
Is
*ValueListnaming correct here ? I believe we are supposed to receive a list of documents from it, not a dictionary-ish. Or is this also applicable to all other*ValueListaccessors ?
-
-
460 474 return clone(expanded_class_definition); 461 475 } 462 476 463 function openEdgeEditionDialog(gadget, connection) { -
What is the reason for deleting these functions ? This is used in business process graph editor ... which not tested, so the test suite will not detect if this got broken.
There was a qunit test for this gadget in
portal_skins/erp5_graph_editor/dream_graph_editor/jsplumb/test.htmlbut this test is not part of ERP5 test suite either. I just tried to run this test locally (before these changes) and it fails.@romain what to do with this qunit test ? rewrite it as a Zelenium test ?
-
Continuing with this theory that this MR removes most of graph editor because it was broken, we have repaired graph editor (with tests this time) in !321 .
I think we should rebase this MR to drop these changes on graph editor.
This MR also contain changes to use a random seed in the automatic layout feature of graph editor, so that the graph is alway initialized with stable layout. We could also extract this in another small merge request.
-
-
502 486 self.assertTrue(message in detail_list, detail_list) 503 487 self.assertTrue(detail_list.count(message), 1) 504 488 505 def test_workflow_chain_constraint(self): -
The commits are way too large and mix too many changes.
It should be very easy to split them to reduce the number of changes happening at the same time, and allowing some obviously-correct changes to get in very quickly.
For example, moving tests to portal_components does not belong to this merge request. It could, and hence must, be done separately (before or after depending on whether the workflow work depends on it, this does not matter).
-
455 455 'setTypeWorkflowList') 456 456 def setTypeWorkflowList(self, type_workflow_list): 457 457 """Setter for 'type_workflow' property""" 458 self.workflow_list = type_workflow_list 458 # We use 'sorted' below to keep an order in the workflow list. Without 459 # this line, the actions can have different order depending on the order 460 # set during the installation or later. This is bad! 461 # It might not be the ideal solution, if you need to have the workflow 462 # defined in a specific order. Then, your new implementation should use 463 # indexes on workflows as in portal types action's priority. 464 # Note: 'sorted' also convert a tuple or a set to a list 465 self.workflow_list = sorted(type_workflow_list) -
Edited
Why not directly implementing the use of sort_index (or whatever it is called exactly) ?
Also, if this change fixes a regression comming from this patch set, it should not be separate from it, but squashed in the commit which introduces the regression.
[edit] And of course if it fixes an issue which pre-dates this merge request, it should be pushed to master without depending on this merge request.
-
-
-
Should be pushed to master without waiting for this merge request.
Unless if the code replaced here comes from this merge request, in which case this change should be squashed in the commit which introduces this code.
-
-
-
I believe this should be squashed in the commit which adds these classes.
-
-
10 <key> <string>categories</string> </key> 11 <value> 12 <tuple> 13 <string>destination/workflow_module/ebusiness_lotse_configuration_workflow/7</string> 14 </tuple> 15 </value> 16 </item> 17 <item> 18 <key> <string>description</string> </key> 19 <value> 20 <none/> 21 </value> 22 </item> 23 <item> 24 <key> <string>id</string> </key> 25 <value> <string>4</string> </value> -
Is it good to commit objects with such annonymous ids ? For example, I believe portal types are committed using actions' title.
-
-
7 7 <pickle> 8 8 <dictionary> 9 9 <item> 10 <key> <string>_Add_portal_content_Permission</string> </key> -
This change seems unrelated
-
-
169 169 <string>modification_date</string> 170 170 <string>Modification Date</string> 171 171 </tuple> 172 <tuple> 173 <string>portal_type</string> 174 <string>Type</string> -
This change seems unrelated (and inconsistent with modules guidelines)
-
-
19 19 <value> <string> Setup an Accounting Account. </string> </value> 20 20 </item> 21 21 <item> 22 <key> <string>erp5workflow_list</string> </key> -
Why do we need to define all these properties on documents (as opposed to defining them on class with a suitable default value) ?
Or is it another case of using tuple vs. list to encode some dinstinction (like role acquisition does) ?
-
-
1649 1649 </dictionary> 1650 1650 </pickle> 1651 1651 </record> 1652 </ZopeData> 1652 </ZopeData> -
This change seem unrelated
-
-
Thanks for your comments.
Unfortunately, @iv is not available this week to work on this.
I need myself to spend at least 2 or 3 days reviewing the code, check anything that sounds useless, proxify fields, continue checking security declaration. I will review your comments in the same time.
@vpelletier for splitting the commit. Easy is not the word I would use. I can split some parts with some effort. But anyway you will finish having a very big commit, too many changes are related between them.
I let you know once I spent my 2-3 days on this. This will not be this week, since I have something else to deliver, but next week should be ok.
-
Easy is not the word I would use.
It is significantly easier to do now before the push than later when trying to understand why some small change was part os a huge commit which does not explain why it happens.
And it would have been even easier if it was done while the change was prepared (resolving conflicts between one own's patches is never immediately rewarding nor fun). Doing small, topical commits and in descending obvious-correctness order is best: they can be pushed to master while one works on a feature, so there is gradual progress happening on master and the final change is review-friendly. Maybe it will happen next time.
But anyway you will finish having a very big commit
I indeed expect the final commit will be large, but "large" is better than "huge".
You can check how I merged the ERP5 Login patches, and the many preparatory commits I could extract. Despite this work, ERP5 Login final commits are still large. They could probably have been reduced further, though it was getting much harder to not break change dependencies. But they would have been ten times larger without the patchset rework I did.
I let you know once I spent my 2-3 days on this.
Thanks in advance for your effort.
-
Please register or sign in to post a comment