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

[10.0][ADD] business code and validation #10

Closed

Conversation

jarmokortetjarvi
Copy link

Adds a business id for partners.
Split to two modules: one just adds the field and other adds validation and formatting.
This way the validation will be optional, and the module can be installed in environments with incorrectly formatted business ids

@jarmokortetjarvi jarmokortetjarvi changed the title [10.0][ADD] l10n fi business code validate [10.0][ADD] business code and validation Oct 24, 2017
Copy link

@mlaitinen mlaitinen left a comment

Choose a reason for hiding this comment

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

There are some minor changes I'd like to see

@@ -0,0 +1,12 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>

Choose a reason for hiding this comment

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

This is Odoo 10, please use <odoo> and in cases of <openerp><data noupdate="1"> use <odoo noupdate="1">

License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). -->

<odoo>
<data>

Choose a reason for hiding this comment

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

<data> is unnecessary. Also, please check the indentation of this file.

@@ -0,0 +1,11 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>

Choose a reason for hiding this comment

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

Please change to <odoo>

from odoo.exceptions import ValidationError


class ResPartnerIdNumber(models.Model):

Choose a reason for hiding this comment

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

What's the purpose of this class?

@@ -0,0 +1,136 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>

Choose a reason for hiding this comment

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

Please use <odoo> instead and check the indentation of this file.

@@ -0,0 +1,25 @@
# -*- coding: utf-8 -*-
from openerp import api, fields, models

Choose a reason for hiding this comment

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

from odoo ..

@@ -0,0 +1,13 @@
# -*- coding: utf-8 -*-
from openerp import api, fields, models

Choose a reason for hiding this comment

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

from odoo ..

)
einvoice_operator_identifier = fields.Char(
string='Operator ID',
compute='get_einvoice_operator_identifier',

Choose a reason for hiding this comment

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

Couldn't this be a related field?

@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="UTF-8"?>
<openerp>

Choose a reason for hiding this comment

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

Use <odoo>, drop <data> and check indentations.

@jarmokortetjarvi
Copy link
Author

Fixed the formatting issues and removed the edicode-module. It had been merged to a wrong branch

@coveralls
Copy link

coveralls commented Dec 19, 2017

Coverage Status

Coverage decreased (-61.7%) to 38.298% when pulling 1dbb08c on Tawasta:10.0-l10n_fi_business_code_validate into a8abb9d on OCA:10.0.

@coveralls
Copy link

Coverage Status

Coverage decreased (-62.0%) to 38.0% when pulling 1dbb08c on Tawasta:10.0-l10n_fi_business_code_validate into a8abb9d on OCA:10.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-62.0%) to 38.0% when pulling 1dbb08c on Tawasta:10.0-l10n_fi_business_code_validate into a8abb9d on OCA:10.0.

@coveralls
Copy link

coveralls commented Dec 20, 2017

Coverage Status

Coverage decreased (-62.7%) to 37.255% when pulling 19ca1bd on Tawasta:10.0-l10n_fi_business_code_validate into a8abb9d on OCA:10.0.

@coveralls
Copy link

Coverage Status

Coverage decreased (-62.7%) to 37.255% when pulling 158fe2b on Tawasta:10.0-l10n_fi_business_code_validate into a8abb9d on OCA:10.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-62.7%) to 37.255% when pulling 158fe2b on Tawasta:10.0-l10n_fi_business_code_validate into a8abb9d on OCA:10.0.

@coveralls
Copy link

Coverage Status

Coverage decreased (-62.7%) to 37.255% when pulling f48107f on Tawasta:10.0-l10n_fi_business_code_validate into a8abb9d on OCA:10.0.

@coveralls
Copy link

coveralls commented Dec 29, 2017

Coverage Status

Coverage decreased (-62.7%) to 37.255% when pulling b1e0190 on Tawasta:10.0-l10n_fi_business_code_validate into 769b423 on OCA:10.0.

Copy link

@mlaitinen mlaitinen left a comment

Choose a reason for hiding this comment

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

LGTM except the payment term module related stuff

@@ -8,8 +8,6 @@ Finnish Invoice Payment Terms

Common Finnish invoice payment terms.

**Please note that this module will remove the default payment terms!**

Choose a reason for hiding this comment

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

Why is l10n_fi_payment_terms in the same PR? You seem to have another PR #12 open for almost the same thing.

@mlaitinen
Copy link

@jarmokortetjarvi Looks good, but there's still the EDI code module in the README which is yet to be merged. Also there seems to be a merge conflict in the README file.

mlaitinen referenced this pull request in avoinsystems/l10n-finland Jun 28, 2018
@jarmokortetjarvi jarmokortetjarvi deleted the 10.0-l10n_fi_business_code_validate branch July 26, 2018 10:10
@jarmokortetjarvi
Copy link
Author

The commit history was a tangled mess between oca/l10n-finland and tawasta/l10n-finland
Closed this and created a new PR #15 that has only the relevant commits via git subtree. The contents should be pretty much identical


business_id = fields.Char(
string='Business ID',
compute=lambda s: s._compute_identification(

Choose a reason for hiding this comment

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

Not sure if it's a big problem, but I'll add a note here anyway: using a lambda here means that anyone who wants to extend this model cannot directly extend the computation method of the field business_id in an Odoo-like fashion: they either have to do some pretty gnarly monkeypatching, or extend _compute_identification and trust or ensure that their changes only apply to the computation of this field. I think it would be a little better if we just go the normal route and define real computation methods and then refer to them by string in the field definition.


# The formal format is ok, check the validation number
multipliers = [7, 9, 10, 5, 8, 4,
2] # Number-space specific multipliers

Choose a reason for hiding this comment

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

Could the comment be above, so the multipliers could fit on a single line?

validation_bit = business_id_number[7:8]

# Test the validation bit
for number in business_id_number[0:7]:

Choose a reason for hiding this comment

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

We could iterate over the zip instead:

for number, multiplier in zip(business_id_number[0:7], multipliers):
    validation_multiplier += multiplier * int(number)

Then we don't need number_index

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.

None yet

4 participants