nexedi / erp5

This can also be achieved with interaction workflow, isn't it? If so, it is probably not needed to add a new functionnality.

How so? I'm not sure I understand what you mean...

BTW, this is not about adding a new feature, but deprecating type_init_script_id of Portal Types in favor of an implicit getTypeBasedMethod (like _afterClone) to avoid customer-side patch of Portal Type to customize type_init_script_id (or to have to add explicitely an XXX_init to all existing Portal Types).

If I remember correctly, this was already proposed by someone which wasn't someone from Nexedi, and rejected long time ago. I'm not sure why, maybe someone remember. @nexedi

There are 2 parts in your commit:

• deprecating the type_init_script_id (I fully agree with this one, as patching portal type is too much maintainance work)
• adding the _init getTypeBasedMethod

I'm not sure the second one is needed, because the same can be achieved with an interaction workflow method like:

Portal Type Filter: ['Foo'] 
Methods : ['manage_afterAdd', 'manage_afterClone'] 

If this is enough for all use cases (which has to be confirmed), why adding the new getTypeBasedMethod.

Then, if we really need the type based method, we should take care of possible unwanted side effect of the name you choose: _init. As it is likely that many script _init already exist, they should not be called on unwanted portal types (like a possible Delivery_init, which would be applied on all Delivery portal types)

the same can be achieved with an interaction workflow method

If we replace all init scripts with interaction workflows, this means a lot more portal types will have interaction workflows associated with them. I would like to have a better UI for inspecting all interaction workflows associated with a given portal type. Currently, I find it unnecessarily slow to check the N associated interaction workflows on a document just to see if I can trigger some unusual transition to make its state consistent with whatever happened to the real world equvalent (ex: un-shipping a packing list). Having a centralised view (in the SQL meaning: listing all interactions applicable to considered type, whatever the workflow defining each) would be so much more convenient. If would at least display the regex with a link to the interaction definition, with clear precedence order.

I'm also wondering about the overhead of using an interaction rather than a trivial script call, both in terms of interaction-time call overhead and in term of cost when reloading portal type configuration (ie: the more workflows are associated with any type, the slower reload is overall when whatever change triggers a type reload).

Another thing to consider is backward compatibility.

Most portal types with an init scripts (at least in erp5 repository) are already using something_init ( but as @romain pointed out, "something" might not always be what type based method would select ), but some portal types are also using something else:

$ grep -A1 -r '<key> <string>init_script</string> </key>' . | grep '<value> <string>' | grep -v '><' | grep -v init
./bt5/erp5_data_set/PortalTypeTemplateItem/portal_types/Data%20Set.xml-            <value> <string>Document_setStartDate</string> </value>
./bt5/erp5_open_trade/PortalTypeTemplateItem/portal_types/Open%20Sale%20Order.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_open_trade/PortalTypeTemplateItem/portal_types/Open%20Purchase%20Order.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_open_trade/PortalTypeTemplateItem/portal_types/Open%20Internal%20Order.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Purchase%20Packing%20List.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Inventory.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Sale%20Packing%20List.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Internal%20Packing%20List.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Internal%20Order.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Returned%20Sale%20Packing%20List.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Returned%20Purchase%20Packing%20List.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Purchase%20Order.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_trade/PortalTypeTemplateItem/portal_types/Sale%20Order.xml-            <value> <string>Delivery_generateReference</string> </value>
./bt5/erp5_mrp/PortalTypeTemplateItem/portal_types/Production%20Packing%20List.xml-            <value> <string>ProductionDelivery_generateReference</string> </value>
./bt5/erp5_mrp/PortalTypeTemplateItem/portal_types/Production%20Report.xml-            <value> <string>ProductionDelivery_generateReference</string> </value>
./bt5/erp5_mrp/PortalTypeTemplateItem/portal_types/Production%20Order.xml-            <value> <string>ProductionDelivery_generateReference</string> </value>

In the suggested interaction workflow approach, we would just leave the init script feature working as it is today and recommend using interaction workflow instead ?

@romain Thanks for explaining. If I understand correctly, this would mean adding one Interaction Workflow per Portal Type (or at least one Interaction Workflow with one Interaction per Portal Type) and then adding them to template_portal_type_workflow_chain_list. Considering there are already 99 distinct init scripts in ERP5 repository (find -name "*_init.py" | sed -r 's#.*/([A-Za-z_]+\.py$)#\1#g' | sort | uniq | wc -l) and more generally that we would certainly wish to have one for each Portal Types, it seems unpractical to me (especially considering this change only introduces 4 new lines in ERP5Type.py,).

@jerome My commit first calls getTypeInitScriptId() and if such script is not defined, it will calls the getTypeBasedMethod() in order to keep backward compatibility.

I will wait until next Wednesday for further reviews/comments and if there aren't, I will implement the Unit Test and merge.

