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.

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

[5148][ADD] account_move_delivery_invoice #2

Open
wants to merge 5 commits into
base: 16.0
Choose a base branch
from

Conversation

AungKoKoLin1997
Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 commented Jan 27, 2025

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-invoice_print_delivery_slip branch from e924b33 to f94f9cd Compare January 27, 2025 11:01
@AungKoKoLin1997 AungKoKoLin1997 marked this pull request as ready for review January 28, 2025 01:17
@AungKoKoLin1997 AungKoKoLin1997 changed the title [ADD] invoice_print_delivery_slip [5148][ADD] invoice_print_delivery_slip Jan 28, 2025
@AungKoKoLin1997
Copy link
Contributor Author

Should we propose this module to OCA?

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-invoice_print_delivery_slip branch from f94f9cd to 89d5c5c Compare January 30, 2025 02:29
@AungKoKoLin1997 AungKoKoLin1997 changed the title [5148][ADD] invoice_print_delivery_slip [5148][ADD] invoice_delivery_slip Jan 30, 2025
@AungKoKoLin1997
Copy link
Contributor Author

@kanda999 Please review my latest code. I was not on the right track and was stuck for a few hours.

@kanda999
Copy link

@AungKoKoLin1997
Okay I will check your code later.

Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

How about account_move_delivery_invoice for the module name?

There are two approaches for the report template:

Option 1. Inherit account.report_invoice_document and add necessary adjustments (the current approach)
Option 2. Copy account.report_invoice_document to define a new template, and add necessary adjustments

While Option 1 is reasonable when adjustments are limited and we can accept all changes in the upstream, the template may get cluttered and become hard to comprehend without looking closely at the origin template.

I'm inclined to think that Option 2 is better for this module.

@kanda999 What do you think?

@kanda999
Copy link

kanda999 commented Feb 4, 2025

I thought Option 2 would be better too.

@AungKoKoLin1997
Could you please follow up on this?

@AungKoKoLin1997 AungKoKoLin1997 changed the title [5148][ADD] invoice_delivery_slip [5148][ADD] account_move_delivery_invoice Feb 5, 2025
@AungKoKoLin1997
Copy link
Contributor Author

Could you please follow up on this?

@kanda999 Done!

@kanda999
Copy link

kanda999 commented Feb 7, 2025

I reviewed your code and removed the sections that were not necessary for the delivery slip.
Additionally, I added patterns such as in_refund as titles. This is because I am considering the possibility of creating this module to OCA's l10n-japan.
I thought it would be useful to have it ready for that purpose.

@AungKoKoLin1997
If you have any comments, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants