-
-
Notifications
You must be signed in to change notification settings - Fork 680
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][FIX] stock_picking_invoicing qty_invoiced value compute #1672
base: 14.0
Are you sure you want to change the base?
Conversation
11b405e
to
f45673f
Compare
@Christian-RB there are a problem about your PR but is not related direct about your code, the problem are the dependecy of Sale module, this was included indirect by the module stock_picking_invoice_link https://github.com/OCA/stock-logistics-workflow/blob/14.0/stock_picking_invoice_link/__manifest__.py#L20 , the main reason are modularization and isolation of code, because the main goal of this module are "create invoices directly from picking, without having to use Sale or Purchase Orders." https://github.com/OCA/account-invoicing/blob/14.0/stock_picking_invoicing/readme/DESCRIPTION.rst, (I thinking about propusing to remove it to avoid the direct dependency) to integrate this module with Sale or Purchase modules the better approuch should be create new modules sale_stock_picking_invoicing and purchase_stock_picking_invoicing. There are an example of how it can done in Brazilian Localization, where we have the case of the Invoice creation from Picking without relation with a Sale or Purchase Order ( by the Fiscal rules, even when the company move a product to different address, is not a Sale or Purchase case, the company has the Fiscal obligation to generate an Invoice/Fiscal Document ), but we also are using this module when we want to force the creation of the Invoice from the Picking that was created by a Sale or Purchase Order, you can see here https://github.com/OCA/l10n-brazil/tree/14.0/l10n_br_sale_stock and https://github.com/OCA/l10n-brazil/tree/14.0/l10n_br_purchase_stock . I have made a PR in this repo based in l10n_br_sale_stock module for v14 #1025 at that time I was think it just used by Brazilian Localization, so as I wrote in that PR we plan to propuse add this module in v16 and check if there are intest, but if other people has necessity to use this module, we can think and consider re-open the PR. But it's important to hear from you, and other people, about the use of the module and if we should to add it in v14 or waititng for v16, what do you think? |
f45673f
to
f31dac1
Compare
Hello @mbcosta. Thank you for taking the trouble to review the PR. I understand your point of view regarding the unnecessary dependency on the 'sale' module and that a separate module should be created to add this functionality. Despite this, I believe that separating the functionalities of an existing module (which surely has people using it) will lead to more problems than solutions. I think the most appropriate approach would be to separate them at the time of module migration, so that openupgrade can handle the necessary adjustments. Let me know what you think. |
@Christian-RB thank you for reply, I believe will be better to solve it with the module sale_stock_picking_invoicing, because Brazilian Localization use it for a while so the isolation, modularization and operation seems works as necessary, the only doubt in the PR was if people beyond Brazilian Localization has interest in the module and as you said "which surely has people using it" I'm reopen the PR and appreciate if you can review. There are a difference from your PR, it's was unnecessary changed the methods _get_invoiced and _get_invoice_qty because during the creation of the Invoice from Picking the sale_line_ids are informed https://github.com/akretion/account-invoicing/blob/14.0-NEW-sale_stock_picking_invoicing/sale_stock_picking_invoicing/wizards/stock_invoice_onshipping.py#L87 The module was made as reference Brazilian Localization in the implementation, so will be important the review of other people and cases to check if something need to be change to have a better compatible, if you find something please let me know. I'm looking to solve the problem of the tests in the PR. |
PR of sale_stock_picking_invoicing #1025 is Ready for Review |
Please @OCA/accounting-maintainers , don't merge this too quickly and read @mbcosta remarks and mine in #1025 . Indeed it would be much better if stock_picking_invoicing wouldn't carry a hard dependency to the sale module as @mbcosta explained and this PR would just make it harder to make it modular later... |
@mbcosta I believe something great could be migrate stock_picking_invoice_link to v17 making it fully independent from sale and create a new |
This PR fix the invoice qty compute on the sale when the invoice is created from the picking.