Merge lp:~camptocamp/new-report-intrastat/7.0-post-all-errors-rather-than-one-by-one into lp:new-report-intrastat

[Guewen Baconnier @ Camptocamp]

Post a message with all errors rather than raising them one by one.

In l10n_fr_intrastat_product:

When:
When we generate DEB lines and several lines have errors (misconfiguration of products, partners, ...).

Current behavior:
The first error is raised so the user must correct it and launch again the generation, and repeat this cycle until all errors are fixed.

Proposal:
Accumulate all the errors and show them in a mail message at the bottom of the view.

[Alexis de Lattre (alexis-via)]

I merged this earlier, but I found a regression tonight and I reverted the commit.

There is a regression in this MP :
the modifications l 235 and 269 are wrong : putting the keys in the parent_values dict to False should only be made when parent_values['is_fiscal_only'] == True, that's why it was inside the "else"... and you removed the "else" !

It may not be the only regression ; I think there is the same kind of thing on line 199.

You made some modifications to the "functionnal code" (it seems to me that you thought it was to clean-it-up) that are not linked to the error messaging system, but it causes some issues.

[Alexis de Lattre (alexis-via)]

review: Needs Fixing

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Sorry for the regression.
I did not changed that for code cleaning. The problem when initializing the keys in the 'else' was that we could have KeyError when they are accessed later, that's why I initialized them before the if.
I will try to find another way to do that.

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

I reverted the change for the keys that were already initialized earlier in the process. I left the initialization of the keys that were never set before.

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

I completely checked when the keys are initialized for the first time and I'm sure that keys are no longer squashed.

[lp:~camptocamp/new-report-intrastat/7.0-post-all-errors-rather-than-one-by-one]

94. By Guewen Baconnier @ Camptocamp on 2014-08-12

skip the source uom checks when a line has no uom

Unmerged revisions

94. By Guewen Baconnier @ Camptocamp on 2014-08-12

skip the source uom checks when a line has no uom

93. By Guewen Baconnier @ Camptocamp on 2014-07-04

'partner_country_id_to_write' could have been initialized before

92. By Guewen Baconnier @ Camptocamp on 2014-07-04

do not reset 'transaction_code_to_write', it should be False only when is_fiscal_only is true

91. By Guewen Baconnier @ Camptocamp on 2014-06-12

Post a message with all errors rather than raising them one by one.

=== modified file 'l10n_fr_intrastat_product/intrastat_product.py'
--- l10n_fr_intrastat_product/intrastat_product.py	2014-07-03 22:18:44 +0000
+++ l10n_fr_intrastat_product/intrastat_product.py	2014-08-12 13:38:07 +0000
@@ -20,8 +20,10 @@
 #
 ##############################################################################
 
+from collections import defaultdict
 from openerp.osv import orm, fields
 from openerp.tools.translate import _
+from openerp import pooler
 import openerp.addons.decimal_precision as dp
 from datetime import datetime
 from dateutil.relativedelta import relativedelta
@@ -213,6 +215,7 @@
         lines_to_create = []
         total_invoice_cur_accessory_cost = 0.0
         total_invoice_cur_product_value = 0.0
+        errors = defaultdict(list)
         for line in invoice.invoice_line:
             line_qty = line.quantity
             source_uom = line.uos_id
@@ -248,47 +251,77 @@
             total_invoice_cur_product_value += line.price_subtotal
             invoice_currency_id_to_write = invoice.currency_id.id
 
+            weight_to_write = False
+            source_uom_id_to_write = False
+            intrastat_code_id_to_write = False
+            intrastat_code_to_write = False
+            quantity_to_write = False
+            intrastat_uom_id_to_write = False
+            product_country_origin_id_to_write = False
+
             if not parent_values['is_fiscal_only']:
 
                 if not source_uom:
-                    raise orm.except_orm(_('Error :'), _("Missing unit of measure on the line with %d product(s) '%s' on invoice '%s'.") % (line_qty, line.product_id.name, invoice.number))
+                    errors[line.id].append(
+                        _("Missing unit of measure on the line with %d "
+                          "product(s) '%s' on invoice '%s'.") %
+                        (line_qty, line.product_id.name, invoice.number)
+                    )
                 else:
                     source_uom_id_to_write = source_uom.id
 
-                if source_uom.id == kg_uom_id:
-                    weight_to_write = line_qty
-                elif source_uom.category_id.id == weight_uom_categ_id:
-                    dest_uom_kg = self.pool.get('product.uom').browse(cr, uid,
-                        kg_uom_id, context=context)
-                    weight_to_write = self.pool.get('product.uom')._compute_qty_obj(cr, uid,
-                        source_uom, line_qty, dest_uom_kg, context=context)
-                elif source_uom.category_id.id == pce_uom_categ_id:
-                    if not line.product_id.weight_net:
-                        raise orm.except_orm(_('Error :'), _("Missing net weight on product '%s'.") % (line.product_id.name))
-                    if source_uom.id == pce_uom_id:
-                        weight_to_write = line.product_id.weight_net * line_qty
+                    if source_uom.id == kg_uom_id:
+                        weight_to_write = line_qty
+                    elif source_uom.category_id.id == weight_uom_categ_id:
+                        dest_uom_kg = self.pool.get('product.uom').browse(cr, uid,
+                            kg_uom_id, context=context)
+                        weight_to_write = self.pool.get('product.uom')._compute_qty_obj(cr, uid,
+                            source_uom, line_qty, dest_uom_kg, context=context)
+                    elif source_uom.category_id.id == pce_uom_categ_id:
+                        if not line.product_id.weight_net:
+                            errors[line.id].append(
+                                _("Missing net weight on product '%s'.") %
+                                (line.product_id.name)
+                            )
+                        elif source_uom.id == pce_uom_id:
+                            weight_to_write = line.product_id.weight_net * line_qty
+                        else:
+                            dest_uom_pce = self.pool.get('product.uom').browse(cr, uid,
+                                pce_uom_id, context=context)
+                            # Here, I suppose that, on the product, the weight is per PCE and not per uom_id
+                            weight_to_write = line.product_id.weight_net * self.pool.get('product.uom')._compute_qty_obj(cr, uid, source_uom, line_qty, dest_uom_pce, context=context)
+
                     else:
-                        dest_uom_pce = self.pool.get('product.uom').browse(cr, uid,
-                            pce_uom_id, context=context)
-                        # Here, I suppose that, on the product, the weight is per PCE and not per uom_id
-                        weight_to_write = line.product_id.weight_net * self.pool.get('product.uom')._compute_qty_obj(cr, uid, source_uom, line_qty, dest_uom_pce, context=context)
-
-                else:
-                    raise orm.except_orm(_('Error :'), _("Conversion from unit of measure '%s' to 'Kg' is not implemented yet.") % (source_uom.name))
+                        errors[line.id].append(
+                            _("Conversion from unit of measure '%s' to "
+                            "'Kg' is not implemented yet.") % (source_uom.name)
+                        )
 
                 product_intrastat_code = line.product_id.intrastat_id
                 if not product_intrastat_code:
                     # If the H.S. code is not set on the product, we check if it's set
                     # on it's related category
                     product_intrastat_code = line.product_id.categ_id.intrastat_id
-                    if not product_intrastat_code:
-                        raise orm.except_orm(_('Error :'), _("Missing H.S. code on product '%s' or on it's related category '%s'.") % (line.product_id.name, line.product_id.categ_id.complete_name))
-                intrastat_code_id_to_write = product_intrastat_code.id
 
-                if not product_intrastat_code.intrastat_code:
-                    raise orm.except_orm(_('Error :'), _("Missing intrastat code on H.S. code '%s' (%s).") % (product_intrastat_code.name, product_intrastat_code.description))
+                if not product_intrastat_code:
+                    errors[line.id].append(
+                        _("Missing H.S. code on product '%s' "
+                            "or on it's related category '%s'.") %
+                        (line.product_id.name,
+                            line.product_id.categ_id.complete_name)
+                    )
                 else:
-                    intrastat_code_to_write = product_intrastat_code.intrastat_code
+                    intrastat_code_id_to_write = product_intrastat_code.id
+
+                    if not product_intrastat_code.intrastat_code:
+                        errors[line.id].append(
+                            _("Missing intrastat code on H.S. code "
+                              "'%s' (%s).") %
+                            (product_intrastat_code.name,
+                             product_intrastat_code.description)
+                        )
+                    else:
+                        intrastat_code_to_write = product_intrastat_code.intrastat_code
 
                 if not product_intrastat_code.intrastat_uom_id:
                     intrastat_uom_id_to_write = False
@@ -302,7 +335,15 @@
                             uid, source_uom, line_qty,
                             product_intrastat_code.intrastat_uom_id, context=context)
                     else:
-                        raise orm.except_orm(_('Error :'), _("On invoice '%s', the line with product '%s' has a unit of measure (%s) which can't be converted to UoM of it's intrastat code (%s).") % (invoice.number, line.product_id.name, source_uom_id_to_write, intrastat_uom_id_to_write))
+                        errors[line.id].append(
+                            _("On invoice '%s', the line with product "
+                              "'%s' has a unit of measure (%s) which can't "
+                              "be converted to UoM of it's intrastat "
+                              "code (%s).") % (invoice.number,
+                                               line.product_id.name,
+                                               source_uom_id_to_write,
+                                               intrastat_uom_id_to_write)
+                        )
 
                 # The origin country should only be declated on Import
                 if intrastat.type == 'export':
@@ -321,26 +362,27 @@
                             ('origin_country_id', '!=', False)
                             ], context=context)
                         if not supplier_ids:
-                            raise orm.except_orm(_('Error :'),
-                                _("Missing country of origin on product '%s' or on it's supplier information for partner '%s'.")
-                                % (line.product_id.name, parent_values.get('origin_partner_name', 'none')))
+                            errors[line.id].append(
+                                _("Missing country of origin on product "
+                                  "'%s' or on it's supplier information for "
+                                  "partner '%s'.") %
+                                (line.product_id.name,
+                                 parent_values.get('origin_partner_name', 'none'))
+                            )
                         else:
                             product_country_origin_id_to_write = supplieri_obj.read(cr, uid,
                                 supplier_ids[0], ['origin_country_id'],
                                 context=context)['origin_country_id'][0]
                     else:
-                        raise orm.except_orm(_('Error :'),
-                            _("Missing country of origin on product '%s' (it's not possible to get the country of origin from the 'supplier information' in this case because we don't know the supplier of this product for the invoice '%s').")
-                            % (line.product_id.name, invoice.number))
-
-            else:
-                weight_to_write = False
-                source_uom_id_to_write = False
-                intrastat_code_id_to_write = False
-                intrastat_code_to_write = False
-                quantity_to_write = False
-                intrastat_uom_id_to_write = False
-                product_country_origin_id_to_write = False
+                        errors[line.id].append(
+                            _("Missing country of origin on product "
+                              "'%s' (it's not possible to get the country "
+                              "of origin from the 'supplier information' "
+                              "in this case because we don't know the "
+                              "supplier of this product for the "
+                              "invoice '%s').") %
+                            (line.product_id.name, invoice.number)
+                        )
 
 
             create_new_line = True
@@ -388,24 +430,34 @@
         # is always True
         # So we should not block with a raise before the end of the loop on the
         # invoice lines
+        invoice_errors = []
         if lines_to_create:
+            parent_values['partner_vat_to_write'] = False
             if parent_values['is_vat_required']:
                 # If I have invoice.intrastat_country_id and the invoice partner
                 # is outside the EU, then I look for the fiscal rep of the partner
                 if invoice.intrastat_country_id and not invoice.partner_id.country_id.intrastat:
                     if not invoice.partner_id.intrastat_fiscal_representative:
-                        raise orm.except_orm(_('Error :'), _("Missing fiscal representative for partner '%s'. It is required for invoice '%s' which has an invoice partner outside the EU but the goods were delivered to or received from inside the EU.") % (invoice.partner_id.name, invoice.number))
+                        invoice_errors.append(
+                            _("Missing fiscal representative for partner "
+                              "'%s'. It is required for invoice '%s' which "
+                              "has an invoice partner outside the EU but "
+                              "the goods were delivered to or received from "
+                              "inside the EU.") %
+                            (invoice.partner_id.name, invoice.number)
+                        )
                     else:
                         parent_values['partner_vat_to_write'] = invoice.partner_id.intrastat_fiscal_representative.vat
                 # Otherwise, I just read the vat number on the partner of the invoice
                 else:
 
                     if not invoice.partner_id.vat:
-                        raise orm.except_orm(_('Error :'), _("Missing VAT number on partner '%s'.") % invoice.partner_id.name)
+                        invoice_errors.append(
+                            _("Missing VAT number on partner '%s'.") %
+                            invoice.partner_id.name
+                        )
                     else:
                         parent_values['partner_vat_to_write'] = invoice.partner_id.vat
-            else:
-                parent_values['partner_vat_to_write'] = False
 
         for line_to_create in lines_to_create:
             line_to_create['partner_vat'] = parent_values['partner_vat_to_write']
@@ -422,7 +474,6 @@
 
             line_to_create['amount_invoice_currency'] = line_to_create['amount_product_value_inv_cur'] + line_to_create['amount_accessory_cost_inv_cur']
 
-
             # We do currency conversion NOW
             if invoice.currency_id.name != 'EUR':
                 # for currency conversion
@@ -439,13 +490,13 @@
             for value in ['quantity', 'weight']:  # These 2 fields are char
                 if line_to_create[value]:
                     line_to_create[value] = str(int(round(line_to_create[value], 0)))
-            line_obj.create(cr, uid, line_to_create, context=context)
-
-        return True
-
+            if not errors and not invoice_errors:
+                line_obj.create(cr, uid, line_to_create, context=context)
+
+        return invoice_errors, errors
 
     def compute_invoice_values(self, cr, uid, intrastat, invoice, parent_values, context=None):
-
+        errors = []
         intrastat_type = self.pool.get('report.intrastat.type').read(cr, uid, parent_values['intrastat_type_id_to_write'], context=context)
         parent_values['procedure_code_to_write'] = intrastat_type['procedure_code']
         parent_values['transaction_code_to_write'] = intrastat_type['transaction_code']
@@ -456,10 +507,20 @@
             # force to is_fiscal_only
             parent_values['is_fiscal_only'] = True
 
+        parent_values.update({
+            'department_to_write': False,
+            'transport_to_write': False,
+        })
+        parent_values.setdefault('partner_country_id_to_write', False)
         if not parent_values['is_fiscal_only']:
             if not invoice.intrastat_transport:
                 if not intrastat.company_id.default_intrastat_transport:
-                    raise orm.except_orm(_('Error :'), _("The mode of transport is not set on invoice '%s' nor the default mode of transport on the company '%s'.") % (invoice.number, intrastat.company_id.name))
+                    errors.append(
+                        _("The mode of transport is not set on invoice '%s' "
+                          "nor the default mode of transport on "
+                          "the company '%s'.") %
+                        (invoice.number, intrastat.company_id.name)
+                    )
                 else:
                     parent_values['transport_to_write'] = intrastat.company_id.default_intrastat_transport
             else:
@@ -467,18 +528,22 @@
 
             if not invoice.intrastat_department:
                 if not intrastat.company_id.default_intrastat_department:
