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

sale_order_type improvements #333

Merged
merged 6 commits into from
Oct 4, 2016
Merged

sale_order_type improvements #333

merged 6 commits into from
Oct 4, 2016

Conversation

eLBati
Copy link
Member

@eLBati eLBati commented Sep 5, 2016

ADD Payment Term, Pricelist and Incoterm to sale order type

ADD sale type for invoices



class AccountInvoice(models.Model):
_inherit = 'account.invoice'

sale_type_id = fields.Many2one(
Copy link
Member

Choose a reason for hiding this comment

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

Better to bind this to account journal instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pedrobaeza, journal contains accounting specific informations. Sale Type is more like a Line of business. That is, for the same journal, I could configure several lines of business: I should not be forced to create a journal for every line of business

Copy link
Member

Choose a reason for hiding this comment

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

OK

@pedrobaeza
Copy link
Member

Please only one module per PR

@eLBati
Copy link
Member Author

eLBati commented Sep 5, 2016

@pedrobaeza
made another PR at #334

Thanks

@pedrobaeza
Copy link
Member

Please check CIs status

@eLBati
Copy link
Member Author

eLBati commented Sep 5, 2016

@pedrobaeza travis should be ok now

@pedrobaeza
Copy link
Member

Please rebase to fix runbot and can you check if you can improve the test coverage?

@eLBati
Copy link
Member Author

eLBati commented Sep 6, 2016

@pedrobaeza thanks, done

@pedrobaeza
Copy link
Member

👍

@StefanRijnhart
Copy link
Member

Thank you, but can you describe these new features in the module's README?

@eLBati
Copy link
Member Author

eLBati commented Oct 3, 2016

@StefanRijnhart description improved

@petrus-v
Copy link

petrus-v commented Oct 4, 2016

LGTM 👍

@atchuthan
Copy link
Member

👍

@tafaRU tafaRU merged commit 8f5d1d1 into OCA:8.0 Oct 4, 2016
if self.type_id.pricelist_id:
self.pricelist_id = self.type_id.pricelist_id.id
if self.type_id.incoterm_id:
self.incoterm = self.type_id.incoterm_id.id

Choose a reason for hiding this comment

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

This method will invoke multiple times the write method of the sale.order object (potentially this can happen up to 6 times). I believe it would be better, as also advised in the official Odoo documentation, to gather all the values that need to be updated in a dictionary and to explicitly invoke the write method once for all in the end. This for performance reasons.

Copy link
Member

Choose a reason for hiding this comment

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

@espo-tony, I agree.

Could you please make a PR on branch 8.0?
Thank you in advance.

Choose a reason for hiding this comment

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

@tafaRU , I'm sorry I made a mistake:

Being this method decorated with the onchange decorator, it will not trigger the write method of the object, instead all the work will be done in the cache. No performance issue in this case.
I'm sorry for wasting your time.

Copy link
Member

Choose a reason for hiding this comment

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

@espo-tony, no problem! I also did not see the onchage decorator 😉

This pull request was closed.
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.

7 participants