-
Notifications
You must be signed in to change notification settings - Fork 23.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IMP] account,*: check invoice report in separate function #147124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only a comment on your hook function
IMO, it's okay to add it in 16.0 but I'm not sure to understand why it's not necessary for 17.0 but for master?
Thanks @FlorianGilbert, it makes sense ! |
dae69ba
to
a89fecc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @remi-filament
It seems your runbot is red, could you please rebase your branch, it should fix it :)
Thank you
a89fecc
to
067b586
Compare
@FlorianGilbert done |
@@ -44,10 +44,13 @@ def _render_qweb_pdf_prepare_streams(self, report_ref, data, res_ids=None): | |||
} | |||
return collected_streams | |||
|
|||
def _is_invoice_report(self, report_ref): | |||
return True if self._get_report(report_ref).report_name in ('account.report_invoice_with_payments', 'account.report_invoice') else False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems your rebase changes this part of code for the original code you made. Could you please simplify as requested before? Thank you :)
*: account_edi, l10n_ch Co-authored-by: Florian Gilbert <flg@odoo.com>
067b586
to
31dd3a1
Compare
Sorry about that, I thought it was the other way around... Just fixed it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, thank you for your contribution 😄
@robodoo r+
*: account_edi, l10n_ch closes #147124 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Co-authored-by: Florian Gilbert <flg@odoo.com>
*: account_edi, l10n_ch closes odoo#147124 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Co-authored-by: Florian Gilbert <flg@odoo.com>
@remi-filament @FlorianGilbert this pull request has forward-port PRs awaiting action (not merged or closed): |
2 similar comments
@remi-filament @FlorianGilbert this pull request has forward-port PRs awaiting action (not merged or closed): |
@remi-filament @FlorianGilbert this pull request has forward-port PRs awaiting action (not merged or closed): |
1 similar comment
*: account_edi, l10n_ch closes odoo#147124 Signed-off-by: Florian Gilbert (flg) <flg@odoo.com> Co-authored-by: Florian Gilbert <flg@odoo.com>
*: account_edi, account_edi_ubl_cii, l10n_ch
Description of the issue/feature this PR addresses:
This PR moves the check whether requested report is an invoice one in a separate function so that it can be inherited in case we need to have extra invoice report formats.
Current behavior before PR:
In case you generate new invoice reports, EDI document will not be included unless you rewrite (or copy/update) the full code of _render_qweb_pdf() and _render_qweb_pdf_prepare_streams() functions.
Desired behavior after PR is merged:
With this PR, you can only inherits _is_invoice_report() function to add your new report name.
Since this is a IMP PR, I am not sure it would be OK to have it in 16.0 (which is where I would need the functionality for now) ? So I made this PR on v16.0 but also another one on master : #147120 (since in 17.0 and master account_edi_ubl_cii does not need update there)
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr