OCA / product-pack

Odoo modules related to product packs
GNU Affero General Public License v3.0
45 stars 126 forks source link

[12.0][IMP] sale_product_pack add pack_modifiable in the sale order line. #12

Closed hparfr closed 4 years ago

hparfr commented 4 years ago

It's helpfull to know directly if the sale order line is editable.

If the sale order is edited from an api, onchange are not always triggered so having this field helps to raise an error.

And it can be used to display in the view that the line is not editable.

OCA-git-bot commented 4 years ago

Hi @ernestotejeda, some modules you are maintaining are being modified, check this out!

ernestotejeda commented 4 years ago

I think that being modifiable or not is a characteristic of the product class, not of the order line class. Image we try to modify a SOline and we get an exception and then we realize that the product has that option marked incorrectly, when we uncheck it to fix it we will have to delete and create the SOLine for this change to take effect, or remove the product and select it again. What do you think @pedrobaeza ? Wouldn't it be better to have a related field instead?

hparfr commented 4 years ago

Image we try to modify a SOline and we get an exception and then we realize that the product has that option marked incorrectly, when we uncheck it to fix it we will have to delete and create the SOLine for this change to take effect, or remove the product and select it again.

The product can be in other sale order as well. Or even multiple times in the same sale order and It will be not updated automatically.

pedrobaeza commented 4 years ago

Why the new field is not shown on the UI?

hparfr commented 4 years ago

Why the new field is not shown on the UI? Because there is already a lot of fields on the sale order lines views. For the moment I prefer letting the integrator to decide if he wants to display it and how (with color ? a logo?...)

pedrobaeza commented 4 years ago

Then I don't see any advantage on having the field ...

hparfr commented 4 years ago

Then I don't see any advantage on having the field ...

pedrobaeza commented 4 years ago

That performance is not so critical in this case, taking into account that is not continuously checked and for sure will be in cache. Any other reason?

hparfr commented 4 years ago

Be more pythonic is not a reason good enough ?

I don't see drawbacks of having it as a field.

(BTW, I use it on e-commerce shop to know if the guy is allowed to change quantities on the line and to manage properly the unlink).

pedrobaeza commented 4 years ago

I prefer to provide whole set in the same PR (Odoo UI part).

hparfr commented 4 years ago

Added the field in the view, just for you @pedrobaeza

hparfr commented 4 years ago

Sorry to still be picky, but with this change, a migration script is required for existing sale order lines with pack that moves the value from product template to line. Can you please include it?

Yes you are ;-). I don't think it's worth the effort. Having some existing draft sale order without the flag is ok and there is no value of changing confirmed sale orders.

Please use always snake case for method variables

Fixed; thanks

pedrobaeza commented 4 years ago

OK, please squash commits and I'll merge.

hparfr commented 4 years ago

Thanks @pedrobaeza

pedrobaeza commented 4 years ago

/ocabot merge minor

OCA-git-bot commented 4 years ago

This PR looks fantastic, let's merge it! Prepared branch 12.0-ocabot-merge-pr-12-by-pedrobaeza-bump-minor, awaiting test results.

OCA-git-bot commented 4 years ago

Congratulations, your PR was merged at 0655c84a5b5e007297fba614823f23e6be3a36c6. Thanks a lot for contributing to OCA. ❤️