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

[13.0]]ADD] Add account_invoicing_mode #755

Merged
merged 9 commits into from
Aug 3, 2021
Merged

Conversation

TDu
Copy link
Member

@TDu TDu commented Jul 14, 2020

Add three modules helping on automatically invoicing customers.

The base module account_invoice_base_invoicing_mode does not actually
do anything but adds a selection field on partner to allow assigning an
invoicing mode to a customer.

The two other modules add specific invoicing mode.

The module account_invoice_mode_monthly creates monthly invoices for
customer on a specific day (configruation is in Accounting Settings)

The module account_invoice_mode_at_shipping creates invoices on the
shipping of the goods.

Those modules use queue_job to generate and validate invoices.

@TDu TDu force-pushed the invoicing-mode-13 branch 3 times, most recently from e463d8f to 14ff383 Compare July 15, 2020 16:36
Copy link
Member

@jgrandguillaume jgrandguillaume left a comment

Choose a reason for hiding this comment

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

Functional test, looks good. Thx for the job

Copy link

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

Some remarks (typo, refunds)

account_invoice_mode_monthly/data/ir_cron.xml Outdated Show resolved Hide resolved
account_invoice_mode_monthly/models/sale_order.py Outdated Show resolved Hide resolved
account_invoice_mode_monthly/models/sale_order.py Outdated Show resolved Hide resolved
@rvalyi rvalyi changed the title [13.0]]ADD] Add acconut_invoicing_mode [13.0]]ADD] Add account_invoicing_mode Jul 16, 2020
Copy link

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

account_invoice_mode_at_shipping/models/stock_picking.py Outdated Show resolved Hide resolved
account_invoice_mode_at_shipping/models/stock_picking.py Outdated Show resolved Hide resolved
@pedrobaeza
Copy link
Member

I think this module is not correctly focused by several reasons:

  • The aim is to invoice sales order, so the name and the repo are not good.
  • It's overlapping sale_automatic_workflow flow.

IMO, you should extend that module for adding the monthly/dayly mechanism, but using the same base methods.

@TDu
Copy link
Member Author

TDu commented Jul 22, 2021

In previous comments there has been a suggestion by @pedrobaeza to include this into sale_automatic_workflow because the use case is overlapping with it.
I don't think so myself, the monthly invoice mode is based on a daily cron, the option does not exist in sale_automatic_workflow and the at shipping invoicing is triggered by some specific code in the action_done of the stock_picking, which would not be easily implemented on top of sale_automatic_workflow.

Moreover the proposed change would also make this pr much more complex and maybe its UI element less intuitive to use.

And I personally think that this pr gives a good base to add different invoicing mode in the future.

@pedrobaeza
Copy link
Member

I don't remember right now my suggestion and the context, so I can't give you now an answer, but if you point me to it, I can agree or not on this.

@TDu
Copy link
Member Author

TDu commented Jul 22, 2021

@pedrobaeza It is about this comment #755 (comment) on the same PR.
We would like to get this merge and I felt your proposition still deserved an answer, yep that is one year ago.

Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

LG, a rebase to keep one commit / module is welcome

@pedrobaeza
Copy link
Member

OK, as you prefer then. I still encounter some common points:

  • sale_automatic_workflow also has a cron that performs the task of invoicing.
  • It contains the code to invoice.

What about the naming, being sale_invoicing_mode or similar?

@TDu
Copy link
Member Author

TDu commented Jul 22, 2021

OK, as you prefer then. I still encounter some common points:

* `sale_automatic_workflow` also has a cron that performs the task of invoicing.

* It contains the code to invoice.

What about the naming, being sale_invoicing_mode or similar?

Well the base module is just a field on the partner really, so it is about invoicing first. So it could be used to invoice other things than a sale order if the business model requires that.

I would really like to keep the naming like it is.

@pedrobaeza
Copy link
Member

What about partner_invoicing_mode then? If not, I won't block it.

@TDu
Copy link
Member Author

TDu commented Jul 22, 2021

What about partner_invoicing_mode then? If not, I won't block it.

I actually like that name better, shorter and more explicit.
Maybe it could be renamed during the 14.0 migration

TDu and others added 5 commits July 28, 2021 09:59
Add three modules helping on automatically invoicing customers.

The base module `account_invoice_base_invoicing_mode` does not actually
do anything but adds a selection field on partner to allow assigning an
invoicing mode to a customer. And a checkbox to choose regrouping
invoices.

The two other modules add specific invoicing mode.

The module `account_invoice_mode_monthly` creates monthly invoices for
customer on a specific day (configuration is in Accounting Settings)

The module `account_invoice_mode_at_shipping` creates invoices on the
shipping of the goods.

Those modules use queue_job to generate and validate invoices.
Sales order with different payment term are not grouped into the
same invoice anymore.
This grouping can be easily changed by overriding the coresponding
method.

During this change the parameter of the job generating the invoices
has changes and is now the sale_order_ids to process.
If we group several sales in the same procurement group, we'll invoice
only one sale order. Tracing back to the sales through the stock moves
ensure we invoice all of them.
Add few changes from reviews.
Use sudo to post the invoice.
@TDu TDu force-pushed the invoicing-mode-13 branch from 29d10d4 to 4b428b3 Compare July 28, 2021 08:15
@TDu
Copy link
Member Author

TDu commented Jul 28, 2021

@simahawk I squashed the commit and added the roadmap as discussed

@simahawk
Copy link
Contributor

@TDu check the build pls

@TDu TDu force-pushed the invoicing-mode-13 branch from 4b428b3 to f15e7de Compare July 29, 2021 02:24
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

@jbaudoux @mmequignon good for you?

class AccountMove(models.Model):
_inherit = "account.move"

@job(default_channel="root.invoice_validation")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the @job decorator deprecated?
OCA/queue#274

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is, but it still works for v13.
It will be changed on the v14 migration.

@simahawk
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 13.0-ocabot-merge-pr-755-by-simahawk-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 29, 2021
Signed-off-by simahawk
@simahawk
Copy link
Contributor

simahawk commented Aug 2, 2021

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-755-by-simahawk-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Aug 2, 2021
Signed-off-by simahawk
@OCA-git-bot
Copy link
Contributor

@simahawk your merge command was aborted due to failed check(s), which you can inspect on this commit of 13.0-ocabot-merge-pr-755-by-simahawk-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@simahawk
Copy link
Contributor

simahawk commented Aug 2, 2021

WTH?

image

@sbidoul
Copy link
Member

sbidoul commented Aug 2, 2021

If you click on the link in the error message, you'll see that travis is indeed red (a transient GitHub error). It is not the same commit as the one just above for a good reason I don't remember right now.

image

@simahawk
Copy link
Contributor

simahawk commented Aug 3, 2021

let's try again
/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-755-by-simahawk-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 2d754d3 into OCA:13.0 Aug 3, 2021
@OCA-git-bot
Copy link
Contributor

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

kv1612 pushed a commit to kv1612/partner-contact that referenced this pull request Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.