OCA
[14.0][IMP] base_exception: Placeholders for exception rule description
Hello.
I changed the exception rules to use Jinja in exception rule descriptions.
For that I changed a bit more in the module so that for exceptions that were detected on underlying records (like in sale_exception) the text is still correctly added to the exceptions summary on the parent records. I adjusted the tests to cover this.
Useful for us as we have a lot of sales exceptions. Some exceptions check whether any child of a sold BOM product contains valid data. For BOMs with lots of children or BOMs in BOMs it is not helpful enough for the user to see that the 1st SO line has the exception "The product has no weight set". With my change you can use "The product {{obj.default_code}} has no weight set" as description.
My changes do not break modules that depend on base_exception. I tested it on sale_exception. The only thing that wouldn't work in sale_exception is if you use the placeholders that the exceptions summary for the SO line wouldn't be correctly computed as sale_exception defines its own fields on the SO line.
EDIT 2: With the latest changes sale_exception will break,
~I wasn't sure whether it is better to have Jinja or a separate code field as placeholders. I like it more to write code, normal non-dev users would rather use Jinja, like in mail templates. That's why I added both to make everyone happy. But for the Jinja stuff I added a dependency to "mail" as this provides already a function that parses Jinja. I saw in another OCA module that it was used like that. I did the same to keep it simple and consistent. If this new dependency is not good, I could move this into a new module.~
EDIT 2: I removed the description code placeholders. Can't say whether they are good for the OCA module. I will add this in a separate module in our repo.
I checked which modules depend on base_exception. The ones I found I already adjusted to work with the new field to define which fields contain sub-records for which exceptions must be checked.
So far I found 5 modules. I already changed the code in them, but I would create the PRs to change them later after the discussion here.
EDIT: I already added the PR for sales_exception: https://github.com/OCA/sale-workflow/pull/3861. Can be used as example or for testing if you want. And for that more changes are needed.
Everything is still more a draft. I would like to hear your opinion on the changes. Also about naming new defs and fields.
Screenshots:
Jinja in exception description:
EDIT 2 Is now
{{...}} and not ${...} to avoid new dependency and to have the same as in later Odoo versions.
Exceptions in sales order:
Hi @sebastienbeau, @hparfr, some modules you are maintaining are being modified, check this out!
IMO you should split this PR into 2.
One which is doing your changes for the _get_sub_exception_field_names, there you can also Backport https://github.com/OCA/server-tools/pull/2670/commits/b5099bc51ea53592cefd5162b6ce72cc5e6edeee from https://github.com/OCA/server-tools/pull/2670 because there are changes on the _reverse_field
And a second PR (this one)
Which just your updates for placeholders in description.
Here i would recommend only to use one method for rendering, since the inline_qweb is not available in v14 stick to Jinja without using the dependency to mail this would ease the FWP to newer versions.
@hparfr, thanks for your reply.
A flag like in the screenshot of the PR description? To choose between plain text description and Jinja. I can add this back
@hparfr, I can add it behind feature flag like in the 1st screenshot:
Can you check PR https://github.com/OCA/server-tools/pull/3354? Maybe that one can be merged 1st
IMO. We should get rid of the two different behaviors. That you can detect exceptions on subrecords and store it there and detect it on subrecords and not store it there. By storing the exceptions always where they are really detected, all of your changes could be simplified.
I think as there are 2 classes, base.exception.method and base.exception, 2 different behaviours are fine. For the case that you do not want to have exception_ids on a model where you will never use it.
Also IMO it would make sense to store the summary when the exceptions are detected. From a performance aspect it should not make a huge difference for plain exceptions.
What is the advantage of saving the summary? The performance drain is probably not much but still a disadvantage for the current state. For my placeholders it is a disadvantage as data can change on a record which is used as placeholder value.
For example if product_id.default_code of a sales order line is changed but in the summary the old SKU is still shown until a recompute is triggered