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
[ADD] Delivery Carrier Seur Module #76
Conversation
@@ -0,0 +1,39 @@ | |||
Module for SEUR carrier labels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use latest README template: https://raw.githubusercontent.com/OCA/maintainer-tools/master/template/module/README.rst
You should add seur library installatin in .travis.yml file. |
Anyway, I can't see the usage of the seur python library in the code. |
Another thing: is this only valid for Spain? If so, you should rename the module with the l10n_es_ prefix, and we have to think about placing this here, or in the l10n-spain repo. |
############################################################################## | ||
from openerp import models, fields, api, exceptions | ||
from openerp.tools.translate import _ | ||
from seur.picking import Picking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the import of the Seur library. And i use it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I didn't see it. Then you should protect the import following this guideline: https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#importerror
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General external dependencies guideline: https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#external-dependencies
The truth is I do not know if it's just for Spain. |
For the deliver types, I would say it's only for Spain, so you should rename it to |
SEUR also operates at least in Portugal. |
But services name (SEUR 24 h, SEUR 8:30) seems very specific of Spain, doesn't it? |
Hi @ismaelcj , maybe this PR can help you to have less code |
class StockPicking(models.Model): | ||
_inherit = 'stock.picking' | ||
|
||
seur_service_code = fields.Selection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may use carrier option mechanism to have a cleaner interface
https://github.com/OCA/carrier-delivery/blob/8.0/delivery_carrier_label_postlogistics/delivery.py#L76
res.append(('seur', 'SEUR')) | ||
return res | ||
|
||
seur_config_id = fields.Many2one('seur.config', string='SEUR Config') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is configuration, wouldn't it be better to have a configuration menu ?
Could those config be setup on company, here is an example:
https://github.com/OCA/carrier-delivery/blob/8.0/delivery_carrier_label_postlogistics/company.py
https://github.com/OCA/carrier-delivery/blob/8.0/delivery_carrier_label_postlogistics/res_config_view.xml
https://github.com/OCA/carrier-delivery/blob/8.0/delivery_carrier_label_postlogistics/res_config.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you mean. It has a menu in Settings > Configuration > Carriers > SEUR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually configs are a transientmodel as it gather data from multiple origin and are linked to a company.
Here you seems to have a config shared by all companies that you can set on a specific delivery method.
In other delivery services we have a single account per company. With management of options on the delivery method.
Is there a reason to have multiple config for one company ?
Nice to see we are reaching spain with this repository, welcome here @ismaelcj Thanks for your contribution. I seems you need to add a python dependency "genshi" for travis Also, it seems this module needs testing. Could you add some? |
Hi, @yvaucher About your comment:
It was an error on the API setup.py. I proposed the changes in the API repository, and are already applied: About the tests, I will to do it. |
The genshi dependence continuous falling in travis, I don't understand. |
@@ -35,6 +35,7 @@ install: | |||
- pip install unidecode | |||
- pip install pycountry | |||
- pip install suds-jurko==0.6 | |||
- pip install seur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tried to install genshi
before seur
?
pip install genshi
pip install seur
I start to migrate this module to version 10, and i found two problems:
I will create new PR for version 10 |
…amount Add expected amount and probability fields on building project
@ismaelcj This one is really old. Do you plan to fix it ? |
Me not. Maybe @liebana or @hugosantosred or @kikopeiro |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Seur module for generate: