[12.0][ADD] - New module: report substitute #307

sbejaoui · 2019-07-19T15:31:12Z

This module allows you to create substitution rules for report actions. A typical use case is to replace a standard report by alternative reports when some conditions are met. For instance, it allows to configure alternate reports for different companies.

etobella

I think that you have a recursive issue.

However, it is really interesting!!!

etobella · 2019-07-22T14:12:59Z

report_substitute/models/ir_actions_report.py

+            self.model, res_ids
+        )
+        if substitution_report:
+            return substitution_report.render(res_ids)


Just wondering one thing, with this change, you may be substituting recursively... Maybe you should make:
return super(IrActionReport, substitution_report).render(res_ids)

yes it is a desired behavior.
we return super only when there is no possible substitution. otherwise we continue the substitution recursively.

if substitution_report: return substitution_report.render(res_ids) return super().render(res_ids, data)

@etobella it's true that a user could create loops, and it might be nice to add a better error than a stack overflow if it happens. But that should not be an issue in practice, what do you think?

sbidoul · 2019-07-23T16:15:37Z

Last thing we'll add is an option to let the user decide what to do if there is no matching substitution: either raise an error, or fallback on the base report.

report_substitute/models/ir_actions_report.py

report_substitute/static/src/js/action_manager.js

report_substitute/wizards/mail_compose_message.py

sbidoul · 2019-08-06T10:52:01Z

@sbejaoui can you check travis?

astirpe · 2019-08-06T11:29:25Z

report_substitute/README.rst

+This module allows you to create substitution rules for report actions.
+A typical use case is to replace a standard report by alternative reports
+when some conditions are met. For instance, it allows to configure alternate
+reports for different companies.


To manage alternative reports for different companies, I find the existing OCA module ir_actions_report_multi_company much easier for a non-technical user.

I would change this line in the readme: instead of mentioning reports for different companies, I would expect some other use case. A real case with some code examples would be here very appreciated.

In standard modules, many report calls are hard-coded by their xml ID. An example, the print quotation action in the sale module.

https://github.com/odoo/odoo/blob/48b3893b84cbd51f880548b38d036db5ebc1e256/addons/sale/models/sale.py#L489-L494

Adding an ir_rule to the report action will not resolve the use case here because this type of action will raise an access error for other companies if a company is set.

the alternative solutions that can be made in this case and that we avoid with this module:

override the print action to improve the way odoo get the report action.

override the report template to manage all the layout differences in the same template.

So the idea is to define some substitution rules on the standard report to return the desired one if some condition are met.

IMO, with the domain widget used in the UI, the the substitution rules is easy to use for non-technical user

astirpe · 2019-08-06T11:30:56Z

report_substitute/models/ir_actions_report.py

+    @api.multi
+    def render(self, res_ids, data=None):
+        substitution_report = self.get_substitution_report(res_ids)
+        return super(IrActionReport, substitution_report).render(res_ids, data)


No need to pass parameters to the super()

the super is called on a different object then self. have to be specified.

astirpe · 2019-08-06T11:33:59Z

report_substitute/views/ir_actions_report.xml

+<odoo>
+
+    <record model="ir.ui.view" id="ir_actions_report_form_view">
+        <field name="name">ir.actions.report.form (in report_dispatch_base)


No need to specify the <field name="name"> ... since you are inheriting. Also report_dispatch_base is probably some leftover from old code.

The name is required even in the case of inheritance.

good catch for the leftover.

If you don't specify the name, Odoo will automatically create one.

astirpe · 2019-08-06T11:48:20Z

report_substitute/models/ir_actions_report.py

+        self.ensure_one()
+        action_report = self
+        substitution_report = action_report
+        while substitution_report:


With this while, the matching rule with higher value of the sequence will be executed instead of the previous matching rules. Is this the desired behavior?

The purpose of this while is to continue the substitution recursively. A substitution report can have a matching on a one of its substitution rules.

With this while, the matching rule with higher value of the sequence will be executed instead of the previous matching rules. Is this the desired behavior?

@astirpe Due to this, it's ok https://github.com/OCA/reporting-engine/pull/307/files#diff-53c1b43ed6b9be73de53ef31d1261b38R11

rousseldenis

LGTM. Code review

rousseldenis · 2019-10-01T13:38:47Z

report_substitute/models/ir_actions_report.py

+        self.ensure_one()
+        action_report = self
+        substitution_report = action_report
+        while substitution_report:


With this while, the matching rule with higher value of the sequence will be executed instead of the previous matching rules. Is this the desired behavior?

@astirpe Due to this, it's ok https://github.com/OCA/reporting-engine/pull/307/files#diff-53c1b43ed6b9be73de53ef31d1261b38R11

rousseldenis · 2019-10-01T13:45:12Z

@astirpe Could you update your review?

etobella

For me it's good, just a nitpicking and a comment about the recursion issue.
It is not blocking

report_substitute/__manifest__.py

etobella · 2019-10-01T18:17:46Z

report_substitute/models/ir_actions_report.py

+    )
+
+    @api.multi
+    def _get_substitution_report(self, model, active_ids):


Can you add a limit=1, increase every time and once it has reached 50 raise a Exception?
This way at least we know if we have recursion issue. What do you think?
It is not blocking.

