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.

Sign up for GitHub

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

Jump to bottom

[9.0] [MIG] base_partner_sequence module #358

Merged
merged 4 commits into from
Dec 19, 2016
Merged

[9.0] [MIG] base_partner_sequence module #358

merged 4 commits into from
Dec 19, 2016

Conversation

cubells
Copy link
Member

@cubells cubells commented Dec 15, 2016

  • Updated README
  • Updated views and sequence

cc @Tecnativa

@cubells cubells changed the title 9.0 mig base partner sequence [9.0] [MIG] base_partner_sequence module Dec 15, 2016
@rafaelbn rafaelbn added this to the 9.0 milestone Dec 15, 2016
rafaelbn
rafaelbn approved these changes Dec 15, 2016
View reviewed changes
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.

Runbot is Red, not sure why... because I 've tested in runbot and functionally is OK, thanks!

pedrobaeza
pedrobaeza requested changes Dec 16, 2016
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Please add a test for creating a contact to see which reference has (it has to be the same as the parent, and the sequence mustn't increase).

base_partner_sequence/__openerp__.py
"category": "Generic Modules/Base",
"website": "http://www.initos.com",
"depends": ["base"],
"website": "http://www.tecnativa.com",
Copy link
Member

Choose a reason for hiding this comment

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

Don't change URL

base_partner_sequence/models/partner.py
partner's 'ref'
"""
if not vals and not partner_id:
raise Exception('Either field values or an id must be provided.')
Copy link
Member

Choose a reason for hiding this comment

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

Use Odoo exception

base_partner_sequence/views/partner_view.xml
<field name="inherit_id" ref="base.view_partner_form"/>
<field name="arch" type="xml">
<field name="ref" position="attributes">
<attribute name="attrs">{
Copy link
Member

Choose a reason for hiding this comment

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

Put this in only line for not messing up architecture view in Odoo

@pedrobaeza
Copy link
Member

There's also a strange repetition of commits that you should put together

This was referenced Dec 16, 2016
Closed
Closed
pedrobaeza
pedrobaeza reviewed Dec 16, 2016
View reviewed changes
base_partner_sequence/models/partner.py
super(ResPartner, partner).write(vals)
return True

@api.one
Copy link
Member

Choose a reason for hiding this comment

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

Don't use @api.one here.

@cubells cubells force-pushed the 9.0-mig-base_partner_sequence branch from abddcf3 to be1a4a1 Compare December 16, 2016 04:35
@cubells cubells force-pushed the 9.0-mig-base_partner_sequence branch from be1a4a1 to 3b6827f Compare December 16, 2016 04:36
gdgellatly
gdgellatly approved these changes Dec 19, 2016
View reviewed changes
Copy link
Contributor

@gdgellatly gdgellatly left a comment

Choose a reason for hiding this comment

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

My comments are non-blocking. I don't know the answer to the base dependency question. I imagine the code is identical for 10.0, are we going to forward port when this gets approved?

base_partner_sequence/models/partner.py

:param parnter_id: id of the partner object
:param vals: known field values of the partner object
:return: true iff a sequence value should be assigned to the\
Copy link
Contributor

Choose a reason for hiding this comment

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

typo - partner
typo - non blocking
typo - unnecessary trailing slash

base_partner_sequence/__openerp__.py
"category": "Generic Modules/Base",
"website": "http://www.initos.com",
"depends": ["base"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I know ordinarily there is an implicit dependency on base, but in this case, because the module is directly referencing base xml ids I think it would be better to explicitly state. I've only done a static review and not tested, so don't know whether this would ever actually cause a problem on a fresh database.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, having tested in runbot, it seems that on a fresh install it will pick up base xml ids so ignore this comment if you choose.

pedrobaeza
pedrobaeza approved these changes Dec 19, 2016
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I have amended last Graeme's comments and this is ready to be merged when CIs finish

@pedrobaeza pedrobaeza merged commit 07d9aae into OCA:9.0 Dec 19, 2016
@pedrobaeza pedrobaeza deleted the 9.0-mig-base_partner_sequence branch December 19, 2016 09:25
@cubells
Copy link
Member Author

cubells commented Dec 19, 2016

Tanks @pedrobaeza

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdgellatly gdgellatly gdgellatly approved these changes

@rafaelbn rafaelbn rafaelbn approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@cubells @pedrobaeza @gdgellatly @rafaelbn @lmignon @oca-transbot