Tax inclusion settings per storeview #74

Merged
merged 1 commit into from Jan 6, 2015

Projects

None yet

4 participants

@mistotebe
Contributor

Closes issue #72.

@coveralls

Coverage Status

Coverage increased (+0.05%) when pulling 9baab59 on credativUK:7.0-tax-inclusion-per-storeview into fb081b8 on OCA:7.0.

@guewen guewen commented on an outdated diff Dec 16, 2014
magentoerpconnect/sale.py
@@ -780,7 +787,7 @@ def _create_data(self, map_record, **kwargs):
**kwargs)
def _update_data(self, map_record, **kwargs):
- tax_include = self.backend_record.catalog_price_tax_included
+ tax_include = self._get_price_included(map_record)
self._check_special_fields()
return super(SaleOrderImport, self)._update_data(
map_record, tax_include=tax_include,
@guewen
guewen Dec 16, 2014 Member

Can I propose a small refactoring for this PR? I won't blame you if you can't afford to change that, your work is well done and my suggestion goes further than your initial goal.

So my feeling is that having the Storeview record in the "options" of the mapper would be interesting because several mapping methods may need it. Currently, each mapping method have to do the same code:

storeview_binder = self.get_binder_for_model('magento.storeview')
storeview_id = storeview_binder.to_openerp(record.source['store_id'])
storeview = self.session.browse('magento.storeview', storeview_id)

My suggestion is to replace _get_price_included by a _get_storeview method that returns the storeview record and pass it in super(SaleOrderImport, self)._update_data() and super(SaleOrderImport, self)._create_data(). Then, the mapper could use self.options.storeview.catalog_price_tax_included instead of self.options.tax_include. (But maybe we want to keep the same value as well in self.options.tax_include just for backward compatibility).

@guewen
Member
guewen commented Dec 16, 2014

Thanks @mistotebe! Well done.

Let me know what you think about my suggestion in the diff lines.

@mistotebe
Contributor

I guess runbot is broken? I have not seen a successful build in there at all (those that are not failed seem they have not actually run at all or have never finished), also I cannot seem to access the build logs to check why it thinks it should not pass.

@guewen
Member
guewen commented Jan 5, 2015

@mistotebe yes, don't mind with runbot, it is not working correctly yet. By cons, Travis did not run, probably due to a conflict.

@mistotebe
Contributor

Do you mind if I rebase then?

@mistotebe
Contributor

(and squash, I guess)

@mistotebe mistotebe [IMP] Tax inclusion settings per storeview
f5e8969
@coveralls

Coverage Status

Coverage decreased (-0.01%) when pulling f5e8969 on credativUK:7.0-tax-inclusion-per-storeview into 0df0279 on OCA:7.0.

@guewen
Member
guewen commented Jan 6, 2015

Excellent! 👍

BTW you can add your name in https://github.com/OCA/connector-magento/blob/7.0/magentoerpconnect/AUTHORS

Thanks

@mistotebe
Contributor

Eventually, or I'll let you do that after this gets merged. No point in adding it to the merge request at this point.

@guewen
Member
guewen commented Jan 6, 2015

Ok. I just need one other person to approve in order to merge it (OCA rules).

Ping @OSguard @sebastienbeau @rdeheele

@rdeheele
Contributor
rdeheele commented Jan 6, 2015

👍
thanks @mistotebe , good work!

@guewen
Member
guewen commented Jan 6, 2015

@mistotebe I'm going to merge right now. Do you want me to add a line in AUTHORS? Do you want me to put your real name (that I don't have) or mistotebe?

@mistotebe
Contributor

@guewen, sure, why not. Looking at the file, the line should probably read "* Ondřej Kuzník at credativ"

@guewen
Member
guewen commented Jan 6, 2015

Alright

@guewen guewen merged commit f5e8969 into OCA:7.0 Jan 6, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment