Proposal: tax processors, by_product vs. by_price

[Nan]

Hi folks...

I'm building a custom tax processor for a store where each product has
both an Area and a Tax Class, and tax on that product depends on both
(rather than on the store's or the purchaser's location).

The trouble is that a lot of tax calculations in Satchmo seem to
depend on the by_price method, which passes in the Tax Class for the
product, but not the product object itself -- so the processor has no
way to determine the product's Area; most tax calculations will
therefore fail.

The by_product method seems to exist on most tax processors but
remains essentially unused[1]. I'm considering patching Satchmo to
use by_product in most places where by_price is used to calculate tax
specifically on a product. Would there be any broader community
interest in such a patch?

Thanks,
-Nan

[1] http://groups.google.com/group/satchmo-users/browse_thread/thread/6bb6707c11e0de16/465e58d3535e8618?lnk=gst

[hynekcer]

Hi Nan,

You can create many tax classes (several for every area) if one area
is bind to the product (us-high us-low uk-high uk-low uk-...) and
create a model for your tax processor with the necessary data. The
area part of the taxclass can be eventually automatically filled by a
custom Widget for Admin and pre_save signal of your product.
You
What is "Product Area"? (You sell houses?) It does ot matter. I think
it is not much typical requirement. How to say, that something is
unused in a toolbox by users?

More desiderable for Satchmo community I consider first to simplify:

- create a class BaseProcessor(object):
# which defines relations between derived methods and the method
by_price which is the only by_* which shold be defined by every
descendant class Processor(BaseProcessor)
def by_price(self, taxclass, price):
raise NotImplementedError()
# This can simplify the code without breaking the compatibility
and it would be easier to create a new tax module.
One class can be easier documented. Descendants need not it. You are
right, the name by_price is not inuitive

- a documented example module with more functionality than 'no' or
'percent' and simpler than 'area' or 'us_sst', with less database
queries.

'by_product' can be defined by 'by_price', but not viceversa.

The Product is a complicated model with submodules. Method
taxer.by_price is called also from productvariation_details. With
taxer.by_product it would be not as easy to say that it can not cause
any recursion, memory cycle etc. Simpler is often better.

Hynek

> [1]http://groups.google.com/group/satchmo-users/browse_thread/thread/6bb...

[Nan]

> You can create many tax classes (several for every area) if one area
> is bind to the product (us-high us-low uk-high uk-low uk-...) and
> create a model for your tax processor with the necessary data. The
> area part of the taxclass can be eventually automatically filled by a
> custom Widget for Admin and pre_save signal of your product.

With products in almost every US state, we'd need a Tax Class for
every product class in every state PLUS a Tax Rate for each Tax Class
for each state, AFAICT. It's just too confusing, when the tax
processor itself should be able to simply select the Tax Rate based on
an attribute of the product. Then we can have just a few tax classes
(e.g. virtual goods, clothing, other), and a tax rate for each of
those for each state.

> What is "Product Area"? (You sell houses?) It does ot matter. I think
> it is not much typical requirement. How to say, that something is
> unused in a toolbox by users?

Yes, it's rare, but real estate is probably similar. We sell tourism
products, and have been advised that each product needs to be taxed
according to the location it relates to.

> More desiderable for Satchmo community I consider first to simplify:
>
> - create a class BaseProcessor(object):
>   # which defines relations between derived methods and the method
> by_price which is the only by_* which shold be defined by every
> descendant class Processor(BaseProcessor)
>     def by_price(self, taxclass, price):
>         raise NotImplementedError()
>     # This can simplify the code without breaking the compatibility
> and it would be easier to create a new tax module.
> One class can be easier documented. Descendants need not it. You are
> right, the name by_price is not inuitive
>
> - a documented example module with more functionality than 'no' or
> 'percent' and simpler than 'area' or 'us_sst', with less database
> queries.
>
> 'by_product' can be defined by 'by_price', but not viceversa.

The trouble is that (a) by_price doesn't receive any data about the
product other than its tax class and a price; and (b) calls to
by_price are *hard-coded* into a lot of places in Satchmo where
by_product would be equally appropriate.

I need a way to pass the product to the tax processor, and the only
way for me to do that is to call by_product instead of by_price.
Working around this without patching Satchmo itself would basically
mean copy-pasting a quarter of satchmo's code, replacing single method
calls in a couple dozen places, and then finding awkward ways to hook
it in.

In the tax processors distributed with Satchmo, by_product simply
calculates the price and then calls by_price. So patching Satchmo to
change those calls shouldn't mean any disruption to people using those
processors.

[Nan]

To follow up, I think your idea of a base class is a really good one.
I'm going to actually try something similar:


1) Create a new BaseProcessor and update the existing Tax Processors
to inherit from it

2) Include a new by_product_and_price method in BaseProcessor, which
defaults to simply calling by_price

3) In locations where by_price is currently called but product data is
available, change the calls to by_product_and_price


This likely could be accepted into Satchmo trunk without any
disruption of existing sites.


Then I can just write a custom processor that inherits from
tax.modules.area and overrides the by_product_and_price method and the
_get_location method.

[hynekcer]

OK. How to do it?

I think that more people wants better performance in their situations
(when you read this forum) and reliability than extending the
complexity.

The name by_product_and_price sounds redundant, hmm, but it is not.It
can be useful because price calculation is an expensive operation and
should not be unnecessarily repeated. It is also useful to keep tax
modules and discount methods independent and preferably simple. Method
by_product is not usable for undiscounted prices. Method get_rate
looks universal and it is basic method for two complicated tax
processors, but it should be internal method because it is not known
out of tax processor which context parameters are mandatory for that
processor.

Why are too much tax.by_* methods? May have been attempts to solve
rounding problems with 2 decimal places arithmetics with specialized
methods for one and total. Finally, it was decided at commit 166 three
years ago to save all 10 decimal places. Then it became less
important. It was before issues history.

> So patching Satchmo to change those calls shouldn't mean
> any disruption to people using those processors.

How do you ensure it? How will the new method by_product_and_price
help you ?

The big problem is with functions which depends on taxer and call only
by_price:
shop.models.OrderItem.update_tax
payment.forms._get_shipping_choices
product.utils.productvariation_details
(All 3 templatetags satchmo_discounts.taxed_* can be eventually
replaced without changing Satchmo or templates.)

Some deep stored methods depends on it.
shop.models.OrderItem.save

product.modules.configurable.models.Configurable.add_template_context
payment.forms.SimplePayShipForm.__init__
Also product itself depends on that. This does look nice at all.

Normal fuction can be dirty patched in a private project by
manipulation with sys.models['modelname'].__dict__['function_name'],
but to do it with django.db.models children would be a harakiri.

This is a mystery now
if self.product.taxable:
self.unit_tax = processor.by_price(taxclass,
self.unit_price)
self.tax = processor.by_orderitem(self)
After you verify internals and simplify existing tax processors it can
be probably also simplified without by_orderitem and make it
deprecated.


These approximately 6 lines with by_price can be replaced by:
- .... taxer.by_price(product.taxClass, price)
+ if hasattr(taxer, 'need_product_detail'):
+ .... price * taxer.get_rate(product=product)
+ else:
+ .... taxer.by_price(product.taxClass, price)

and you can use it soon.

While it will not make any problems some time (exactly the same
results, speed, database queries and memory requirements for normal
taxed projects), it can be something done to be eventually unified in
BaseProcessor and some redundant methods declared as deprecated and
not used in a new development.

[Nan]

I think the best way to explain my patches to Satchmo is to actually
show them to you:

https://bitbucket.org/ringemup/satchmo

The patch is complete, is very simple, and passes all tests. The
additional processing is minimal; by_product_and_price acts
identically to by_price in the existing processors; and by_price is
used directly for shipping and other situations where there is no
product in question.

My next step is to write the custom tax processor; I can use the code
in that to demonstrate to you what it accomplishes and why it needs
the product.

[Chris Moffitt]

To be honest, the patch looks pretty good to me. I haven't thoroughly reviewed it but it appears to be a good implementation.

Ideally, I'd like some docs on how to implement a custom tax processor so we can get something in place. Other than that I'd like people to review but it looks like a reasonable approach.

-Chris

--
You received this message because you are subscribed to the Google Groups "Satchmo users" group.
To post to this group, send email to satchm...@googlegroups.com.
To unsubscribe from this group, send email to satchmo-user...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/satchmo-users?hl=en.

