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

[14.0][ADD] Module sale_stock_picking_invoicing #1025

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

mbcosta
Copy link
Contributor

@mbcosta mbcosta commented Oct 19, 2021

Module sale_stock_picking_invoicing, based in https://github.com/OCA/l10n-brazil/tree/12.0/l10n_br_sale_stock , depends #1014.

This module extends Stock Picking Invoicing implementation to Sale, you can define the Sale Creation Invoice Policy, if the invoice should create from Sale Order or Stock Picking, in this case, the information used at invoice come from Sale Order e.g.: Price Unit, Payment Terms, and others fields. Similar feature exist in v8 core.

Implementation questions when "Sale Creation Invoice Policy" is "Stock Picking" :

  • When Products of Sale Order has lines where "Product Type" is "Storable Product" the button "Create Invoice" in Sale Order become invisible even when Product "Invoice Policy" defined as "Ordered quantities", is this right? Or it should be possible create invoice from Sale Order in this case? My undestand for now is as "Sale Creation Invoice Policy" is general rule should be has priority over the "Invoice Policy" defined in each Product.

  • There are two similar buttons to create invoice from Sale Order, one just appear in status Sale the other appear in all status but will be open with Down Payments option selected. Should be keep the second button to allow Down Paymens?

  • In the case of Sale Order has lines with Product Type "Storable Product" and "Service" will be create two separated Invoices, one created from Sale Order an other from Stock Picking, should be try to join and create only one? Or it should be a Parameter in the system?

As said this module is based in l10n_br_sale_stock, by this reason I like to ask if the License Header and the CONTRIBUTORS.MD should be keeped as the same based files?

It is possible reference mutiple sale lines in only one invoice line https://github.com/odoo/odoo/blob/14.0/addons/sale/models/account_move.py#L34, but there is a error when Posted Invoice:
Issue odoo/odoo#77028
PR with FIX odoo/odoo#77195
By this reason today the module not allow grouping stock moves when has sale_line_id, the PR above solve the problem, waiting merge to make useful here.

cc @renatonlima @rvalyi @mileo @gabrielcardoso21 @marcelsavegnago

@kittiu kittiu changed the title [14.0][NEW] Module sale_stock_picking_invoicing [14.0][ADD] Module sale_stock_picking_invoicing Nov 5, 2021
@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch 3 times, most recently from 27e6185 to beec094 Compare November 12, 2021 21:57
@mbcosta
Copy link
Contributor Author

mbcosta commented Sep 15, 2022

Thanks @kittiu by your review, I'm close this PR because in my view it's important confirm with the mainteners of the repo if there are interest in include this functionality here and I think we lost the "window" to made this extraction from Brazilian Localization and will be better check this case in the future v16.

@mbcosta
Copy link
Contributor Author

mbcosta commented Mar 12, 2024

Reopen because as talked in #1672 the question about if this implementation would has interest outside Brazilian Localization project was answered, and to keep the isolation and modularization of stock_picking_invoicing will be better another module to integrate the functionality of create Invoice from Picking when it's linked to a Sale Order, some points about the implementation:

image

So for keep compatible with the Brazilian Localization I think will be better check this possibility to Grouping Sale Lines in the future in another PR or in versions v16, v17 or the newest.

The review and tests of people outside Brazilian Localization are important to know if there are any change that can make the module more compatible with another cases of use.

cc @renatonlima @rvalyi @mileo @gabrielcardoso21 @marcelsavegnago @kittiu @Christian-RB

@mbcosta
Copy link
Contributor Author

mbcosta commented Mar 26, 2024

About important changes made in the last two commits:

  • Get fields from Sale methods to prepare invoice and invoice lines to avoid the necessity of 'glue' modules and make the Invoice created by Picking equals to created by Sale.

Other modules can included new fields in Sale Order and include this fields in the dict of creation Invoice from sale, for example:

account_payment_sale https://github.com/OCA/bank-payment/blob/14.0/account_payment_sale/models/sale_order.py#L41
sale_commission https://github.com/OCA/commission/blob/14.0/sale_commission/models/sale_order.py#L64

Example with demo data, account_payment_sale and sale_commission modules installed:

image

image

image

image

image

image

image

  • Make module compatible with 'Down Payments' case.
    Initially to force the creation of Invoice from Stock Picking when the "Sale Create Invoice Policy" was as stock_picking, the implementation turned the button 'Create Invoice' in Sale Order view invisible, but this block the case of "Down Payments", now in this case the _get_invoiceable_lines method just return the Products with Type Service and if there is no Service raise user error. Example with demo data:

image

image

image

image

image

image

image

Case where "Sale Create Invoice Policy" defined as stock_picking

image

Create made in Stock Picking

image

image

In order to avoid errors with the tests made by CI when the enviroment are Demo the default "Sale Create Invoice Policy" defined as "Sale Order", so for test this case you need to change in Company view or Sale Settings for "Stock Picking"

image

The commits are separated to let brazilian community check this changes in a easy way, but if necessary I can merge all commits and let only one.

@rvalyi
Copy link
Member

rvalyi commented Mar 27, 2024

thank you for the hard work @mbcosta I promiss I'll look at that soon. If I forget please ping me again. We have to take the opportunity to modularize stock_invoice_picking indeed...

Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

@mbcosta I would say it's a bit late for us to focus on v14 unfortunately... I mean the module is quite large and we are not going to sell any v14 project anymore for instance nor migrate customer to v14 and I think people on v14 can work their way the way it is now in the Brazilian localization...
Would it be possible to try to make the equivalent PR on v16? I know this might be some work... But honestly it has more chance to enter in v16 than v14 that is already being phased out...

This module depends on:

* sale_management
* stock_picking_invoicing
Copy link
Member

Choose a reason for hiding this comment

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

not the same list as in the manifest

"version": "14.0.1.0.0",
"maintainers": ["mbcosta", "renatonlima"],
"depends": [
"sale_management",
Copy link
Member

Choose a reason for hiding this comment

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

are you sure sale_management is required? If yes could you please just explain why? (and eventually leave a short comment why here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Necessary to access Sale Menu
image

image

By some reason the sale module create menuitem with attribute active="False" https://github.com/OCA/OCB/blob/14.0/addons/sale/views/sale_views.xml#L8 and sale_management change for active="True" https://github.com/OCA/OCB/blob/14.0/addons/sale_management/views/sale_management_views.xml#L4

@rvalyi
Copy link
Member

rvalyi commented Mar 27, 2024

I understand you re-opened this because #1672 is for 14.0 ...
Well that sucks... Cause as you explain, if we merge #1672 we would make it even harder to make things modular in the future so merging it is bad. I'll make my best to find time and see if we can get this one merged then.

Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

I reviewed a part of the module (all except the wirzard) and I suggest a few English changes. As for the wizard I'll review later, but eventually that's cool if you could check if all the comments are still required or if some might be dropped.

Actually the module isn't that large, I'm confident we can get it merged in 14.0

cc @marcelsavegnago @antoniospneto @renatonlima

result = "sale_order"
return result

sale_create_invoice_policy = fields.Selection(
Copy link
Member

Choose a reason for hiding this comment

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

sale_invoicing_policy would be a much better name

("sale_order", "Sale Order"),
("stock_picking", "Stock Picking"),
],
string="Sale Create Invoice Policy",
Copy link
Member

Choose a reason for hiding this comment

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

change the variable name and drop the string

],
string="Sale Create Invoice Policy",
help="Define, when Product Type are not service, if Invoice"
" should be create from Sale Order or Stock Picking.",
Copy link
Member

Choose a reason for hiding this comment

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

s/be create/be created/
s/or Stock Picking/or from StockPicking/

string="Sale Create Invoice Policy",
help="Define, when Product Type are not service, if Invoice"
" should be create from Sale Order or Stock Picking.",
default=_default_sale_create_invoice_policy,
Copy link
Member

Choose a reason for hiding this comment

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

change to default_sale_invoicing_policy

class ResConfigSettings(models.TransientModel):
_inherit = "res.config.settings"

sale_create_invoice_policy = fields.Selection(
Copy link
Member

Choose a reason for hiding this comment

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

change to sale_invoicing_policy

# Case only Products Type 'product'
raise UserError(
_(
"When 'Sale Create Invoice Policy' is defined as"
Copy link
Member

Choose a reason for hiding this comment

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

change to When 'Sale Invoicing Policy' is defined...

@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch from 1e1c624 to 1e2bedf Compare March 28, 2024 17:04
@mbcosta
Copy link
Contributor Author

mbcosta commented Mar 28, 2024

Thanks @rvalyi for review, I made the changes, about the wizards it's important to check if the Refund case should not get values from "_prepare_invoice" and "_prepare_invoice_lines" of Sale module as I did, and in the _create_invoice method https://github.com/akretion/account-invoicing/blob/14.0-NEW-sale_stock_picking_invoicing/sale_stock_picking_invoicing/wizards/stock_invoice_onshipping.py#L149 I use as reference the original Sale method https://github.com/OCA/OCB/blob/14.0/addons/sale/models/sale.py#L681 to get and include the Down Payments.

raise UserError(
_(
"When 'Sale Invoicing Policy' is defined as"
"'Stock Picking' the Invoice only can be created"
Copy link
Member

Choose a reason for hiding this comment

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

s/only can be/can only be/

"When 'Sale Invoicing Policy' is defined as"
"'Stock Picking' the Invoice only can be created"
" from Stock Picking, if necessary you can change"
" this in Company or Sale Settings."
Copy link
Member

Choose a reason for hiding this comment

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

s/from Stock Picking/from the Stock Picking/
s/change this in Company/in the Company/

lines = new_lines
else:
# Case only Products Type 'product'
raise UserError(
Copy link
Member

Choose a reason for hiding this comment

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

Could you just continue and don't add the product?

How we gonna deal with this example?

image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mileo for review, the UserError just appear when there are only Lines with Product Type 'product' if there are any Service line the Invoice will be create, it's a way to inform the user about the "Sale Invoicing Policy" ( similar to the message when Product must be Delivered to be able Create the Invoice ), in the case of Services the 'product' must be defined as Type 'service'

image

Demo data has a Sale Order of the case of Product and Service, for now in this case will be create two Invoices, one with the Service created from Sale Order and the other from Picking,

image

image

Service Invoice

image

Try to create Invoice from Sale Order, there lines but the policy chosen are Stock Picking

image

Create from Stock Picking

image

image

image

image

This is the expected behavior in brazilian localization, but I think this can be analise, debate and if necessary change, for example:

  • If Policy are Stock Picking create only one Invoice from Picking( get Services Lines in the same way of 'Down Payments' )
  • If there are Product and Service lines ignore the "Sale Invoicing Policy" and allow Create from Sale Order
  • Other ideas

@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch from 1e2bedf to 092aa34 Compare April 3, 2024 22:12
@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch 2 times, most recently from 9e5995e to e1528f2 Compare August 23, 2024 15:00
@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch from e1528f2 to 9fa9973 Compare October 22, 2024 20:59
@mbcosta
Copy link
Contributor Author

mbcosta commented Oct 22, 2024

Force-Push to remove file sale_stock_picking_invoicing/models/stock_rule.py, because it should be in stock_picking_invocing as suggest in the above Review, by this reason the CI tests fail here so this PR now depends of PR #1782 ( already with two Approves )

@rvalyi
Copy link
Member

rvalyi commented Oct 23, 2024

@mbcosta #1782 was merged. I also re-launched the failed tests as the date pricelist issue in the CI seems fixed.

@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch from 9fa9973 to 4fefe3a Compare October 23, 2024 19:27
@mbcosta
Copy link
Contributor Author

mbcosta commented Oct 23, 2024

Thanks @rvalyi with rebase the PR back to be green, I took a time to check the PR again to be sure everything work as well with Extract PR of l10n_br_sale_stock


def _get_partner_to_invoice(self):
partner_id = super()._get_partner_to_invoice()
partner = self.env["res.partner"].browse(partner_id)
Copy link
Member

@rvalyi rvalyi Oct 23, 2024

Choose a reason for hiding this comment

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

you can avoid this browse here if you compare partner_id with self.sale_id.partner_invoice_id.id directly 2 lines below in if partner != self.sale_id.partner_invoice_id:
(partner_invoice_id is required so no need to check it's not False/None)

@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch from 4fefe3a to d536bbe Compare October 23, 2024 21:19
partner_id = super()._get_partner_to_invoice()
if self.sale_id:
if partner_id != self.sale_id.partner_invoice_id.id:
partner_id = self.sale_id.partner_invoice_id.id
Copy link
Member

Choose a reason for hiding this comment

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

in fact, you can simplify further to just:

        if self.sale_id:
            partner_id = self.sale_id.partner_invoice_id.id

if it's the same it will affect partner_id but it won't hurt performance and the code will be simpler

or eventually something like:

partner_id = self.sale_id.partner_invoice_id.id if self.sale_id else super()._get_partner_to_invoice()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, the error in the tests is the problem we saw yesterday the Tests of module account_invoice_pricelist fail at a specific Date Time

Copy link
Member

Choose a reason for hiding this comment

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

OK, the PR seems OK now. I will wait a few hours, trigger the tests again and approve once everything is green again. Thank you for the awesome work and sorry for the delay. Would be great if you could port it to 16.0 as well... (you can skip v15)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't saw your comment and just make a force push, I have tried to find where is the problem but not success, the clue is the problem related with Date Time, after UTC midnight the tests back to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rvalyi I decide to make v15 migration #1826 just to avoid the possibility of a problem for the cases of someone by some reason still using v15, after merge I'll port to v16

Copy link
Member

Choose a reason for hiding this comment

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

Ok, perfect

@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch from d536bbe to 065bfb8 Compare October 23, 2024 22:05
@mbcosta mbcosta force-pushed the 14.0-NEW-sale_stock_picking_invoicing branch from 065bfb8 to 5fca51a Compare October 24, 2024 00:06
Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

OK for me.

@mbcosta
Copy link
Contributor Author

mbcosta commented Oct 24, 2024

Thanks @rvalyi for your review, no problems for delay, this was important to make a better implementation

Copy link
Contributor

@antoniospneto antoniospneto left a comment

Choose a reason for hiding this comment

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

LGTM

@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). 🤖

@rvalyi
Copy link
Member

rvalyi commented Oct 24, 2024

and finally...

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-1025-by-rvalyi-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0050187 into OCA:14.0 Oct 24, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

7 participants