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

[FIX] web: show page number in report #30095

Closed
wants to merge 1 commit into
base: 12.0
from

Conversation

Projects
None yet
3 participants
@nle-odoo
Copy link
Contributor

nle-odoo commented Jan 10, 2019

Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, we only check the condition that we require to only
remove page number when report is html embedded on portal.

opw-1924729

@nle-odoo nle-odoo added the OE label Jan 10, 2019

@nle-odoo nle-odoo force-pushed the odoo-dev:12.0-web-opw-1924729-nle branch from 45f3cb1 to 42f2a22 Jan 10, 2019

nle-odoo added a commit to odoo-dev/odoo that referenced this pull request Jan 10, 2019

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, we only check the condition that we require to only
remove page number when report is html embedded on portal.

opw-1924729
closes odoo#30095

@nle-odoo nle-odoo force-pushed the odoo-dev:12.0-web-opw-1924729-nle branch from 42f2a22 to faff13c Jan 10, 2019

nle-odoo added a commit to odoo-dev/odoo that referenced this pull request Jan 10, 2019

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, we only show page number when it is used (ie. when
rendering PDF).

opw-1924729
closes odoo#30095

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 10, 2019

@nle-odoo nle-odoo force-pushed the odoo-dev:12.0-web-opw-1924729-nle branch from faff13c to 07822d7 Jan 11, 2019

nle-odoo added a commit to odoo-dev/odoo that referenced this pull request Jan 11, 2019

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, we only show page number when it is used (ie. when
rendering PDF).

opw-1924729
closes odoo#30095

@robodoo robodoo removed the CI 🤖 label Jan 11, 2019

@nle-odoo nle-odoo requested a review from seb-odoo Jan 11, 2019

@nle-odoo

This comment has been minimized.

Copy link
Contributor

nle-odoo commented Jan 11, 2019

replacing report_type != 'html' instead of report_type == 'pdf' fixed the issue on the portal, but in the backend html printing would display "Page of " without numbers.

I am checking if it would not be better to always pass variable "report_type".

@robodoo robodoo added the CI 🤖 label Jan 11, 2019

@nle-odoo nle-odoo force-pushed the odoo-dev:12.0-web-opw-1924729-nle branch from 07822d7 to e3d304b Jan 11, 2019

nle-odoo added a commit to odoo-dev/odoo that referenced this pull request Jan 11, 2019

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, the report alway know if he is pdf, html or text
and only print the page number if it is of type pdf.

opw-1924729
closes odoo#30095

@robodoo robodoo removed the CI 🤖 label Jan 11, 2019

@seb-odoo

This comment has been minimized.

Copy link
Contributor

seb-odoo commented Jan 11, 2019

Do you want me to look the PR as it is, or are you still working on it?

@nle-odoo

This comment has been minimized.

Copy link
Contributor

nle-odoo commented Jan 11, 2019

It should be ready, both solutions works in portal and in normal report in backend (not showing page number in html, text, showing it in pdf):

  • the current version adding the report_type in all instances (so in backend it is present): e3d304b
  • the previous version where page number is only shown when page number is set: 07822d7

Thanks )

@seb-odoo

This comment has been minimized.

Copy link
Contributor

seb-odoo commented Jan 11, 2019

New version looks much better, I'll test and let you know.

Why do you set the default so late in the pdf method? I think it might be better if the html fallbacks were also rendered as if it was a "report_type pdf", because that's what was asked to render after all.

@nle-odoo

This comment has been minimized.

Copy link
Contributor

nle-odoo commented Jan 11, 2019

Well, my understanding was that if we returned an html instead of a pdf we don't want to render it as pdf but html. Currently there is some differences (page number, style, ...) but if we display an html, having the style change for html display (or not having "Page of" without number) seem to make sense.

But it is the first time I work on report in 12.0 so maybe I am wrong and miss something.

@seb-odoo

This comment has been minimized.

Copy link
Contributor

seb-odoo commented Jan 11, 2019

If a fallback was displayed to the user in a normal flow, I would agree it shouldn't show page numbers etc.

That said I believe the fallbacks are only used by the tests, then the reasoning would be that the test should test what was requested as closely as possible (= rendering the template with the pdf condition).

There is debate on whether we should use report_type == 'html' or report_type != 'pdf' depending on the result we want for the other report types (currently there is text which I'm not even sure what it is supposed to do).

Other than that, I tested the commit and it looks good to me.

@seb-odoo
Copy link
Contributor

seb-odoo left a comment

LGTM, just the small comment about the fallback

@@ -674,6 +674,7 @@ def render_qweb_pdf(self, res_ids=None, data=None):
# bypassed
raise UserError(_("Unable to find Wkhtmltopdf on this system. The PDF can not be created."))

data.setdefault('report_type', 'pdf')

This comment has been minimized.

@seb-odoo

seb-odoo Jan 11, 2019

Contributor

I would put it at the top of the method as discussed.

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, the report alway know if he is pdf, html or text
and only print the page number if it is of type pdf.

opw-1924729
closes #30095

@nle-odoo nle-odoo force-pushed the odoo-dev:12.0-web-opw-1924729-nle branch from e3d304b to 533940c Jan 11, 2019

@nle-odoo

This comment has been minimized.

Copy link
Contributor

nle-odoo commented Jan 11, 2019

ok, thanks :)

done for setdefault in top of render_pdf, I also added if not data: data = {} on render_text and render_html

for the text as far as I can tell it just download the renderer template (as a .txt file) so the page number also has no meaning (i guess it is in the case where you have a report template, but it contains only text/xml and you don't want to render it in wkhtml to print it in pdf)

@robodoo robodoo added the CI 🤖 label Jan 11, 2019

seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Jan 11, 2019

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, the report alway know if he is pdf, html or text
and only print the page number if it is of type pdf.

opw-1924729
closes odoo#30095

seb-odoo added a commit to odoo-dev/odoo that referenced this pull request Jan 11, 2019

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, the report alway know if he is pdf, html or text
and only print the page number if it is of type pdf.

opw-1924729
closes odoo#30095
@nle-odoo

This comment has been minimized.

Copy link
Contributor

nle-odoo commented Jan 11, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Jan 11, 2019

robodoo pushed a commit that referenced this pull request Jan 11, 2019

[FIX] web: show page number in report
Following 4a3fd02 on the portal when displaying an invoice we may get
a report_type variable (with value 'html' or 'pdf') on the portal.

This was used to not display the page number when checking the report,
but when the report is printed in the backend this report_type was unset
so the page number was mistakenly removed too.

With this changeset, the report alway know if he is pdf, html or text
and only print the page number if it is of type pdf.

opw-1924729
closes #30095
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Jan 11, 2019

Merged, thanks!

@robodoo robodoo closed this Jan 11, 2019

@nle-odoo nle-odoo deleted the odoo-dev:12.0-web-opw-1924729-nle branch Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment