[9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0 #296

jbaudoux · 2017-01-31T12:24:37Z

Previous migration did not keep the business functionnality of the previous module. Link from invoice to picking must link only related delivery and not all deliveries in case of partial deivery.

The previous port of this module changed the purpose of the module that is to link invoice (and lines) to the related picking (moves) to allows to find out exactly what move is invoiced. This was broken as each invoice was linked to all deliveries of all related sales orders.

I reverted the code to the original functionality in v8.0 and performed again the migration to the v9.0.
From version 9.0, you invoice always from the sale order based on quantites to invoice. With this module, you can know exactly what delivery is in the invoice.

jbaudoux · 2017-01-31T12:25:37Z

This module fix the previous port here: #261

elicoidal · 2017-02-01T07:23:55Z

Can you recheck Travis status?

jbaudoux · 2017-02-01T07:30:42Z

@elicoidal I'm trying to get the test working which proves that the module does what is expected. That's why I kept the PR in WIP for now. I'll remove the WIP status and notify when it's working.

elicoidal

Some details on README and an important question on the SQL usage

elicoidal · 2017-02-01T07:25:45Z

stock_picking_invoice_link/README.rst

@@ -44,6 +42,7 @@ Contributors
 * Unai Alkorta
 * Iñaki Zabala
 * Oihane Crucelaegui <[email protected]>
+* Jacques-Etienne Baudoux <[email protected]>

 Maintainer
 ----------


Please update below to the newest README template

elicoidal · 2017-02-01T07:27:23Z

stock_picking_invoice_link/models/account_invoice.py

+        string='Related Pickings', readonly=True,
+        copy=False,
+        help="Related pickings "
+             "(only when the invoice has been generated from the picking).")


s/picking/pickings

elicoidal · 2017-02-01T07:27:36Z

stock_picking_invoice_link/models/account_invoice.py

+        comodel_name='stock.move', inverse_name='invoice_line_id',
+        string='Related Stock Moves', readonly=True,
+        help="Related stock moves "
+             "(only when the invoice has been generated from the picking).")


s/picking/pickings

elicoidal · 2017-02-01T07:28:09Z

stock_picking_invoice_link/models/sale_order.py

+    @api.multi
+    def _prepare_invoice_line(self, qty):
+        vals = super(SaleOrderLine, self)._prepare_invoice_line(qty)
+        # move_ids = self.procurement_ids.mapped('move_ids').filtered(


please remove dead code

elicoidal · 2017-02-01T07:30:03Z

stock_picking_invoice_link/models/sale_order.py

+        #     x.location_dest_id.usage == 'customer').ids
+
+        # For performance reason, we compute the list of move in SQL
+        self._cr.execute("""


I raise a strong warning here against this SQL statement!
This is bypassing all ACL (customer, multi-company etc.)
@pedrobaeza

Yes, I'm sure this is not really needed to be in SQL. In previous version it doesn't.

jbaudoux · 2017-02-01T07:49:13Z

Thanks for you comments. I'm not sure of the best approach to keep performance if you generate invoices end of month for thousands of sales orders.

pedrobaeza · 2017-02-01T07:52:09Z

Well, the approach is always to make it work first, and then see performance bottlenecks to try to improve.

jbaudoux · 2017-02-02T23:12:49Z

@elicoidal @pedrobaeza Module is now working properly. I removed WIP status.

elicoidal · 2017-02-03T06:22:14Z

some small details left on the README file. Other than that LGTM

jbaudoux · 2017-02-10T10:15:27Z

@elicoidal Can you explain what should be improved in the README file, I can see anything missing or wrong?

elicoidal · 2017-02-10T13:07:45Z

one URL AFAICS.
Good job!

jjscarafia · 2017-04-03T11:50:07Z

If I understand this right, this will break actual behavior and will only works for newly created pickings.

A nice to have would be a post install script that could be used also in a upgrade script for actual v9 module that, at least, build a link for all invoices (actual v9 behavior).

jjscarafia · 2017-04-03T11:51:31Z

stock_picking_invoice_link/models/account_invoice.py

+        comodel_name='stock.picking', string='Related Pickings', readonly=True,
+        copy=False,
+        help="Related pickings "
+             "(only when the invoice has been generated from the picking).")


I thinks this sould be "help="Related pickings " (only when the invoice has been generated from a sale order)".

jjscarafia · 2017-04-03T11:51:37Z

stock_picking_invoice_link/models/account_invoice.py

+        readonly=True,
+        copy=False,
+        help="Related stock moves "
+             "(only when the invoice has been generated from the picking).")


I thinks this sould be "help="Related pickings " (only when the invoice has been generated from a sale order)".

jjscarafia · 2017-04-03T11:52:36Z

stock_picking_invoice_link/views/stock_view.xml

+                        groups="account.group_account_invoice"
+                        attrs="{'invisible':[('invoice_ids', '=', False)]}"
+                        widget="many2many_tags"
+                        context="{'tree_view_ref': 'account.invoice_tree', 'form_view_ref': 'invoice_form'}"/>


As explain in this pr https://github.com/OCA/stock-logistics-workflow/pull/319/files , full reference is required so customer invoice form is correctly displayed. Use "stock_picking_invoice_link.invoice_form" instead of "invoice_form".

…ration did not keep the business functionnality of the previous module. Link from invoice to picking must link only related delivery and not all deliveries in case of partial deivery

eLBati · 2017-04-10T08:40:31Z

Hello @jbaudoux I can't reproduce the problem:

Created a SO with 2 lines (2 products with Invoicing Policy = Delivered quantities)
Delivered 1 line and created back order
Invoiced the done delivery order

Tab 'pickings' of the invoice contains only 1 picking

Completely transferred the back order

Tab 'pickings' of the invoice contains only 1 picking

From the SO, Invoiced the remaining quantities

Both invoices, in 'pickings' tab, contain only 1 picking

eLBati · 2017-04-10T09:34:48Z

@jbaudoux I looked at the tests and reproduced manually:

Created a SO with 2 products (with Invoicing Policy = Delivered quantities) and quantities = 2
Partially delivered the picking (quantity = 1 for both lines)
Created invoice

'pickings' tab of the invoice has 2 pickings, while it should only contain the 'done' one.

I'm going to review the changes now.

Thanks

eLBati

Thanks

eLBati · 2017-04-10T10:41:42Z

@jjscarafia Fields account.invoice.picking_ids and account.invoice.line.move_line_ids will be empty after updating to this new version of the module, in existing DBs.

I suspect they can't be correctly computed, as the information is lost at the moment of invoice creation (see methods invoice_line_create and _prepare_invoice_line of sale.order.line).
@jbaudoux am I right?

We could write it in README, explaining how to retrieve the old (in some cases wrong) values, using the old version of the fields:

class AccountInvoice(models.Model):
    _inherit = "account.invoice"

    old_version_picking_ids = fields.Many2many(
        comodel_name='stock.picking', string='Related Pickings (old version)',
        compute="_compute_picking_ids")

    @api.multi
    def _compute_picking_ids(self):
        for invoice in self:
            invoice.picking_ids = self.env['stock.picking']
            for line in invoice.invoice_line_ids:
                invoice.picking_ids |= line.move_line_ids.mapped('picking_id')


class AccountInvoiceLine(models.Model):
    _inherit = "account.invoice.line"

    old_version_move_line_ids = fields.Many2many(
        comodel_name='stock.move', string='Related Stock Moves (old version)',
        compute="_compute_move_line_ids")

    @api.multi
    def _compute_move_line_ids(self):
        for line in self:
            line.move_line_ids = self.env['stock.move']
            for sale_line in line.sale_line_ids:
                for proc in sale_line.procurement_ids:
                    line.move_line_ids |= proc.move_ids

What do you think?

…#296) Previous migration did not keep the business functionality of the previous module. Link from invoice to picking must link only related delivery and not all deliveries in case of partial delivery

jbaudoux changed the title ~~[9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0.~~ [9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0. - WIP Jan 31, 2017

elicoidal suggested changes Feb 1, 2017

View reviewed changes

jbaudoux changed the title ~~[9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0. - WIP~~ [9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0 Feb 2, 2017

elicoidal approved these changes Feb 10, 2017

View reviewed changes

pedrobaeza mentioned this pull request Apr 1, 2017

FIX link invoice view on pickings #319

Merged

jjscarafia suggested changes Apr 3, 2017

View reviewed changes

jbaudoux added 9 commits April 4, 2017 14:43

[FIX] stock_picking_invoice_link: redo migration to 9.0. Previous mig…

7cf0aac

…ration did not keep the business functionnality of the previous module. Link from invoice to picking must link only related delivery and not all deliveries in case of partial deivery

[FIX] test

6eb463c

[FIX] test

196e5e4

[FIX] redo migration

2bdcdb8

[IMP] README.rst

ff478c9

[FIX] flake8

35c58ce

[FIX] README.rst

9cec1dc

[FIX] README.rst

fe02bdb

[FIX] help and full reference to invoice view loaded in action

44f7bcf

jbaudoux force-pushed the 9.0-fixmig-stock_picking_invoice_link branch from 50ec010 to 44f7bcf Compare April 4, 2017 12:44

eLBati approved these changes Apr 10, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0 #296

[9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0 #296

jbaudoux commented Jan 31, 2017

jbaudoux commented Jan 31, 2017 •

edited

Loading

elicoidal commented Feb 1, 2017

jbaudoux commented Feb 1, 2017

elicoidal left a comment

elicoidal Feb 1, 2017

elicoidal Feb 1, 2017

elicoidal Feb 1, 2017

elicoidal Feb 1, 2017

elicoidal Feb 1, 2017

pedrobaeza Feb 1, 2017

jbaudoux commented Feb 1, 2017

pedrobaeza commented Feb 1, 2017

jbaudoux commented Feb 2, 2017

elicoidal commented Feb 3, 2017

jbaudoux commented Feb 10, 2017

elicoidal commented Feb 10, 2017

jjscarafia commented Apr 3, 2017

jjscarafia Apr 3, 2017

jjscarafia Apr 3, 2017

jjscarafia Apr 3, 2017

eLBati commented Apr 10, 2017

eLBati commented Apr 10, 2017

eLBati left a comment

eLBati commented Apr 10, 2017

[9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0 #296

[9.0] stock_picking_invoice_link: redo migration from 8.0 to 9.0 #296

Conversation

jbaudoux commented Jan 31, 2017

jbaudoux commented Jan 31, 2017 • edited Loading

elicoidal commented Feb 1, 2017

jbaudoux commented Feb 1, 2017

elicoidal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaudoux commented Feb 1, 2017

pedrobaeza commented Feb 1, 2017

jbaudoux commented Feb 2, 2017

elicoidal commented Feb 3, 2017

jbaudoux commented Feb 10, 2017

elicoidal commented Feb 10, 2017

jjscarafia commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eLBati commented Apr 10, 2017

eLBati commented Apr 10, 2017

eLBati left a comment

Choose a reason for hiding this comment

eLBati commented Apr 10, 2017

jbaudoux commented Jan 31, 2017 •

edited

Loading