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

Port of partner_contact_birthdate #243

Merged
merged 21 commits into from
Aug 17, 2016

Conversation

leemannd
Copy link
Contributor

There was an error with multiple tabs "personal information".
xmids have been changed.
And duplicate views are supressed.

@pedrobaeza
Copy link
Member

The solution is not to make one module depending on the other, but put the same XML-ID on all modules declaring the same view (the one including the new tab). This XML-ID should be of a common existing module, for example "base.base_view". And then, declare another view that depends on this one adding the field in the tab.

<record id="base.personal_contact_information" model="ir.ui.view">
<field name="name">Personal information page for contacts form</field>
<record id="view_personal_information_gender" model="ir.ui.view">
<field name="name">Partner gender</field>
Copy link
Member

Choose a reason for hiding this comment

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

Wrong name here, it seems you copy/pasted the one from partner_contact_gender

@yvaucher
Copy link
Member

Reading description of https://github.com/OCA/partner-contact/tree/9.0/partner_contact_personal_information_page

It seems this module isn't intended anymore to be an inheritable module.

Thus, you should be able to remove the dependencies on it and declare in each module the extension
base.personal_contact_information . Having the same XML ID will make sure only one is valid at the end. And you should have only 1 tab.

This change took place here #193

@leemannd
Copy link
Contributor Author

@pedrobaeza Ok. Making it better.

@guewen
Copy link
Member

guewen commented Jan 18, 2016

This XML-ID should be of a common existing module, for example "base.base_view". And then, declare another view that depends on this one adding the field in the tab.

I disagree. IMO, we should never create records hijacking the namespace of another addon. Odoo won't be able anymore to clean the records created by the addon. Example: you have 2 addons that create a view base.partner_personal_page, even if you uninstall both, the view remains in the database. It sounds cleaner to me to have an intermediate addon for the creation of the view.

@leemannd
Copy link
Contributor Author

@guewen It is correct. The view remains in the database. For the moment, in wich manner should I make the modifications?

@leemannd leemannd mentioned this pull request Jan 18, 2016
31 tasks
@yvaucher
Copy link
Member

@pedrobaeza Have you seen @guewen comment ?
What is the benefit of the change you asked over the previous version using module dependencies?

@leemannd leemannd changed the title Fix multiple tabs "contact information" Port of partner_contact_birthdate Jan 18, 2016
@leemannd
Copy link
Contributor Author

@pedrobaeza @yvaucher @guewen
I have corrected the PR to fit the actual vision of partner_contact_information.
So now this is a port in version 9.0 of partner_contact_birthdate

@@ -18,15 +18,15 @@

{
Copy link
Member

Choose a reason for hiding this comment

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

Short header to use here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in all the files

@yvaucher
Copy link
Member

@leemannd Thanks 👍 it comply with the current state of other addons, whereas I'm not convinced it's the best way to do it.

EDIT: added reserves about the removal of the base module. Though this PR is ok for the current state.

@guewen
Copy link
Member

guewen commented Jan 20, 2016

I disagree. IMO, we should never create records hijacking the namespace of another addon. Odoo won't be able anymore to clean the records created by the addon. Example: you have 2 addons that create a view base.partner_personal_page, even if you uninstall both, the view remains in the database. It sounds cleaner to me to have an intermediate addon for the creation of the view.

Another effect: when you upgrade base, it won't find the view base.partner_personal_page in there so it will delete it (it will be created again when the dependent addon is installed but that's still undesirable).

@pedrobaeza
Copy link
Member

Hi, @guewen, you're totally right. I used this trick, but I didn't realize about the side effect, so the benefits are less than the cons. Go ahead with the other approach.

@leemannd
Copy link
Contributor Author

@pedrobaeza @guewen @YannickB This PR is doing two things:

  1. Port of partner_contact_birthdate & making
  2. The modules using the tab "personal informations" (nationality, gender, birthdate, "in_several_companies") depends now on the "partner_contact_personal_information_page"

@yvaucher
Copy link
Member

@leemannd Thanks 👍

@guewen
Copy link
Member

guewen commented Jan 25, 2016

Thanks @leemannd
👍

@pedrobaeza
Copy link
Member

Why Travis is failing?

@yvaucher
Copy link
Member

Hum a change is missing on

https://github.com/OCA/partner-contact/blob/9.0/partner_contact_personal_information_page/views/res_partner.xml#L6

As we need to revert it to ensure it is not on base module.

@guewen
Copy link
Member

guewen commented Mar 31, 2016

@leemannd did you see @yvaucher 's comment?

@JosDeGraeve
Copy link

A rebase fixes the failing build, see: leemannd#4

LGTM: 👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.277% when pulling 7a175a0 on leemannd:personal_information-fix into 73320d6 on OCA:9.0.

@lmignon
Copy link
Sponsor Contributor

lmignon commented May 30, 2016

👍 LGTM (Code review + Functional test) Not a blocking issue but it would be nice to move the override of res.partner from models.py to models/res_partner.py to be conform to the OCA standards.

@rafaelbn rafaelbn added this to the 9.0 milestone Jul 1, 2016
@rafaelbn
Copy link
Member

rafaelbn commented Jul 1, 2016

Hi @leemannd . Thanks a lot fot this PR. Please, as this module is for v9 it's very important to achive the simple change @lmignon commented in #243 (comment). Could you make it? I will merge after this. Thanks again

# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

# © <YEAR(S)> <AUTHOR(S)>
Copy link
Member

Choose a reason for hiding this comment

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

You should maintain original authors
cc @yajo

"version": "8.0.1.0.0",
"author": "Odoo Community Association (OCA)",
"version": "9.0.1.0.0",
"author": "Jairo Llopis",
Copy link
Member

Choose a reason for hiding this comment

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

The comma here goes inside the string

@yajo
Copy link
Member

yajo commented Aug 1, 2016

Thanks for taking care of this @leemannd 😉

@dreispt
Copy link
Sponsor Member

dreispt commented Aug 2, 2016

@leemannd Still needs fixing - see TravisCI log.

@leemannd
Copy link
Contributor Author

@yvaucher @dreispt @yajo Pep8 corrected and freshly rebased.

@gurneyalex
Copy link
Member

👍

@gurneyalex gurneyalex merged commit 691c6a1 into OCA:9.0 Aug 17, 2016
@leemannd leemannd deleted the personal_information-fix branch August 17, 2016 07:07
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