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

[IMP] base_location: Updated to v8 syntax and uses. Added filters in som... #81

Merged
merged 5 commits into from
Jan 21, 2015
Merged

[IMP] base_location: Updated to v8 syntax and uses. Added filters in som... #81

merged 5 commits into from
Jan 21, 2015

Conversation

alejandrosantana
Copy link

...e views. Added two columns in respective tree views.

base_location/partner-contact changes

API

  • Updated code to v8 API syntax
  • Moved models to 'models/' folder.
  • Moved views to 'views/' folder, removed suffix '_views' as redundant (for being now in 'views/').

Views

  • Added 'currency_id' column to res.country tree view.
  • Added 'state_id' column to better.zip tree view
  • Added several filters to views, now you can:
    • search a country by its code in country view.
    • search by zip, city code, city name, state and country in better_zip view.
    • group by state and country in better_zip search view.

Questions

  • My PEP8/flake8 check throws this false error:
    • ./base_location/models/partner.py:28:13: E222 multiple spaces after operator
    • But there is only a slash(/) in the field string value, so there is no real operator there.

…some views. Added two columns in respective tree views.
@pedrobaeza
Copy link
Member

What is the purpose of the currency_id? I think in this module (name base_location), this concept doesn't apply.

}
{
'name': 'Location management (aka Better ZIP)',
'version': '0.3.2',
Copy link
Member

Choose a reason for hiding this comment

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

As this is in new API, you can put 1.0 as a major version

@alejandrosantana
Copy link
Author

OK, changes in a few minutes.

…van currency_id column on country tree view, enhanced description and manidest, addded README.rst file.
@alejandrosantana
Copy link
Author

Changes done.
May I ask, why README.rst? (instead of .md).

@pedrobaeza
Copy link
Member

Because Odoo only parses reStructuredText (although in a .md file), and GitHub will be confused if we put .md.

@pedrobaeza
Copy link
Member

Can you check Travis?

'Alejandro Santana <alejandrosantana@anubia.es>',
],
'summary': '''Enhanced zip/npa management system''',
'description': '''
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to put now this key as you have README.rst

@pedrobaeza
Copy link
Member

The convention about view file names is to have the suffix _view.xml, so please rename your files accordingly.

</search>
</field>
</record>

Copy link
Member

Choose a reason for hiding this comment

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

Please, don't add useless spaces

@yvaucher
Copy link
Member

@pedrobaeza about file name, convention is _view.xml unless it is in view or views directory. So nothing to change here for that.

@pedrobaeza
Copy link
Member

Not exactly, because inside view/views folder you can also put _menu.xml files. That's why I keep also the _view suffix.

@alejandrosantana
Copy link
Author

OK, I am doing all changes. Regarding the views naming I will stick to "_view.xml" naming and later on we can discuss what naming should be preferred, all right?

@alejandrosantana
Copy link
Author

Regarding Travis, there was a trailing space in this module causing flake8 errors. Then, I see some error in another module (first_name).

@yvaucher
Copy link
Member

@pedrobaeza yep you can also have qweb report in view thus you can add suffix for menu and prefix for report and keep it short with view. No real convention is there thought. In most case you don't even have menu, just views, so I'm for view without suffix.

@yvaucher
Copy link
Member

@alejandrosantana You might have misread it:

./base_location/__openerp__.py:37:76: W291 trailing whitespace

'summary': '''Enhanced zip/npa management system''',
'description': '''
This module introduces a better zip/npa management system.
It enables zip, city, state and country auto-completion on partners and
Copy link
Member

Choose a reason for hiding this comment

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

Here is the extra space to remove at EOL

@alejandrosantana
Copy link
Author

@yvaucher Yes, that's the one causing flake8 to fail. Anyway, that is fixed now. One minute and I push the changes.

…ted fields. Also use @api.one in onchanges. Using now spaces instead of tabs in xml files.
@alejandrosantana
Copy link
Author

Changes done. Push done. Let's see what Travis has to say.

@alejandrosantana
Copy link
Author

Seems Travis is failing with tests in 'partner_firstname'.

@pedrobaeza
Copy link
Member

OK, Travis is unrelated, so please make the rest of the changes. Remove also better_zip symlink, because it's deprecated.

@alejandrosantana
Copy link
Author

All changes done, except removal of better_zip, which I will do in a minute.

@alejandrosantana
Copy link
Author

@pedrobaeza better_zip removed.

@pedrobaeza
Copy link
Member

Thanks for the changes!

👍

@alejandrosantana
Copy link
Author

Nice! I liked collaborating! Great way to learn too.
Have to steal some more time for more.

@yvaucher
Copy link
Member

Error is indeed unrelated thanks

👍

@api.onchange('zip_id')
def onchange_zip_id(self):
if self.zip_id:
bzip = self.zip_id[0]
Copy link
Member

Choose a reason for hiding this comment

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

[0] is useless, bzip = self.zip_id will behave exactly the same

@guewen
Copy link
Member

guewen commented Jan 21, 2015

1 minor remark, but 👍

@alejandrosantana
Copy link
Author

@guewen Minor thing, but you are right. Already fixed.

@guewen
Copy link
Member

guewen commented Jan 21, 2015

Thanks!

guewen added a commit that referenced this pull request Jan 21, 2015
[IMP] base_location: Updated to v8 syntax and uses. Added filters in som...
@guewen guewen merged commit b6315c6 into OCA:8.0 Jan 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants