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

11.0 mig website_sale_vat_required #236

Merged
merged 5 commits into from
Jun 22, 2018

Conversation

ernestotejeda
Copy link
Member

@ernestotejeda ernestotejeda commented Jun 3, 2018

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

'author': "Agile Business Group, "
"Tecnativa, "
"Odoo Community Association (OCA)",
'website': 'http://www.agilebg.com',
Copy link
Member

Choose a reason for hiding this comment

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

@rafaelbn rafaelbn added this to the 11.0 milestone Jun 5, 2018
@rafaelbn
Copy link
Member

rafaelbn commented Jun 5, 2018

cc @Tecnativa

@rafaelbn rafaelbn requested review from yajo and rafaelbn June 5, 2018 05:01
/* Copyright 2017 Jairo Llopis <jairo.llopis@tecnativa.com>
* License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). */

odoo.define('website_sale_vat_required.tour', function (require) {
Copy link
Member

Choose a reason for hiding this comment

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

If you remove this patch, then the website_sale main tour fails (as it's happening right now)

<xpath expr="//input[@name='vat']/.." position="replace"/>
<xpath expr="//input[@name='company_name']/../.." position="after">
<t t-if="mode[1] == 'billing'">
<div t-attf-class="form-group #{error.get('vat') and 'has-error' or ''} col-md-6 div_vat">
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. It seems included upstream already.

Copy link
Member Author

Choose a reason for hiding this comment

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

This migration has been made from v9 directly . In v9 the only thing you need to do is set 'vat' as mandatory field using _get_mandatory_billing_fields method. In v10 and v11, if you only do that and you don't modify the template 'address', then, when an user is logged in, odoo will not permit to complete the form until this field is filled, but this field will be hide and the user will wonder what happened. This is, because now, odoo only show this field if there is no user logged in.

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting. So, Odoo doesn't let a user modify his VAT number. IMHO that makes sense; if you need to change such field, you're probably better creating a new customer.

Then maybe you should only require vat when he's adding a new address. I think you can check that by making sure there's no request.params["partner_id"].

'base_vat',
],
'demo': [
'views/templates.xml',
Copy link
Member

@yajo yajo Jun 5, 2018

Choose a reason for hiding this comment

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

This is not a demo file. Either put it in the demo folder or in the data key.

<xpath expr="//input[@name='vat']/.." position="replace"/>
<xpath expr="//input[@name='company_name']/../.." position="after">
<t t-if="mode[1] == 'billing'">
<div t-attf-class="form-group #{error.get('vat') and 'has-error' or ''} col-md-6 div_vat">
Copy link
Member

Choose a reason for hiding this comment

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

Good question. It seems included upstream already.

@rafaelbn
Copy link
Member

Please @pedrobaeza @yajo could you update you reviews after last commits? Thanks!

@rafaelbn
Copy link
Member

@ernestotejeda please review travis 🔴 thanks!

@yajo
Copy link
Member

yajo commented Jun 13, 2018

You need to fix the test: https://travis-ci.org/OCA/e-commerce/jobs/388963897#L3102

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Functionally tested 👍

* `Tecnativa <https://www.tecnativa.com>`_:

* Jairo Llopis <jairo.llopis@tecnativa.com>
* Ernesto Tejeda <ernesto.tejeda87@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't put your personal e-mail here. In fact, we are even removing our corporate ones in all places. I'm changing this myself if the rest is correct.

@pedrobaeza
Copy link
Member

Travis is failing. Please check that and make the change about the email address.

@pedrobaeza pedrobaeza mentioned this pull request Jun 20, 2018
22 tasks
eLBati and others added 5 commits June 22, 2018 14:03
[FIX] Depend on vat validation module that causes the VAT number field to be displayed at checkout in the first place
This addon got migrated from 8.0. Relevant notes:

- Moved from `OCA/website` to `OCA/e-commerce`.
- Reduced license headers to new style ones, keeping copyright.
- Updated README template.
- Replaced dirty hack that disables addon in test mode, and hack the tour instead (actually testing the addon).
- Benefit from upstream updates, that now handles incorrect VAT errors.
@pedrobaeza pedrobaeza force-pushed the 11.0-mig-website_sale_vat_required branch from e0531f9 to b844d8e Compare June 22, 2018 12:04
@pedrobaeza pedrobaeza merged commit ac5269c into OCA:11.0 Jun 22, 2018
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

7 participants