nexedi / erp5

This MR comes with a test. For the moment, I only focused on the computation part of the problem, and completely left aside the generation of Supply Cells and how this mehod is activated (would it be a checkbox in SaleSupplyLine_viewBasePrice ?).

I would like to validate the data model, and discuss how we could improve it (for exemple, I don't like very much storing a dictionnary in the property sliced_base_price of Supply Cells).

Also, I had to do a change on DomainTool, to allow more than one Predicate (=Supply Cell here) to match and still get all the values of these predicates, whereas currently if a second predicate matches for a same mapped_value_property, then its value is discarded in favor of the value of the first matching predicate.

@jerome , @seb, @jm : what do you think of this WIP MR ? Do you think it is a good starting point, or should I apprehend this problem in a different way ?

There's something I'm not confortable with this at business level, in the example from the description the calculation is 15 * 9.66 = 144.9 so there will be lots of rounding problems. It's not possible at business level to handle this as 10 * 10 + 5 * 9 ie. with different movements ?

• lots of rounding problems and nobody will understand where this price comes from

Hi @jerome

I don't want to work with different movements, as it will become a huge mess in a very short time, both from a Products point of view, and an interface point of view (how do we mark Movements grouped togethers ? If a user changes the quantity on a Movement, which Movement should be updated ? Or should a Movement be added, or deleted ? Should we throw simulation in ?).

The easiest solution that comes to my mind here would be returning a total_price from the Resource, and making the unit price a function of it (just like we make the total_price a function of the price currently). It could be as simple as modifying Resource_getPriceCalculationOperandDict to add a total_price in the dictionary returned, adding a property on Movements total_price, and updating the code to behave correctly depending on if the Movement has the property price or total_price set. I also noticed that in the stock table we index the total_price, which means that we recognise that sometimes it is easier to work with this than the unit price (and it would avoid abusing calls to round() in too many places....)

Another idea: There could be a transformation/trade model line in the process, the input movement is 15 "request for product" which price is 0 and this is transformed in 10 "products" with "0-10" variation and 5 "products" with "10-20" variation (and price are given by variation - but I don't know where we define the quantity steps for a variation). The UI could be: the user change the "request for product" line and getAggregatedAmountList is called to generate the "product" lines with correct quantities and variations. This is more or less how payroll (where we already have slice-base price) work I think. This also does not work out of the box with transformations and we need a script for this.

About total price, stock.total_price is not the total price of movement, this column is misleadingly named, it is the total source (or destination) asset price, ie the "value" of this movement for this side. The data model is really quantity * price, and another way to fit in this data model can be with a line quantity 1 and price 145 and a description on the line, but this does not work when the original quantity ( 10+5 ) needs to be changed by users and if this is we want to track stock of this product.

Anyway, I was not really saying "don't do this way", more "are you sure this is really what business people want ?"

added 2 commits

• 78613254 - SaleSupply: correctly update Supply Cells
• db9680cc - WIP SupplyLine: set sliced_range property on Supply Cells

Compare with previous version

added 8 commits

• 53fc99d5 - erp5_property_sheets: update SupplyLine properties
• c396458f - erp5_pdm : update Resource_getPriceCalculationOperandDict
• 80e583fd - erp5_pdm: update supply_line_interaction_workflow/SupplyLine_updateBasePriceCellRange
• d8cf81ad - erp5_pdm: update SupplyLine's view field to support sliced price
• c168bc4c - fixup
• 0ed65b65 - Resource: add property needed to calculate sliced price
• 44d91466 - SupplyLine: update
• c9da1b7f - testResource: update

Compare with previous version

added 8 commits

• e3a2eef4 - erp5_property_sheets: rename and factor some properties in new property sheet
• e030cc3e - erp5_pdm: add SupplyCell property_sheet to all Supply Cell portal types
• f3600938 - erp5_property_sheets: update SupplyLine property sheet
• ac0002b1 - erp5_pdm: update SapplyLine_viewBasePrice form
• b511cbdb - erp5_pdm: update supply_line_interaction_workflow/scripts/SupplyLine_updateBaseP…
• 6aaf4c79 - erp5_pdm: update SaleSupplyLine_viewBasePrice form
• f01f4c64 - SupplyLine: update
• 04e59994 - testResource: update to follow properties' names

Compare with previous version

added 1 commit

• d041598f - fixup! DomainTool: list predicate values if more than one match

Compare with previous version

added 1 commit

• 373ddc6b - SupplyLine: fixup

Compare with previous version

added 2 commits

• 53b3bcb5 - erp5_pdm: round price when price per sclice is used
• b0edf538 - testResource: update

Compare with previous version

added 1 commit

• c77abdc1 - erp5_pdm_ui_test: update selenium test so it works in chrome

Compare with previous version

added 1 commit

• e248d4bc - erp5_pdm_ui_test: fixup

Compare with previous version

added 1 commit

• 1d5b2089 - erp5_pdm_ui_test: test with 


Compare with previous version

added 1 commit

• 30a50329 - erp5_core: remove abbreviations in Base_edit

Compare with previous version

added 1 commit

• 885b13e3 - DONOTMERGE git add -p tests/! disable some tests

Compare with previous version

added 1 commit

• 874d124a - erp5_core: fixup (Base_edit)

Compare with previous version

added 15 commits

• e09dd56d - erp5_pdm: add workflow interaction to update cells when price is set per slice
• eace4637 - Revert "erp5_pdm: add SupplyCell property_sheet to all Supply Cell portal types"
• 245da7cd - erp5_property_sheets: merge back SupplyCell with SupplyLine
• 9a10398f - erp5_pdm: make SaleSupplyLine_viewBasePrice always use base_price property
• f51ab96e - SupplyLine: use correct sliced_quantity_range property
• 734c6a59 - SupplyLine: add method to update silced_quantity_range
• 69bd46d9 - fixup! Resource: add property needed to calculate sliced price
• aa81bc05 - erp5_property_sheets: put back sliced_base_price
• f7005659 - fixup! Resource: add property needed to calculate sliced price
• 621957dd - erp5_property_sheets: give correct name to per_slice properties of SupplyLine
• 604ef310 - erp5_pdm: update SaleSupplyLine_viewBasePrice
• 0dc53d07 - erp5_pdm: update property names in Resource_getPriceCalculationOperandDict
• 7cbe104e - erp5_pdm: update supply_line_interaction_workflow
• 80597721 - testResource: updaet
• f8852db0 - Resource: update

Compare with previous version

added 7 commits

• a3582961 - DomainTool: list predicate values if more than one match
• 2c6669c5 - erp5_property_sheets: add properties needed to calculate price per slice
• 11f096ae - erp5_pdm: support calculation of base_price applying to all items of a same slice
• 7bc47227 - testResource: test unit price calculated by slice
• 2a240dd6 - erp5_pdm_ui_test: update selenium test so it works in chrome
• 5c0d0dc7 - erp5_core: remove abbreviations in Base_edit
• d55c566d - DONOTMERGE git add -p tests/! disable some tests

Compare with previous version

I have pushed an almost finished version of this new feature.

I'm still having issues with the interface (ie: when changing the quantity_step_list, it doesn't update the cells like I would like).

Test result : https://nexedijs.erp5.net/#/test_result_module/20190704-E433A6C

erp5_pdm_ui_test:testFunctionalPDM is failing as I discovered that this functional test doesn't work when run by myself in the browser. I've been looking on how to make the erp5_pdm_ui_test work on both chrome and firefox, but I didn't find it yet.

I will push the commit of erp5_core to master directly (it is not strictly part of this MR).

@jerome , @seb : could you review this MR ? Also, I would like to know if you have ideas on how to improve the matrixbox UI (for updating the slice_* parameters correctly when the view is updated).

Finally, I need to think how this price calculation plays with other price parameters, and if it is even possible. For exemple, would it make sense to allow "Additional Price" or "Discount Ratio" when we calculate a base price per slice.

( this seems like a useless XML change)

I pushed a fixup to remove this useless change.

What the status of this, it's not passing in chrome and passing in firefox ?

Have you tried like this ? selenium.browserbot.getCurrentWindow().document.getElementsByName("field_my_sale_supply_line_quantity_step_list")[0].value === "10.0
20.0"

full_prop_dict becomes useless with this change. This dict is used as a set to only get the first mapped_value_property, like we did before python had set support.

Isn't there a lower level test that we need to update with this behaviour ?

I'm not sure price is not so much tested, this change might have side effects.

( edit to fix grammar )

@jm : I see you have commits on this function (even if it more about rewriting...), do you think this change could have side effects ?

All tests currently pass with this change (testDomainTool, pdm, trade, ...), so we may lack some tests here.

If we want to keep a strict backward compatibility, maybe we can pass as a parameter of this function a list of properties for which we want to return a list if several predicates match, instead of just the value of the last predicate. I think it would be better than a global switch "return list / return only one parameter for each property tested".

There's a frenchism in that description: s/command/order

also I'm not sure what the proper english term for that. Isn't price per slice about ?

s/command/order/g applied

About "slice", it is a vocabulary that we already use in ERP5 (in payroll at least). "Slice" is also an array function in js, and a datatype in golang. I'm not sure how much "Price per slice" is connoted as pizza/cake though...

Why do we need both slice_base_price_property and base_price_per_slice_property ?

slice_base_price_property is a float property containing the price applying to the slice. It is held by the cells.

base_price_per_slice_property is a boolean that says if this pricing mode should be used or not. It is held by the line.

Why not create cells with newCell ?

Also, I know Products.ERP5Type.tests.utils.updateCellList exists, but I don't know if it (still) works. In budget tests we just use Base_edit in tests, because the logic for creating cells is in fact in Base_edit

I tried with newCell at first, but it didn't work. Maybe it's just because at that time I didn't understand well matrices. I remember to be puzzled because it was simply returning None.

I'll give another try, but newCell doesn't seem easier to use than newContent. I'll think about the best way to make it understandable.

We can maybe use assertAlmostEqual here. Not sure it's really better, but assertion failed message is probably more informative in case of failure.

mmh this might be a bad idea. What we are asserting here is that the rounded price using the method that's used in the UI is equal to the expected value.

Finally, I need to think how this price calculation plays with other price parameters, and if it is even possible. For exemple, would it make sense to allow "Additional Price" or "Discount Ratio" when we calculate a base price per slice.

Isn't it working because of https://en.wikipedia.org/wiki/Distributive_property ?

If there's for example a -10% discount ratio, If we used explicit movements, we would have something like this:

Quantity	Unit Price ( before discount ratio)	Unit Price	Total Price
10	10	9	90
5	9	8.1	40.5

Unit Price: (90 + 40.5)/15 = 8.7, which is same as 9.666666667 * 0.9

---

With additional price of +3, we have:

Quantity	Unit Price ( before additional price)	Unit Price	Total Price
10	10	13	130
5	9	12	60

Unit Price: (130 + 60)/15 = 12.666667, which is same as 9.666667 + 3

You might move changes of Base_edit away from that merge request.

Sorry @Nicolas to only see this merge request now.

I wonder if this is a good direction. I have somehow similar case on one project which is handled by hand just using separated order lines (and with price set by hand).

The case is as follow. Similar to your need here, a supplier sale goods at different price depending on quantity. So then purchase order is created like this :

• order line A : quantity 10, price 10
• order line B : quantity 10, price 9,
• order line C : quantity 5, price 8 Then Purchase Packing List is generated. In july, only 6 items are delivered, so you have two Purchase Packing list :
• PPL1 with :
  • order line A : quantity 6, price 10
• PPL2 with :
  • order line A : quantity 4, price 10
  • order line B : quantity 10, price 9,
  • order line C : quantity 5, price 8 What even happens, is that the supplier even send an invoice for the 4 delivered products, and the invoice would have a price of 10.

So with suggested solution here, we loose tracking of which product has which price. In the case I know, it is very important to track, if it is agreed to cancel ordering of two products, this would be on order line C having price 8 without the need of recalculating average price.

So it might be possible to use matrix on order/delivery line level to set price/quantities for the difference slices without the need to calculate an average price.

And may be two distinct features are needed.

I don't want to be complaining, but to be clear: I still don't like that we have only one movement with the "calculated" price and I feel it should be done with multiple movements (with cells, maybe a local variation to manage the slices). I also don't like that even though this looks very very similar to the "salary ranges" in french payroll (tranche A, tranche B), the data model is different. But this maybe just because I am missing something.

About the payroll model : my opinion is that in reality it is the payroll data model which is very specific to its purpose, and I hope that this Merge Request could bring features generic enough that could be shared with the payroll system.

Some specificities about the payroll model are :

• To define the slices, it uses a custom category salaryrange : the issue is not really about the custom category, but about the fact it uses categories, so it can't be modified by a user.
• The quantity (= gross salary) is defined on only one invoice line of the paysheet, then other lines are calculated automatically, making use of a chain of base_applications/base_contributions . If the quantity changes, user updates only one line, and other lines are recalculated afterwards. In Orders, if we create more than 1 movement, we will have to add too much intelligence in order to update the quantities if the user does some changes.

Am I too nice with my users ? Maybe if they decide to remove 2 products from the wrong line it's their decision. Still, I'm afraid that it would then break the Apply Trade Condition action.

As you both raise the same concerns, I'll discuss with Thierry about it.

• To define the slices, it uses a custom category salaryrange : the issue is not really about the custom category, but about the fact it uses categories, so it can't be modified by a user.

Using "local variations" we can partially do this. At least by adding "Product Individual Variations" in products this solve the fact that this is not a central category that users cannot change.

On resource:

On movement:

What's missing in this configuration is the fact that the local variation with title "0 -> 10" should be used for quantity range from 0 to 10.

• The quantity (= gross salary) is defined on only one invoice line of the paysheet, then other lines are calculated automatically, making use of a chain of base_applications/base_contributions . If the quantity changes, user updates only one line, and other lines are recalculated afterwards. In Orders, if we create more than 1 movement, we will have to add too much intelligence in order to update the quantities if the user does some changes.

I tried again this idea of having a "request for product" service that would be transformed into the product with variations, but several things are missing.

We could configure a transformation (in the abstract term, this is not possible with trade model line, transformations or payroll) to generate:

• a movement of quantity 10 for variation 0-10 if quantity is >= 10 ( using Predicate tab, with quantity as predicate property )
• a movement of quantity 100% of original movement for variation 0-10 if quantity is < 10
• a movement of quantity 100% of original movement for variation 10-20 + a movement of quantity -10 if quantity > 10 , then total of this slice would be 100% - 10 (this is if we only have two slices, but the idea seems working )
• ... etc ( I don't know why we did not implement payroll like this, maybe because it's complex )

then we "build" these movements with cells and variation and let the price calculation happen. Then the quantity for slices would be defined by these predicates.

But this does not work, several things are missing in getAggregatedAmountList, but I feel this is just because we did not implement all the ideas we had at the time ... I could not find the document we wrote ( there's this page on old wiki, but it's different) but we discussed having rules like "if you buy more than 5 products X, you receive 1 product Y for free" and I think we cannot do this yet. This might be a similar thing.

Currently only payroll allow:

• to define the quantity as "absolute value" ( receive 1 product Y for free ), with trade model line it's only percentage of total price.
• to define variations in the generated lines. so things are missing with trade model lines.

These are just some random "brainstorming" ideas, all this is not clear at all to me but I feel what's implemented here only fits the use cases of taxes, but the initial design was more ambitious, maybe just some small parts of UI and maybe "useless" limitations of current getAggregatedAmountList are preventing us from doing this kind of things with transformation only.

Local variation should not be final solution. Slices depends on supply line, we might have various different slices changes from one year to another and changing per supplier.

Sometimes I feel that we like a bit too much very complex solutions.

I might use just very simple solution like a dialog offering the possibility to just split a line with the different quantities with the different prices. This could be much simpler, just reuse what we have, compatible with tracking, simplicity for users, with split and defer, etc.

@Nicolas if you have modification on erp5_core Base_edit, please also port them to hal_style Base_edit and check that the matrixbox works the way you expect.

added 5 commits

• fab7b627 - erp5_pdm: add supply_cell_interaction_workflow
• 28829c5e - fixup! erp5_pdm: support calculation of base_price applying to all items of a same slice
• fb899455 - [WIP] fixup! erp5_pdm: add supply_cell_interaction_workflow
• 0a6664a2 - fxup! erp5_pdm: support calculation of base_price applying to all items of a same slice
• ad1c36c0 - erp5_pdm: remove needless conversion in my_quantity_step_lst fields

Compare with previous version

added 1 commit

• a79c542c - fixup! erp5_pdm: remove needless conversion in my_quantity_step_lst fields

Compare with previous version

added 11 commits

• 84cc2b6b - erp5_property_sheets: allow acquisition for new supply line property
• 15ec3c98 - erp5_pdm: proxify base_price_per_slice field and add it in Resource's views
• e4d27a16 - fixup! erp5_pdm: support calculation of base_price applying to all items of a same slice
• 430e21ae - fixup! erp5_pdm: support calculation of base_price applying to all items of a same slice
• 2cfede8d - erp5_pdm_ui_test: validate product used in tests
• ee72cf86 - fixup! erp5_pdm: proxify base_price_per_slice field and add it in Resource's views
• cf2f6192 - erp5_pdm_ui_test: support Purchase and Internal deliveries too
• c9fb850f - erp5_pdm_ui_test: add tests for the "price per slice" checkbox
• 7cbb491c - erp5_pdm: proxify mapped_value_property_list fields in *SupplyLine_viewBasePrice
• d21f12e0 - erp5_pdm: add base_price_per_slice field on all *SupplyLine_viewBasePrice views
• b55d082b - erp5_trade_ui_test: add functional tests on the use of "price per slice"

Compare with previous version

added 1 commit

• 4ed6da97 - fixup! erp5_pdm: remove needless conversion in my_quantity_step_lst fields

Compare with previous version

added 1 commit

• eae666ab - fixup! erp5_pdm: support calculation of base_price applying to all items of a same slice

Compare with previous version

After discussing of the needs of Thierry on this feature, we have decided to keep going with the current implementation. Even if there is no tracking on how the price is calculated, this is what our customers want.

About rounding issues, this feature is using the "Price precision" (base_unit_price_property) defined on the Product or the Supply Line.

I've extended my current work so this feature works on all Internal/Purchase/Sale Supply Lines, as being defined in a Sale Trade Condition, Sale Supply, or within the product itself.

Still some tests are not passing, I'll be working on it from now on.

added 3 commits

• 2bd56fde - fixup! erp5_property_sheets: add properties needed to calculate price per slice
• 86f92ea6 - fixup! [WIP] fixup! erp5_pdm: add supply_cell_interaction_workflow
• 7cb58641 - fixup! erp5_pdm: support calculation of base_price applying to all items of a same slice

Compare with previous version

added 2 commits

• b5681f45 - fixup! DomainTool: list predicate values if more than one match
• 468bf213 - erp5_trade_ui_test: run functional tests

Compare with previous version

added 1 commit

• c1e3271a - fixup! erp5_trade_ui_test:

Compare with previous version

added 1 commit

• 92cf514b - fixup! erp5_trade_ui_test:

Compare with previous version

added 1 commit

• 7dd06e7e - fixup!

Compare with previous version

added 1 commit

• b961ae56 - fixup! erp5_trade_ui_test: add functional tests on the use of "price per slice"

Compare with previous version

added 152 commits

• b961ae56...2df4b71f - 135 commits from branch nexedi:master
• 5fbca7e0 - DomainTool: list predicate values if more than one match
• 79799ca1 - erp5_property_sheets: add properties needed to calculate price per slice
• dac24ecf - erp5_pdm: support calculation of base_price applying to all items of a same slice
• 165ca62b - testResource: test unit price calculated by slice
• 1780a1e9 - erp5_pdm_ui_test: update selenium test so it works in chrome
• 12748dc5 - erp5_pdm: add supply_cell_interaction_workflow
• 53f425f3 - erp5_pdm: remove needless conversion in my_quantity_step_lst fields
• 4fe72db2 - erp5_property_sheets: allow acquisition for new supply line property
• 2f8aabf0 - erp5_pdm: proxify base_price_per_slice field and add it in Resource's views
• 1b23822a - erp5_pdm_ui_test: validate product used in tests
• 50844f12 - erp5_pdm_ui_test: support Purchase and Internal deliveries too
• eca18b22 - erp5_pdm_ui_test: add tests for the "price per slice" checkbox
• ba56050d - erp5_pdm: proxify mapped_value_property_list fields in *SupplyLine_viewBasePrice
• 3dc49d5b - erp5_pdm: add base_price_per_slice field on all *SupplyLine_viewBasePrice views
• e620c2a4 - erp5_trade_ui_test: add functional tests on the use of "price per slice"
• 6e5bb95f - erp5_trade_ui_test: run functional tests
• 9bc72ff9 - DONOTMERGE git add -p tests/! disable some tests

Compare with previous version

added 3 commits

• 2281e3d3 - Revert "erp5_pdm_ui_test: update selenium test so it works in chrome"
• e421c54b - fixup! erp5_pdm_ui_test: add tests for the "price per slice" checkbox
• fdd7c7ec - Functional PDM and Trade tests now have their dedicated test suite

Compare with previous version

added 2 commits

• e5f441f6 - fixup! erp5_pdm_ui_test: add tests for the "price per slice" checkbox
• 4495b102 - fixup! erp5_pdm_ui_test: add tests for the "price per slice" checkbox

Compare with previous version

added 1 commit

• 9a1f36dd - fixup! erp5_pdm: support calculation of base_price applying to all items of a same slice

Compare with previous version

added 1 commit

• 51bd6859 - fixup! erp5_pdm: support calculation of base_price applying to all items of a same slice

Compare with previous version

added 1 commit

• af77d81e - SQUASH! erp5_pdm_ui_test: validate product used in tests

Compare with previous version

added 1 commit

• 916e5e2f - fixup PdmZuite_createDelivery

Compare with previous version

added 1 commit

• 03e8b4d8 - fixup! PdmZuite_createDelivery

Compare with previous version

Tests are passing (except the usual failures). I had to rebase on master as I encountered the bug about the number of mysql connections

https://nexedijs.erp5.net/#/test_result_module/20190808-7F5338C

For me the last remaining discussion to have is about the change in DomainTool that may return a list instead of a unique value (Nicolas/erp5@5fbca7e0).

This function is used in ERP5 generic here only : https://lab.nexedi.com/Nicolas/erp5/blob/5fbca7e0b4bc2087dfc307d9acb8ee706204afef/product/ERP5/Document/Resource.py#L698

I proposed a solution already (adding a new parameter in generateMultivaluedMappedValue which says which properties can be returned as a list, instead of returning the value of the first predicate found). @jm and @seb , I think you are the ones who know the most about this API and how this can be used in other projects. What is your opinion on this ?

Regards,

Nicolas

Hi,

Below is the latest test result (with a few unrelated failures coming from ERP5-MASTER) : 

https://nexedijs.erp5.net/#/test_result_module/20190828-996A1E3F

@jerome , @seb : could you do a last review before I squash and merge ? I think the last thing that still needs discussion is about the change in DomainTool.py (+ whatever new issue you find).

Regards,

Nicolas

Below is the latest test result (with a few unrelated failures coming from ERP5-MASTER) : 

https://nexedijs.erp5.net/#/test_result_module/20190828-996A1E3F

is it really for this branch ? the hash is different

I'm not sure when the parameter "explanation_only" is used.

But it looks like the code is not doing the same thing as before. Now we have :

explanation_dict = defaultdict(dict)
# then some code not using and not changing explanation_dict
...
if explanation_only:
  return dict(explanation_dict)

So if I'm correct, if explanation_only is passed, we just have empty dict now, and in all cases !

It seems explanation_only is just never used, so this is probably why all tests are passing. So we should either decide that this parameter "explanation_only" is useless and can be dropped, or we should fix.

Grepping on explanation_only` only returns this function in DomainTool.py :

https://lab.nexedi.com/search?utf8=✓&search=explanation_only&group_id=&project_id=1&search_code=true&repository_ref=

The commit introducing it is :

db962268

I asked to @jerome about it, he said that detailed pricing reports are (were ?) used in your project @seb . Could you grep your code base to see if it is still used ? If not I prefer to drop it as it is dead code. Otherwise with your help I'll introduce a small test.

I had a look, and I have no particular comment. I also try to check code with the eyes of someone not having the clue about what this feature is. I see you already added some nice comments like on the propertysheet of SupplyLine. I would explain again also in the docstring of testGetPriceWithBasePriceDefinedPerSlice. This would allows to understand much more quickly how you get the tested values.

For the change of DomainTool, I added a comment.

added 1 commit

• 6d5ed61f - fixup! Zuite_deleteDataForTradeZuite

Compare with previous version

FYI, If you want to get a 'unit price' for Nth item in such configuration, here is the way...

fake_movement.asContext(quantity=N).getTotalPrice() - fake_movement.asContext(quantity=N-1).getTotalPrice()

added 2 commits

• 17075581 - fixup! testResource: test unit price calculated by slice
• 6b059633 - squash! DomainTool: list predicate values if more than one match

Compare with previous version

I applied the last propositions, and tests run OK : https://nexedijs.erp5.net/#/test_result_module/20190917-326570E7

I'll squash and merge.

Thanks @jerome and @seb for your feedbacks !

added 16 commits

• a032230c - DomainTool: list predicate values if more than one match
• d570fb1d - erp5_property_sheets: add properties needed to calculate price per slice
• 96e4c768 - erp5_pdm: support calculation of base_price applying to all items of a same slice
• 22db9230 - testResource: test unit price calculated by slice
• 6e18b0f6 - erp5_pdm: add supply_cell_interaction_workflow
• c4423406 - erp5_pdm: remove needless conversion in my_quantity_step_lst fields
• 452589a4 - erp5_property_sheets: allow acquisition for new supply line property
• 9810bcc6 - erp5_pdm: proxify base_price_per_slice field and add it in Resource's views
• 1a1498fe - erp5_pdm_ui_test: validate product used in tests
• f470d414 - erp5_pdm_ui_test: support Purchase and Internal deliveries too
• 1eeec38f - erp5_pdm_ui_test: add tests for the "price per slice" checkbox
• b1c0a1fe - erp5_pdm: proxify mapped_value_property_list fields in *SupplyLine_viewBasePrice
• 9f4fc0b0 - erp5_pdm: add base_price_per_slice field on all *SupplyLine_viewBasePrice views
• bfbef96e - erp5_trade_ui_test: add functional tests on the use of "price per slice"
• 95a9b050 - erp5_trade_ui_test: run functional tests
• 334b21a7 - Functional PDM and Trade tests now have their dedicated test suite

Compare with previous version

unmarked as a Work In Progress

changed title from WIP : Price per quantity per slice to Price per quantity per slice

merged

mentioned in commit 05f913a0

I think round() here is bad, especially if we want to calculate price for 100 by (total price for 100) - (total price for 99).

If a Sale Supply defines a Price Precision for a product, it doesn't make sense to return a price with more significative number.

See the following example :

• unit_price = 18.5 (n<=49) or 9.25 (n>50)
• base_price precision is 5
• total_price precision is 2 (like cents)

And 'unit price of Nth' can be defined like this :

def getBasePrice(n):
   return round((18.5*49 + 9.25*(n-49)) / n, 5)

def getUnitPrice(n):
  return getBasePrice(n) * n - getBasePrice(n-1) * (n-1)

Ideally, round(getUnitPrice(n), 2) should be ALWAYS 9.25, but in some cases not.

>>> [(n, getUnitPrice(n)) for n in range(50, 700) if round(getUnitPrice(n), 2) != 9.25]
[(523, 9.24489000000085), (593, 9.244729999999436), (634, 9.255949999999757), (646, 9.25503000000026), (664, 9.256349999999657), (680, 9.244330000000446), (681, 9.25597000000107), (692, 9.255449999999655)]

This is why I said bad.

In such average unit_price implementation, the total price of all slices is important. Rounding base_price just loses the way to get the precise total price like above.

In that case the issue is your base_price precision. For exemple, if we now take 6 :

>>> def getBasePrice(n):
...    return round((18.5*49 + 9.25*(n-49)) / n, 6)
... 
>>> [(n, getUnitPrice(n)) for n in range(50, 700) if round(getUnitPrice(n), 2) != 9.25]
[]

Anyway, for any given base_price precision, we can find a number big enough for which we will encounter rounding issues.

Also, if we don't want to set an arbitrary precision, the code handles the case base_unit_price is None :

  if result.get('base_unit_price', None) is None:
    result["base_price"] = total_price / quantity
  else:
    result["base_price"] = round(total_price / quantity, int(round(- log(result['base_unit_price'], 10),0)))

Kazuhiko convinced me that base_unit_price is not the rounding expected for the price returned by the Supply Line, but more a data used to overwrite the precision of the resource, which may not be enough.

Also, in Resource_getPriceCalculationOperandDict the base_price is never returned rounded, so this case shouldn't be an exception.

Kazuhiko will commit a patch.

Why not just supply_line.isBasePricePerSlice() ?

I did this because supply_line.isBasePricePerSlice() was returning the value before the property change. I think it happened when I tried to access base_price_per_slice in an interaction that was not triggered by _setBasePricePerSlice. It must have ended here during a refactoring. I'll fix to make the code more readable.

(In fact as you are going to commit your patch above, there is no need for me to commit this change now).

supply_line.isBasePricePerSlice() was returning the value before the property change.

Why would it ? This is an after interaction. It has no reason to return the original value.

This script depends on isBasePricePerSlice on the Line, but is not called when that value changes.

This is needed to calculate the price : https://lab.nexedi.com/nexedi/erp5/blob/master/product/ERP5/Document/Resource.py#L698-730

Also, I prefer having these values as data instead of logic for different reasons, including backward compability and debugging.

EDIT : deleted because it answers the wrong comment

No, when base_price_per_slice changes the cells are deleted (cf suplly line workflow interaction). This was done to circumvent UI limitation.

And they are deleted because this interaction is not triggered. This is circular reasoning, which is bad.

Independent code must be independent. Not doing so leads to obscure the bugs against which you fought, spent time, and eventually gave up by falling back to a broken solution. We need a proper solution.

Why do we even need these properties ? They are just duplicating other already-available values, and they could just as well be computed when needed without requiring the use of interactions to try to keep them consistent.

This is needed to calculate the price : https://lab.nexedi.com/nexedi/erp5/blob/master/product/ERP5/Document/Resource.py#L698-730

Also, I prefer having these values as data instead of logic for different reasons, including backward compability and debugging.

One of ERP5 design principles is that we must never store the result of computations. This design goes against this principle. If it must stay, it will need stronger motivations.

Also, note how backwards it is that this duplication, intended for easier debugging, is causing bugs.

After putting thoughts in it...

Main reason we have these properties is to get the price_parameter_dict, as computed here : https://lab.nexedi.com/nexedi/erp5/blob/a3c40d0b7e717daa9ffe7e1ef7ac97859b2bc32e/product/ERP5/Document/Resource.py#L698-730

If we decide to not use the slice_base_price and `slice_quantity_range properties in Cells, we may escape this hell of mess of workflow interactions.

We could go with a patch like this (update of the snippet of Resource.py linked above) :

[...]
      # Get price parameters
      price_parameter_dict = {
        [...]
        'getSliceBasePrice': [],
        'getSliceQuantityRange': [],
      }
     [...]
      for mapped_value_property in mapped_value.getMappedValuePropertyList():
        value = getattr(mapped_value, mapped_value_property)
        try:
          value = value()
        except TypeError:
          continue

So the properties are replaced by instance methods getSliceBasePrice and getSliceQuantityRange which will have for side effect of loading the parent (Supply Line) in memory.

Then the supply_cell_interaction_workflow cloud be dropped.

The issue remaining is that we need to update the _range_criterion property of Cells with a workflow interaction on SupplyLine._setBasePricePerSlice(). I think this is not done by updateCellRange, but by Base_edit (correct me if I'm wrong). Then I don't know how to solve this problem (iow : if I just change the value of base_price_per_slice of the line, cells won't be edited, thus their criterion won't change, thus they won't be found by portal_domains.generateMultivaluedMappedValue)

Main reason we have these properties is to get the price_parameter_dict, as computed here :

Interesting. It seems quite un-ERP5 to access values with mere getattr and not accessors. It would indeed seem much better to use individual getters, which we can then customise so they dynamically compute their result. I'm curious of the reasons which led to such implementation.

which will have for side effect of loading the parent (Supply Line) in memory.

Which which is basically free, because parents are virtually always loaded when accessing any document as they are either part of the acquisition chain or were used to even get the object to begin with.

For the predicate part of the question, I sadly do not have my examples at hand now, so I cannot comment in great details. All I remember is that there is a weird duplication of predicates: the cell is a predicate but we also create a separate predicate-only document.

As discussed yesterday, this is incorrect:

• it causes an inconsistency between existing cells and matrix.py-maintained internal cell index
• it bypasses the work done by matrix.py to preserve cells uids

The following code seems to work just as well without this deletion, and all that was needed was triggering another interaction workflow (which I'm not sure we should be having to begin with):

supply_line = state_change['object']
supply_line.updateQuantityPredicate(
  'slice_base_price'
  if supply_line.isBasePricePerSlice() else
  'base_price'
)
supply_line.updateCellRange(base_id='path')
for cell_value in supply_line.getCellValueList(base_id='path'):
  # XXX: only to trigger supply_cell_interaction_workflow/scripts/SupplyCell_updateSliceBasePrice
  cell_value.setBasePrice(cell_value.getBasePrice())

/cc @jerome @kazuhiko @seb

I run the functionnal tests with your code, and they pass. You can commit it.

That XXX in my code is bad, so it cannot be used as such. More work is needed before it can be applied.