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)

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 )

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 ?

Why do we need both slice_base_price_property and base_price_per_slice_property ?

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

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.