[Nan]

Heh, I'd love to have some documentation to work from, too! I'll
mostly be duplicating tax.modules.area and working from [1], but will
jot some notes in the process, and hopefully at least be able to add
to what exists. I'll be starting in on that some time Monday.

If you'd like documentation for the BaseProcessor in my patch, I'll
try, but TBH, I'm not entirely clear yet on what each of those methods
is used for, or where it's called from.

[1] http://groups.google.com/group/satchmo-users/browse_thread/thread/6bb6707c11e0de16/465e58d3535e8618?lnk=gst

On Mar 18, 11:12 pm, Chris Moffitt <ch...@moffitts.net> wrote:
> To be honest, the patch looks pretty good to me. I haven't thoroughly
> reviewed it but it appears to be a good implementation.
>
> Ideally, I'd like some docs on how to implement a custom tax processor so we
> can get something in place. Other than that I'd like people to review but it
> looks like a reasonable approach.
>
> -Chris
>

[Nan]

I've added some skeleton documentation to the branch:

https://bitbucket.org/ringemup/satchmo

On Mar 18, 11:28 pm, Nan <ringe...@gmail.com> wrote:
> Heh, I'd love to have some documentation to work from, too!  I'll
> mostly be duplicating tax.modules.area and working from [1], but will
> jot some notes in the process, and hopefully at least be able to add
> to what exists.  I'll be starting in on that some time Monday.
>
> If you'd like documentation for the BaseProcessor in my patch, I'll
> try, but TBH, I'm not entirely clear yet on what each of those methods
> is used for, or where it's called from.
>

> [1]http://groups.google.com/group/satchmo-users/browse_thread/thread/6bb...

[hynekcer]

Nan,
Nan,
It is very good, that you wrote also .txt documentation for the
taxprocessor.
I think, it is better to remove long implementation details, even then
the code can change, but the interface remain the same for long time.
For the user is important only to differentiate, that some method is
empty and should be defined, some has a self-explanatory one line code
and other is more complicated.

I created a ticket #1288 [1] for this purpose.

I wrote some comments, example module, not ready. [2]
I moved tax/modules/base/processor.py to tax/modules/processor.py to
be the dependency more evident.
It would be better to start with a minimal safe code with garanteed
compatibility as wrote you and the code which wrote I let under
development and testing so that old modules would not much dependent
on the new code and new modules need not be done by big copy-paste and
simplified interated version of old modules would be accessible as an
option to verify, whether are equivalent to the last penny.

Nan, you forgot to change
self.tax = processor.by_orderitem(self)
The name "by_price_and_product" is longer than I can read by a glance
and with the additional parameter it is much less readable then the
original.
What about "by_price_obj"? (By price and some not trivial object, the
price is usually just the only important and the used keyword
"product=" explains everything.)


[1] https://bitbucket.org/chris1610/satchmo/issue/1288/tax-processors-refactor
[2]

On Mar 19, 4:28 am, Nan <ringe...@gmail.com> wrote:
> Heh, I'd love to have some documentation to work from, too!  I'll
> mostly be duplicating tax.modules.area and working from [1], but will
> jot some notes in the process, and hopefully at least be able to add
> to what exists.  I'll be starting in on that some time Monday.
>
> If you'd like documentation for the BaseProcessor in my patch, I'll
> try, but TBH, I'm not entirely clear yet on what each of those methods
> is used for, or where it's called from.
>

> [1]http://groups.google.com/group/satchmo-users/browse_thread/thread/6bb...

[Nan]

Thanks for touching up my work. I know I've got a lot to learn,
especially about producing good documentation.

> It is very good, that you wrote also .txt documentation for the
> taxprocessor.
> I think, it is better to remove long implementation details, even then
> the code can change, but the interface remain the same for long time.
> For the user is important only to differentiate, that some method is
> empty and should be defined, some has a self-explanatory one line code
> and other is more complicated.

Do you propose that instead of the sample processor in the .txt doc,
we have an explanation of each of the methods? Or is that already
covered by the more thorough docstrings you added?

Is there anything else that should be covered in the .txt doc that
isn't?

> Nan, you forgot to change
> self.tax = processor.by_orderitem(self)

Where was this change omitted?

> The name "by_price_and_product" is longer than I can read by a glance
> and with the additional parameter it is much less readable then the
> original.
> What about "by_price_obj"? (By price and some not trivial object, the
> price is usually just the only important and the used keyword
> "product=" explains everything.)

I have no objections to renaming it -- what's important to me is that
we're actually passing the product in the calls to it, and making
calls to the new method instead of directly to by_price.

[hynekcer]

Nan,
I think that text documentation is very useful.
A zero tax example is not much helpful. Commands "return 0" do not
make difference between 0 and 100 * 0.
It does not reveal to the reader, whether the matter have been
understand correctly. Example with fixed taxes 5% / 10% inland
clients and 6% / 12% abroad clients or 7% / 14% the product "Egypt
tour" for all clients, this example can be very short and much better
to understand, why can be useful the paramerer product.

-- begin a short inspirative example
from tax.modules.processor import * # this imports also Decimal
without try/else
class Processor(BaseProcessor):
method="YOUR_MODULE_NAME"
def by_price_and_product(self, taxclass, price, product=None):
percent = Decimal(str(product.name == 'Egypt tour' and 14 or \
(self.order.bill_country.name == 'Hawai' and 10 or
12)))
return price * (percent / 100) / (taxclass.name != 'food' and
1 or 2)
-- end

> > Nan, you forgot to change
> > self.tax = processor.by_orderitem(self)

> Where was this change omitted?

Here
if self.product.taxable:
- self.unit_tax = processor.by_price(taxclass,
self.unit_price)
+ self.unit_tax = processor.by_price_and_product(taxclass,
self.unit_price, product=self.product)
self.tax = processor.by_orderitem(self)

> I have no objections to renaming it...

I am not sure, what shorter to use instead of name
"by_price_and_product". We can decide it later. Maybe someone has an
idea.

Thanks. I have a language handicap.

[Nan]

> > > Nan, you forgot to change
> > > self.tax = processor.by_orderitem(self)
> > Where was this change omitted?
>
> Here
>          if self.product.taxable:
> -            self.unit_tax = processor.by_price(taxclass,
> self.unit_price)
> +            self.unit_tax = processor.by_price_and_product(taxclass,
> self.unit_price, product=self.product)
>              self.tax = processor.by_orderitem(self)

Sorry, still confused. Which file / class is that in?

[hynekcer]

I looked to your changeset https://bitbucket.org/ringemup/satchmo/changeset/20ebf1e5ccf0
I had noticed it there.
The command
grep -r "processor.by_orderitem" path/to/your/satchmo
says that it is
satchmo/apps/satchmo_store/shop/models.py
in the context, what you modified.

Please, verify then, that results in all templates are these correct
new with the respect to product area. It shold not introduced bugs in
Satchmo by the patch.
If possible, use first those parts from my BaseProcessor required by
short tax processor examples.

[Nan]

Thanks. I'll take a closer look tonight and make the changes. I'll
also pull in the changesets from your branch (still working out how to
make git and hg place nice).



On Mar 24, 11:34 am, hynekcer <hy...@sdb.cz> wrote:
> I looked to your changesethttps://bitbucket.org/ringemup/satchmo/changeset/20ebf1e5ccf0

[Nan]

Ah, I see what you're referring to. It's in the by_orderitem methods
of the existing tax processors.

I didn't think it was necessary to change in those processors because
their calls to by_product_and_price are simply redirected to by_price
anyway. I was more concerned with the calls in parts of the code
(such as template tags) that wouldn't be directly overridden by a new
tax processor class that used the product in its calculations.

Do you think it's cleaner to change those calls anyway? I'm
undecided.


As for verifying that the tax processor output is unchanged, I have
done so. Do we need to write new test cases?

Meantime, I'll contribute a sample tax processor that uses
by_product_and_price.

On Mar 24, 11:34 am, hynekcer <hy...@sdb.cz> wrote:

> I looked to your changesethttps://bitbucket.org/ringemup/satchmo/changeset/20ebf1e5ccf0

[Nan]

Cleaner version of the patch (many thanks to hynekcer), with tests and
preliminary docs, now posted at [1].

[1] https://bitbucket.org/ringemup/satchmo-clean/changeset/e1ffd15fda4b