-
-
Notifications
You must be signed in to change notification settings - Fork 301
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] 9.0 saleorder_import_invoice2data and [MIG] sale_commercial_partner / sale_order_import #18
[ADD] 9.0 saleorder_import_invoice2data and [MIG] sale_commercial_partner / sale_order_import #18
Conversation
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.
@Kiplangatdan requesting changes
sale_order_import/__openerp__.py
Outdated
'category': 'Sales Management', | ||
'license': 'AGPL-3', | ||
'summary': 'Import RFQ or sale orders from files', | ||
'author': 'Akretion,Odoo Community Association (OCA)', | ||
'author': 'Akretion,Odoo Community Association (OCA), Sunflower IT', |
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.
If done properly I think we should be second in the list
partner_change_res = dict( | ||
(key, value.id if not isinstance(value, int) else value) | ||
for key, value in dict(fake_so._cache).iteritems() | ||
) |
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.
There must be a prettier way to do this, the issue was that dict(fake_so._cache)
returns the objects in a dict, whereas we need the id's.
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.
Actually have found it in @hbrunn's commits, see comment below
|
||
<record id="sale_order_import_form" model="ir.ui.view"> | ||
<field name="name">sale.order.import.form</field> | ||
<field name="model">sale.order.import</field> | ||
<field name="arch" type="xml"> | ||
<form string="Import Sale Orders"> | ||
<group name="technical" invisible="1"> | ||
<group string="technical" invisible="1"> |
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.
@Kiplangatdan Why was it necessary to change all the name
occurrences to string
or group
? IMO name
is the preferred way to make the object accessible to inheriting views, and it's commonly used in Odoo 9.
If the aim was to add a caption, you can leave the name
and add string
separately.
@@ -0,0 +1,100 @@ | |||
=================================== | |||
Sale Order Import Saleorder2data |
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.
Invoice2data
Maintainer | ||
---------- | ||
|
||
This module is maintained by Sunflower IT and Therp BV. |
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.
Maybe this should be the OCA
instead.
}) | ||
new_line._onchange_product_id() | ||
vals = { | ||
f: new_line._fields[f].convert_to_write(new_line) |
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 think we should use this as an example to clean up this
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.
this is slightly more complicated, here the fixed version: https://github.com/OCA/edi/pull/11/files#diff-57b8c6799922b0e67a34d15dc8b010a8R142
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 made it into a helper function:
def get_values_for_create(instance):
""" Helper function to get values from object as dict """
return {
f: instance._fields[f].convert_to_write(v)
if not isinstance(instance._fields[f],
fields._RelationalMulti)
else v.ids
for f, v in instance._cache.iteritems()
}
We should also fix these style issues |
sql_str = super(SaleReport, self)._select() | ||
sql_str += ', s.commercial_partner_id as commercial_partner_id' | ||
return sql_str | ||
# def _select(self): |
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 seen that Odoo 9.0 already puts partner.commercial_id
in this view, so this part can indeed be removed.
@thomaspaulb I just changed my proposal for the invoice line parsing, this now is a lot more robust and gives better results. You will have to change your templates though, but the new format is a lot simpler to write |
Alright, we will change the template for the test accordingly. Do you have
an example or is it clear from the code?
Op 8 mei 2017 18:52 schreef "Holger Brunn" <notifications@github.com>:
… @thomaspaulb <https://github.com/thomaspaulb> I just changed my proposal
for the invoice line parsing, this now is a lot more robust and gives
better results. You will have to change your templates though, but the new
format is a lot simpler to write
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18 (comment)>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/ABZf9KxMQhWJFl1cxLW4CXZAKAZYN6zeks5r30g9gaJpZM4NPQgr>
.
|
logger.debug( | ||
'Calling invoice2data.extract_data with templates=%s', | ||
templates) | ||
testspath = os.path.dirname(os.path.realpath(__file__)) |
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.
there's an API for this: https://github.com/OCA/OCB/blob/9.0/openerp/modules/module.py#L142
But in the end, I think we need a base module that provides a model
class Invoice2dataTemplate(models.Model):
type = fields.Selection([]) # other modules like yours add a type of template, as in invoice_import, sale_order_import, etc
template = fields.Text('The template')
This makes it much more convenient to deliver templates in your module.
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.
Is this something you have done in the invoice version of this module already?
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.
no, not yet. For now, just remove this part of the code and set your template path in the test. Then it's very simple to change this later to use the yet-to-be-written module, and the test template will not interfere with normal operations
'taxes': line.get('taxes'), | ||
'unit_price': line.get('unit_price'), | ||
'price': line.get('price'), | ||
} |
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 think it would be nice to add the intersection of line.keys()
and self.env['sale.order.line']._fields.keys()
to the line values too. This way, you simply have to use odoo names in your templates, and you get whatever exotic field you might want to use without having to write custom code
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.
in my own override of this, I use
common_fields = set(self.env['sale.order.line']._fields.keys()) &\
set(parsed_line.keys()) - set(order_line.keys())
for field in common_fields:
order_line[field] = parsed_line[field]
this way, you can support arbitrary fields without having to write custom code
logger.info('Sale Order ID %d created', order.id) | ||
return order | ||
|
||
class BusinessDocumentImport(models.AbstractModel): |
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.
one class per file
return parsed_order | ||
|
||
@api.model | ||
def create_order(self, parsed_order, price_source): |
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.
what's this override good for? Looks suspiciously like you just copied https://github.com/OCA/edi/blob/9.0/sale_order_import/wizard/sale_order_import.py#L212
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.
Suspicions confirmed.
# return self.invoice2data_parse_saleorder(file_data) | ||
# | ||
@api.model | ||
def parse_pdf_order(self, file_data, detect_doc_type=False): |
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 destroy modularity here. Do something similar to what account_invoice_import does in sale_order_import
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 am calling super now in case I am not happy with the PDF, I hope that solves the issue
@@ -152,6 +152,7 @@ def parse_pdf_order(self, order_file, detect_doc_type=False): | |||
# }] | |||
|
|||
@api.model | |||
@api.multi |
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.
it's either model
or multi
, not both
84561ba
to
a2eeffd
Compare
1 similar comment
1 similar comment
closing this in favor of #21 |
@hbrunn Tests are running but code needs review