-                    raise orm.except_orm(_('Error :'), _("The intrastat department hasn't been set on invoice '%s' and the default intrastat department is missing on the company '%s'.") % (invoice.number, intrastat.company_id.name))
+                    errors.append(
+                        _("The intrastat department hasn't been set on "
+                          "invoice '%s' and the default intrastat department "
+                          "is missing on the company '%s'.") %
+                        (invoice.number, intrastat.company_id.name)
+                    )
                 else:
                     parent_values['department_to_write'] = intrastat.company_id.default_intrastat_department
             else:
                 parent_values['department_to_write'] = invoice.intrastat_department
         else:
-            parent_values['department_to_write'] = False
-            parent_values['transport_to_write'] = False
-            parent_values['transaction_code_to_write'] = False
-            parent_values['partner_country_id_to_write'] = False
-        #print "parent_values =", parent_values
-        return parent_values
+            parent_values.update({
+                'transaction_code_to_write': False,
+                'partner_country_id_to_write': False,
+            })
+        return parent_values, errors
 
 
     def generate_product_lines_from_invoice(self, cr, uid, ids, context=None):
@@ -497,7 +562,7 @@
         if intrastat.type == 'import':
             # Les régularisations commerciales à l'HA ne sont PAS
             # déclarées dans la DEB, cf page 50 du BOD 6883 du 06 janvier 2011
-            invoice_type = ('in_invoice', 'POUET')  # I need 'POUET' to make it a tuple
+            invoice_type = 'in_invoice',  # must be a tuple
         if intrastat.type == 'export':
             invoice_type = ('out_invoice', 'out_refund')
         invoice_ids = invoice_obj.search(cr, uid, [
@@ -508,13 +573,15 @@
             ('company_id', '=', intrastat.company_id.id)
         ], order='date_invoice', context=context)
         #print "invoice_ids=", invoice_ids
+        errors = defaultdict(list)
+        line_errors = {}
         for invoice in invoice_obj.browse(cr, uid, invoice_ids, context=context):
             #print "INVOICE num =", invoice.number
             parent_values = {}
 
             # We should always have a country on partner_id
             if not invoice.partner_id.country_id:
-                raise orm.except_orm(_('Error :'), _("Missing country on partner '%s'.") % invoice.partner_id.name)
+                errors[invoice.id].append(_("Missing country on partner '%s'.") % invoice.partner_id.name)
 
             # If I have no invoice.intrastat_country_id, which is the case the first month
             # of the deployment of the module, then I use the country on invoice partner
@@ -540,17 +607,38 @@
                     if intrastat.company_id.default_intrastat_type_out_invoice:
                         parent_values['intrastat_type_id_to_write'] = intrastat.company_id.default_intrastat_type_out_invoice.id
                     else:
-                        raise orm.except_orm(_('Error :'), _("The intrastat type hasn't been set on invoice '%s' and the 'default intrastat type for customer invoice' is missing for the company '%s'.") % (invoice.number, intrastat.company_id.name))
+                        raise orm.except_orm(
+                            _('Error'),
+                            _("The intrastat type hasn't been set on "
+                              "invoice '%s' and the 'default intrastat type "
+                              "for customer invoice' is missing for "
+                              "the company '%s'.") % (invoice.number,
+                                                      intrastat.company_id.name)
+                        )
                 elif invoice.type == 'out_refund':
                     if intrastat.company_id.default_intrastat_type_out_refund:
                         parent_values['intrastat_type_id_to_write'] = intrastat.company_id.default_intrastat_type_out_refund.id
                     else:
-                        raise orm.except_orm(_('Error :'), _("The intrastat type hasn't been set on refund '%s' and the 'default intrastat type for customer refund' is missing for the company '%s'.") % (invoice.number, intrastat.company_id.name))
+                        raise orm.except_orm(
+                            _('Error'),
+                            _("The intrastat type hasn't been set on refund "
+                              "'%s' and the 'default intrastat type for "
+                              "customer refund' is missing for the company"
+                              "'%s'.") % (invoice.number,
+                                          intrastat.company_id.name)
+                        )
                 elif invoice.type == 'in_invoice':
                     if intrastat.company_id.default_intrastat_type_in_invoice:
                         parent_values['intrastat_type_id_to_write'] = intrastat.company_id.default_intrastat_type_in_invoice.id
                     else:
-                        raise orm.except_orm(_('Error :'), _("The intrastat type hasn't been set on invoice '%s' and the 'Default intrastat type for supplier invoice' is missing for the company '%s'.") % (invoice.number, intrastat.company_id.name))
+                        raise orm.except_orm(
+                            _('Error'),
+                            _("The intrastat type hasn't been set on invoice "
+                              "'%s' and the 'Default intrastat type for "
+                              "supplier invoice' is missing for the company "
+                              "'%s'.") % (invoice.number,
+                                          intrastat.company_id.name)
+                        )
                 else:
                     raise orm.except_orm(_('Error :'), "Hara kiri... we can't have a supplier refund")
 
@@ -567,10 +655,88 @@
             parent_values['origin_partner_id'] = invoice.partner_id.id
             parent_values['origin_partner_name'] = invoice.partner_id.name
 
-            parent_values = self.compute_invoice_values(cr, uid, intrastat, invoice, parent_values, context=context)
-
-            self.create_intrastat_product_lines(cr, uid, ids, intrastat, invoice, parent_values, context=context)
-
+            parent_values, vals_errors = self.compute_invoice_values(
+                cr, uid, intrastat, invoice, parent_values, context=context)
+            if vals_errors:
+                errors[invoice.id] += vals_errors
+
+            # message_post
+            create_errors, create_line_errors = self.create_intrastat_product_lines(
+                cr, uid, ids, intrastat, invoice,
+                parent_values, context=context)
+
+            if create_errors:
+                errors[invoice.id] += create_errors
+            if create_line_errors:
+                line_errors[invoice.id] = create_line_errors
+
+        if errors or line_errors:
+            message_template = _(
+                "<b>The generation of lines could not be completed "
+                "due to the following issues:</b>"
+                "<p>"
+                "%s"
+                "</p>"
+            )
+            invoice_template = _(
+                "<b>Invoice %(name)s:</b>"
+                "<ul>"
+                "%(errors)s"
+                "</ul>"
+                "<ul>"
+                "%(lines)s"
+                "</ul>"
+            )
+            lines_template = _(
+                "<b>Line %(name)s:</b>"
+                "<ul>"
+                "%(errors)s"
+                "</ul>"
+            )
+            invoice_line_obj = self.pool['account.invoice.line']
+            invoice_messages = []
+            for invoice_id in invoice_ids:
+                line_message = []
+                for line_id, line_errs in line_errors.get(invoice_id, {}).iteritems():
+                    line = invoice_line_obj.browse(cr, uid, line_id,
+                                                   context=context)
+                    line_msg = lines_template % {
+                        'name': line.name,
+                        'errors': ''.join("<li>%s</li>" % err for err
+                                          in line_errs)
+                    }
+                    line_message.append(line_msg)
+
+                invoice = invoice_obj.browse(cr, uid, invoice_id,
+                                             context=context)
+                inv_errs = errors.get(invoice_id, [])
+                if inv_errs or line_message:
+                    invoice_message = invoice_template % {
+                        'name': invoice.number,
+                        'errors': ''.join("<li>%s</li>" % err for err
+                                          in inv_errs),
+                        'lines': ''.join(line_message),
+                    }
+                    invoice_messages.append(invoice_message)
+            message = message_template % ''.join(invoice_messages)
+
+            db = pooler.get_db(cr.dbname)
+            local_cr = db.cursor()
+            try:
+                self.message_post(local_cr, uid, ids, body=message,
+                                  subtype='mail.mt_comment', context=context)
+            except:
+                local_cr.rollback()
+                raise
+            else:
+                local_cr.commit()
+            finally:
+                local_cr.close()
+            raise orm.except_orm(
+                _('Error'),
+                _('The lines could not be generated, please '
+                  'refresh the form and see the messages.')
+            )
         return True