I added a check for the substitution infinite loop when creating the rules. can you check it please.

This addon give the possibility to substitute a report action by another based on some criteria.

…the chatter

sbidoul · 2019-10-21T14:53:55Z

Looks like all comments have been handled here.

/ocabot merge, please

OCA-git-bot · 2019-10-21T14:54:04Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-307-by-sbidoul-bump-no, awaiting test results.

Signed-off-by sbidoul

OCA-git-bot · 2019-10-21T15:27:29Z

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

This comment has been minimized.

Sign in to view

sbidoul mentioned this pull request Jul 22, 2019

[ADD] partner_brand OCA/partner-contact#754

Closed

3 tasks

sbejaoui force-pushed the 12.0-report_dispatch-sbj branch from e462def to bd1e716 Compare July 22, 2019 13:45

sbejaoui changed the title ~~[12.0][WIP] - New module: report substitute~~ [12.0][ADD] - New module: report substitute Jul 22, 2019

etobella requested changes Jul 22, 2019

View reviewed changes

sbidoul added the needs review label Jul 22, 2019

sbejaoui force-pushed the 12.0-report_dispatch-sbj branch 2 times, most recently from c10e81b to 133d289 Compare July 25, 2019 11:23

sbidoul reviewed Jul 26, 2019

View reviewed changes

sbejaoui force-pushed the 12.0-report_dispatch-sbj branch from ba07ee9 to 25fb2a0 Compare July 26, 2019 14:19

sbidoul mentioned this pull request Aug 6, 2019

[11.0][ADD] ir_actions_report_multi_company OCA/multi-company#80

Merged

astirpe suggested changes Aug 6, 2019

View reviewed changes

sbejaoui requested review from etobella and astirpe October 1, 2019 13:39

rousseldenis approved these changes Oct 1, 2019

View reviewed changes

astirpe approved these changes Oct 1, 2019

View reviewed changes

etobella reviewed Oct 1, 2019

View reviewed changes

sbejaoui added 10 commits October 1, 2019 23:52

[ADD] - report substitute

cd0c960

This addon give the possibility to substitute a report action by another based on some criteria.

[ADD] - Add readme

255cf1d

[ADD] - Add unit tests

6b658a6

[FIX] - missing method param

9f2f1b4

[FIX] - prevent substitution loop

590377e

[IMP] - manage subtitute report for multi-action

ea774aa

[IMP] - manage substitution for mail.thread message_post_with_template

ed669b0

[IMP] - add unit tests

1393f84

[FIX] - manage the case where mail.compose.message is triggered from …

9bf090a

…the chatter

[FIX] - Fix typo

103aecb

[12.0][IMP] - Add check for substitution infinite loop

2eb9253

sbejaoui force-pushed the 12.0-report_dispatch-sbj branch from caa970b to 2eb9253 Compare October 1, 2019 21:52

OCA-git-bot added a commit that referenced this pull request Oct 21, 2019

Merge PR #307 into 12.0

ab0fa7f

Signed-off-by sbidoul

OCA-git-bot merged commit 2eb9253 into OCA:12.0 Oct 21, 2019

OCA-git-bot added the merged 🎉 label Oct 21, 2019

sbidoul deleted the 12.0-report_dispatch-sbj branch October 26, 2019 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12.0][ADD] - New module: report substitute #307

[12.0][ADD] - New module: report substitute #307

sbejaoui commented Jul 19, 2019 •

edited

Loading

This comment has been minimized.

etobella left a comment

etobella Jul 22, 2019

sbejaoui Jul 22, 2019

sbidoul Jul 23, 2019 •

edited

Loading

sbidoul commented Jul 23, 2019

sbidoul commented Aug 6, 2019

astirpe Aug 6, 2019

sbejaoui Aug 7, 2019

sbejaoui Aug 7, 2019

astirpe Aug 6, 2019

sbejaoui Aug 7, 2019

astirpe Aug 6, 2019

sbejaoui Aug 7, 2019 •

edited

Loading

astirpe Aug 9, 2019

astirpe Aug 6, 2019

sbejaoui Aug 7, 2019

rousseldenis Oct 1, 2019

rousseldenis left a comment

rousseldenis Oct 1, 2019

rousseldenis commented Oct 1, 2019

etobella left a comment

etobella Oct 1, 2019

sbejaoui Oct 1, 2019

sbidoul commented Oct 21, 2019

OCA-git-bot commented Oct 21, 2019

OCA-git-bot commented Oct 21, 2019

[12.0][ADD] - New module: report substitute #307

[12.0][ADD] - New module: report substitute #307

Conversation

sbejaoui commented Jul 19, 2019 • edited Loading

This comment has been minimized.

etobella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

sbidoul commented Jul 23, 2019

sbidoul commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbejaoui Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousseldenis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousseldenis commented Oct 1, 2019

etobella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul commented Oct 21, 2019

OCA-git-bot commented Oct 21, 2019

OCA-git-bot commented Oct 21, 2019

sbejaoui commented Jul 19, 2019 •

edited

Loading

sbidoul Jul 23, 2019 •

edited

Loading

sbejaoui Aug 7, 2019 •

edited

Loading