There are cases a bit useless too inside the 99, for example:

f4f7286ff742eba839d90579b065d251  ./bt5/erp5_trade/SkinTemplateItem/portal_skins/erp5_trade/InternalOrder_init.py
f4f7286ff742eba839d90579b065d251  ./bt5/erp5_trade/SkinTemplateItem/portal_skins/erp5_trade/InternalPackingList_init.py
f4f7286ff742eba839d90579b065d251  ./bt5/erp5_trade/SkinTemplateItem/portal_skins/erp5_trade/PurchaseOrder_init.py
f4f7286ff742eba839d90579b065d251  ./bt5/erp5_trade/SkinTemplateItem/portal_skins/erp5_trade/PurchasePackingList_init.py
f4f7286ff742eba839d90579b065d251  ./bt5/erp5_trade/SkinTemplateItem/portal_skins/erp5_trade/SaleOrder_init.py

Those are exactly the same file with the content:

context.Delivery_generateReference(**kw)

I see some (few others with similar behaviour, only there to call other script with a single line).

Others like: ./bt5/erp5_hr_calendar/SkinTemplateItem/portal_skins/erp5_hr_calendar/PublicHoliday_init.py contains an empty script, only there to allow customisation which could be removed.... there are few others cases.

So maybe if we look carefull, it is not that many real uniq init scripts out there... or at least some of them can be totally useless.

To be clear, I'm not saying you shouldn't do, but it just seems you are just adding a third way to define or overwrite an init script... and in the end nothing will actually change much (just maybe some unintended calls to some *_init that people don't know it existed inside their projects).

@rafael Several of these scripts have been defined to be customized in customer's project. For instance, for my customer, I have 12 of those _init script and several Portal Types patched only to customize the init_script. After seeing some commits from @vpelletier and @seb (for example af68bab2) and doing a git log --stat | grep '_init.xml', it appears to be useful... About PublicHoliday_init.py you're talking about, the commit it was introduced clearly states that projects need it.

About the possibility to use interactions instead of init.... Then don't we have the same issue as before the suggested change here ? I mean the idea of type based init script is to allow easy customization of a generic init script. But if we move generic erp5 init scripts to interaction workflows, isn't it worse for customization ?

@seb interaction workflow you can use:

 Person | -erp5_base_init_workflow
 Person | slapos_base_init_workflow

@arnau You could had created 1 interaction workflow with all the init scripts for your project instead "copy" all 12 portal types.... here is just 2 ways of doing the same thing (which here I consider bad as is a duplication of feature, which I think it is what @romain had in mind).

In any case, I don't mind you apply this change at all, as we have PT_afterClone. The afterClone is an kind of hidden interaction outside the expected API for me. Which is bad for me from general design point of view, like a little hack, but I can understand that this simplifies this particular case.

Another thing I though (again, not preventing you to merge), Did anyone consider that this is a problem of Business Template? You can customize every other aspect of the Portal Type w/o customize it, ie.: allowed content types, categories, actions, etc etc ... but not init script property. Maybe this type based method is just a way to workarround Business Template system with some "non-declarative" configuration.

I'm just posting this to explain my point of view on this "core" feature, and I custom Portal Types on my Project for another reason... (not solvable via this or interaction workflow) so if it will make others life simpler (as you said), so go ahead.

After thinking about it, experimenting and talking with everyone, I will not merge my change and instead I will follow @romain 's approach of using Interaction Workflow for my customer project because:

• When getTypeBasedMethod() is called, it looks for PortalType_initand then in its MRO from top to bottom for a script matching CLASSNAME_init. One problematic use case that @vincent pointed out and I didn't consider when submitting this MR (considering Portal Type PT1 and PT2 whose MRO are respectively (A, B) and (C, B) so both have B in their MRO):

  In a customer project, add B_init to customize initialization for both PT1 and PT2, then later on prepare ERP5 upgrade which has since introduced A_init.

  Thus customer project customization for PT1 initialization will now be overriden by A_init and I think it is something very easy to miss/overlook. Moreover, when considering this use case more generally, even for ERP5 repository itself, it is going to be a mess as init scripts are added in bt5 here and there and thus we would need some way to see what's really going on and which brings me to the next point.

• As @rafael pointed out, there is also the problem of _init being non-declarative (and requiring object introspection) and thus not being consistent with our current approach to define everything at bt5 level in a declarative way. So from a practical point of view _init` would mean adding yet another UI to be able to figure out how/where_init`` are being called for a given object/Portal Type by introspection through looking at objects MRO.

I still think that type_init_script_id should be deprecated though. So what about deprecating it in ERP5 itself (while of course keeping backward compatibility) and replace it by Interaction Workflow?

@arnau So... close this merge and remove this branch to clean up?

/cc @vpelletier

closed