Code review comment for lp:~camptocamp/carriers-deliveries/7.0-add-delivery_carrier_label_postlogistics-yvr

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

On the name of the module: rename delivery_carrier_label_laposte to delivery_carrier_label_postlogistics: "laposte" has been used for "La Poste" (from France) for the delivery files.

Suds does not have an exception class for the authentication errors? I think that it should, seems to be suds.transport.TransportError

        try:
            res['value'] = request(**kwargs)
            res['success'] = True
        except WebFault, e:
            res['success'] = False
            res['errors'] = [e[0]]
        except Exception, e:

On a side note, the preferred notation now is "except Exception as e:

            # if authentification error
            if isinstance(e[0], tuple) and e[0][0] == 401:
                raise orm.except_orm(
                    _('Error 401'),
                    _('Authorization Required\n\n'
                    'Please verify postlogistics username and password in:\n'
                    'Configuration -> Postlogistics'))
            raise e

"raise e" is wrong, you will mess the traceback (it will start from the current line instead of the original one), to propagate correctly the current exception, the correct idiom is just "raise".

Regarding the comment below, the weight (and weight_net) field already exists on the picking.

        #XXX get weight or set it as an option on picking
        weight = 0

« Back to merge proposal