Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[14.0] [ADD] new module account_receipt_print #849

Merged
merged 1 commit into from
Jul 15, 2022
Merged

[14.0] [ADD] new module account_receipt_print #849

merged 1 commit into from
Jul 15, 2022
+881 −0

Conversation

primes2h
Copy link
Contributor

This module allows printing of sale and purchase receipts.

@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 3 times, most recently from a71316a to f36abfc Compare January 24, 2021 22:29
marcelofrare
marcelofrare approved these changes Jan 25, 2021
View reviewed changes
Copy link

@marcelofrare marcelofrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works ok for me

@primes2h primes2h marked this pull request as draft February 5, 2021 09:24
@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 3 times, most recently from 249fa5c to 799f62d Compare February 11, 2021 19:32
@primes2h primes2h marked this pull request as ready for review February 11, 2021 19:33
@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 2 times, most recently from 5f6a46c to adf37a2 Compare February 20, 2021 20:25
marcelofrare
marcelofrare approved these changes Feb 21, 2021
View reviewed changes
Copy link

@marcelofrare marcelofrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! more quickly!
Ok for me

@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 4 times, most recently from 6da1776 to 380a621 Compare February 27, 2021 19:20
This was referenced Feb 27, 2021
Closed
Closed
@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 2 times, most recently from ced5208 to 61d19c4 Compare March 4, 2021 15:31
@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch from 61d19c4 to 4edfbea Compare May 5, 2021 18:35
@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 3 times, most recently from 649d60c to b054768 Compare January 15, 2022 20:11
@primes2h primes2h mentioned this pull request Mar 24, 2022
Open
1 task
tafaRU
tafaRU requested changes May 5, 2022
View reviewed changes
Copy link
Member

@tafaRU tafaRU left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, trying the module I noticed that there are some problems with page break by using paperformat_euro_a5.

See below some examples:

image
image

Could you please have a look?

The rest LGTM, thanks a lot!

account_receipt_print/i18n/it.po Outdated Show resolved Hide resolved
account_receipt_print/report/a5_report.xml Outdated Show resolved Hide resolved
account_receipt_print/views/account_move_view.xml Outdated Show resolved Hide resolved
@enigmista91
Copy link

@primes2h will you fix the comment?

@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 2 times, most recently from b48d1c0 to ebd0199 Compare June 16, 2022 19:15
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

tafaRU
tafaRU requested changes Jul 11, 2022
View reviewed changes
Copy link
Member

@tafaRU tafaRU left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@primes2h is there any particular reason for creating report_receipt_document without inheriting from account.report_invoice_document (by setting primary mode)?

Similarly to what has been achieved in https://github.com/OCA/l10n-italy/blob/764df48a8334a4e3cfde71d9e339921de6b7d2d3/l10n_it_corrispettivi/views/account_report.xml#L24

@primes2h
Copy link
Contributor Author

@primes2h is there any particular reason for creating report_receipt_document without inheriting from account.report_invoice_document (by setting primary mode)?

Similarly to what has been achieved in https://github.com/OCA/l10n-italy/blob/764df48a8334a4e3cfde71d9e339921de6b7d2d3/l10n_it_corrispettivi/views/account_report.xml#L24

Good point.
My first idea needed specific report format (margins etc.) to fit into a5 size, so a custom report would have been more flexible.
After recent changes I think we could opt to inherit report with primary set as you pointed out.

@primes2h primes2h force-pushed the 14.0-add_receipt_printing branch 6 times, most recently from 7d59984 to 00fcdb1 Compare July 13, 2022 20:37
@tafaRU
Copy link
Member

tafaRU commented Jul 14, 2022 •
edited
Loading

@pedrobaeza I'm wondering about what name is more suited for this new module (see https://github.com/PNLUG/account-invoicing/blob/14.0-add_receipt_printing/account_receipt_print/README.rst for having its description) between these two: account_receipt_print and account_move_receipt_print. IMO it is the second one. What about you?
Could you please also explain the reason for your choice?

Thank you in advance.

@pedrobaeza
Copy link
Member

pedrobaeza commented Jul 14, 2022 •
edited
Loading

I would go for account_receipt_report. My logic behind this is to try to homogenize the nomenclature following this scheme:

area/app + subarea + function

Regards.

@tafaRU
Copy link
Member

tafaRU commented Jul 15, 2022 •
edited
Loading

@primes2h last thing: please rename the module to account_receipt_report according to #849 (comment)
Thanks!

@primes2h
Copy link
Contributor Author

primes2h commented Jul 15, 2022 •
edited
Loading

I would go for account_receipt_report.
My logic behind this is to try to homogenize the nomenclature following this scheme:
area/app + subarea + function

Did you mean account_receipt_print ? (first of two proposed by @tafaRU and current module name)
It fits right in the scheme, in fact the function is to print.

@primes2h
Copy link
Contributor Author

I would go for account_receipt_report.
My logic behind this is to try to homogenize the nomenclature following this scheme:
area/app + subarea + function

Did you mean account_receipt_print ? (first of two proposed by @tafaRU and current module name) It fits right in the scheme, in fact the function is to print.

@pedrobaeza

@pedrobaeza
Copy link
Member

report is because it's used in other modules:

https://odoo-community.org/shop/adr-products-report-5730
https://odoo-community.org/shop/valued-picking-linked-with-mrp-kits-5717
https://odoo-community.org/shop/stock-picking-product-barcode-report-5709
https://odoo-community.org/shop/stock-picking-report-undelivered-products-5629

but if the report exists and this only enables the print option, then go ahead with it.

@tafaRU
Copy link
Member

tafaRU commented Jul 15, 2022

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-849-by-tafaRU-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e29aae4 into OCA:14.0 Jul 15, 2022
@OCA-git-bot
Copy link
Contributor

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

@primes2h primes2h deleted the 14.0-add_receipt_printing branch July 15, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tafaRU tafaRU tafaRU requested changes

@enigmista91 enigmista91 enigmista91 approved these changes

@marcelofrare marcelofrare marcelofrare approved these changes

@matteoopenf matteoopenf matteoopenf approved these changes

Assignees
No one assigned
Labels
merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@primes2h @enigmista91 @matteoopenf @OCA-git-bot @tafaRU @pedrobaeza @marcelofrare