satchmo payment and shipping signal enhancements

Create issue
Issue #1351 open
Former user created an issue

I found it quite complicated while trying to implement some custom functionality on payment and shipping modules, that could easily be done with some small modifications to the core-code (including a new signal). After applying this changes, I was able to implement everything I needed (modify payment and shipping-options based on shipping country/state) and add a 2% discount (of the order's total) if a bank-transfer payment is selected.

This would be the patch to achieve all this. There are still some TODOs to be thought of, before merging this into trunk... but I wanted to get some feedback first.

{{{ #!diff diff --git a/satchmo/apps/payment/forms.py b/satchmo/apps/payment/forms.py --- a/satchmo/apps/payment/forms.py +++ b/satchmo/apps/payment/forms.py @@ -133,11 +133,28 @@

 shipping_options = [(key, rendered[key]) for cost, key in sortme]
  • shipping_choices_query.send(sender=cart, cart=cart,
  • Listeners may return an updated kwargs if needed.

  • ret would then contain a list of tuple pairs [(receiver, response).

  • This allows a very easy fine-tunning, based on signals and in

  • combination with the tiered-shipping-module, to modify

  • the shipping options based on contact-infos like ship_country/state,

  • payment method, etc.

  • ret = shipping_choices_query.send(sender=cart, cart=cart, paymentmodule=paymentmodule, contact=contact, default_view_tax=default_view_tax, order=order, shipping_options = shipping_options, shipping_dict = shipping_dict)
  • TODO: hard-coding the sender's name is far from a "good" idea...

  • maybe it would be a better approach to loop through all responses and look for the

  • shipping_options and shipping_dict? But what if there are more than one responses?

  • args = [t[1] for t in ret if t[0].name == 'modify_shipping_choices']
  • if args and args[0]:
  • if 'shipping_options' in args[0]:
  • shipping_options = args[0]['shipping_options']
  • if 'shipping_dict' in args[0]:
  • shipping_dict.update(args[0]['shipping_dict']) + return shipping_options, shipping_dict

diff --git a/satchmo/apps/satchmo_store/shop/models.py b/satchmo/apps/satchmo_store/shop/models.py --- a/satchmo/apps/satchmo_store/shop/models.py +++ b/satchmo/apps/satchmo_store/shop/models.py @@ -999,12 +999,17 @@

     log.debug("Order #%i, recalc: sub_total=%s, shipping=%s, discount=%s, tax=%s",
         self.id,
  • moneyfmt(item_sub_total),
  • moneyfmt(self.sub_total), moneyfmt(self.shipping_sub_total), moneyfmt(self.discount), moneyfmt(self.tax))

  • self.total = Decimal(item_sub_total + self.shipping_sub_total + self.tax)

  • this signal is usefull to add additional order discounts or changes to order's total (like for payment methods)

  • #TODO: it would be nice if multiple discounts could be added to show them on the order totals
  • signals.satchmo_order_total_calc.send(self, order=self) +
  • changed item_sub_total -> self.sub_total, to be able to change it in the signal and use it here

  • self.total = Decimal(self.sub_total - self.discount + self.shipping_sub_total + self.tax)

     if save:
         self.save()
    

diff --git a/satchmo/apps/satchmo_store/shop/signals.py b/satchmo/apps/satchmo_store/shop/signals.py --- a/satchmo/apps/satchmo_store/shop/signals.py +++ b/satchmo/apps/satchmo_store/shop/signals.py @@ -247,6 +247,26 @@ #satchmo_shipping_price_query.send(order, adjustment=shipadjust) satchmo_shipping_price_query = django.dispatch.Signal()

+#: Sent by the order during the calculation of the total. +#: +#: Listeners may modify the order's total by using: +#: order.sub_total, order.discount, order.shipping_sub_total, order.tax +#: +#: An example of usage would be, to add an aditional 3% disocunt on the order's total +#: for a given payment. The listener would look something like this: +#: +#: def add_payment_discount(sender=None, **kwargs): +#: """ add 2% discount of total if paying with bank_transfer """ +#: order = kwargs.get('order', None) +#: if not order: +#: return +#: bank_transfer = sender.payments.filter(payment='BANK_TRANSFER') +#: if bank_transfer: +#: discount = (order.sub_total - order.discount + order.shipping_sub_total + order.tax) * Decimal('00.02') +#: order.discount += discount +#satchmo_order_total_calc.send(order, order=order) +satchmo_order_total_calc = django.dispatch.Signal() + #: Sent to determine where to redirect a user for a DownloadableProduct. #: #: :param sender: None

}}}

Comments (6)

  1. Hynek Cernoch

    Does it really work for you? Your example is incomplete, not only related what you noted in TODO comments. I see no "connect" for satchmo_order_total_calc and no usage of shipping_choices_query.

    "Hardcoding sender" or not what you discuss is not a question related to "send" but to "connect".

    It is not good to do discount which look like wrong calculated total, without writing the line with discount on invoice. It is better to do all discounts before calculating tax, otherwise you get wrong tax. For additional charges it would be even tax evasion. I think you should add a negative item to the order or maybe better to add an item with zero price but with a discount which you need. For different tax classes you probably need more such discount lines and create a smart templatetag to get a sum.

    Yet it seems to me confused.

    Future recommendation: Use dispatch_uid to prevent repeated applying of the same discount in the case of repeated import of module with connect in some user configuration etc. This is a basic requirement for discounts

  2. Former user Account Deleted

    Hi Hyneck. Yes it's working and in production.

    I put in the comment the listener as an example. I didn't see the need to post an example of a connect line too, but it would look something like this, if we take the same listener-function from the example:

    #somewhere in a models.py, e.g. localsite/payment/models.py
    from satchmo_store.shop.signals import satchmo_order_total_calc
    from localsite.payment import listeners as payment_listeners
    ...
    satchmo_order_total_calc.connect(payment_listeners.add_payment_discount)
    

    I only posted the parts, that I had to change on the core. And it's not very much (if we take the comments out) because, as I said, I wanted to keep it to a minimum :-)

    I don't get that you mean with: '"Hardcoding sender" or not what you discuss is not a question related to "send" but to "connect".'

    I'm sending the signal on the core-code, then connecting (in my local-app) to this signal and on the listener (also locally) changing some stuff, after that I wan't these new shipping options to get back to the form. If I don't update the shipping-options there, I would have to overwrite a lot of files locally. So the thing with the TODO and the hard-coding is, I want to update the shipping options "after" the listener has done its job, or else the form will still be getting the "old" shipping-options. So what I'm trying to do is to make a "plugin-point" available, to be able to change these (through the signal) and then the form should get updated shipping-options, in case they have changed. I know it's far from ideal, and I'm would love to find an even more generic solution to this.

    To clarify things a bit, these would be a real example of the listener for the shipping-options-signal, where depending on a group of states, islands or oversea-locations, the shipping-mode is then defined (what i'm actually doing here, is just leaving one of the shipping options and removing the other ones). The shipping-options (pricings, etc) are defined on the tiered-shipping-module, and I'm using the Key on the listener to select "the" right one for this user depending on the shipping address:

    # somewhere on localsite/payment/models.py ...
    shipping_choices_query.connect(payment_listeners.modify_shipping_choices)
    
    # somewhere on localsite/payment/listeners.py
    def modify_shipping_choices(sender, **kwargs):
        """ remove options depending on shipping address """
        if not 'shipping_options' in kwargs or not 'order' in kwargs:
            return
        key = ""
        order = kwargs['order']
        # case international, not spain or islands
        int_es_states = ['Santa Cruz de Tenerife', 'Las Palmas', 'Ceuta', 'Melilla']
        logger.debug("modify_shipping_choices: %s, %s" % (order.ship_country, order.ship_state))
        if order.ship_country != 'ES':
            logger.debug("shipping international")
            key = 'SHIPPING_INTERNATIONAL'
        elif order.ship_state in int_es_states:
            logger.debug("shipping canarias, ceuta y melilla")
            key = 'SHIPPING_SPAIN_CANARIAS'
        elif order.ship_state == "Islas Baleares":
            logger.debug("shipping to Baleares")
            key = 'SHIPPING_SPAIN_BALEARES'
        else:
            logger.debug("shipping national")
            key = 'SHIPPING_SPAIN'
    
        options = kwargs['shipping_options']
        options = [t for t in options if t[0] == key]
        kwargs['shipping_options'] = options
        return kwargs
    
    

    In the example for the payment-discounts, which just to make it clear, has nothing to do with the shipping options-example above, as it's only depending on the payment method, I'm using the modified signal to calculate a new discount.

    Here the client gets an "Skonto"-Discount if the payment method is "Prepay" (I don't know if that's the right word in engl.) This Skonto means a Discount on the final order-total. And the taxes are calculated accurately of course!!! My client doesn't want to get in trouble with the financial office ;-) The 2%-discount can be actually calculated after or before applying taxes and at the end you get the same. E.g: If 10 is the order's subtotal and lets say we have here 19% taxes, then the total would be: 10 *1.19 = 11.9 If I now want to apply a 2% discount it's the same if a say: Skonto_total = 11.9 * 0.02 = 0.238 -> T2 = 11.662 What is the same as: skonto_sub = 10 * .02 = 0.2 10 - 0.2 = 9.8 -> 9.8 * 1.19 = 11.662

    Which looks a little confusing, is that i'm using the discount, to add this skonto-discount and so that at the end I get the same as the calculation above. But if you make some examples through you'll see, it's correct. I did A LOT of examples to make sure everythiing was beeing calculated right.

    And in the invoice and every relevant checkout screen, this "extra"-disocunt is also showed to the client. The discount in this case would be a sum of the product-disocunts (if any) AND the additionally 2% skonto, which I calculated on the listener, and added it to the discount. So the client gets everything shown on the discount line.

    Of course, I completely agree, if there were more than one disocunt-line... That was what I wrote on the "#TODO: it would be nice if multiple discounts ..."-comment but, that needed a lot more changes on the core than just 1-2 lines.

    Thanks a lot for your feedback, but probably we're focusing to much on this one example instead of the changes to actually be done on the core-part. As everybody will have some completely different problems and special needs, I wanted to propose something, that can make people life easy in trying, like me, to change things easy without changind the core-part. So I would love to get to some solution generic and simple enough to get it into the core.

    Thanks Andrea

  3. Hynek Cernoch

    Thanks.
    I think that the right core solution for your issue should also solve the issue https://bitbucket.org/chris1610/satchmo/issue/1381/discount-system-brainstorm . Try to take it into account whether your core patch is sufficient.

    Every enhancement needs not only the patch to core but also an explanation and maybe a very short example how it is useful. Nobody other could use your patch without information which you considered unimportant because nobody would understand what is the string "modify_shipping_choices" which is only once used in your patch. Imagine that users should not need hunt for your changeset, they would see primarily the documentation and could search something in the source, but unsuccessfully because your patch without an example is scattered like an well hidden backdoor. :-)

    I did not understand your comment in the source last time:

    "# TODO: hard-coding the sender's name is far from a "good" idea..."

    (I tried to guess what you meant and viceversa you did not understand me. Aha, you hard-coded not sender name but listener name.)

    I think that if you want be minimalist:
    No change is necessary in satchmo/apps/payment/forms.py because you can modify the list shipping_options and the dictionary shipping_dict directly the listener without using result return command.

    A right place for sending the new signal is between the lines

            self.sub_total = full_sub_total
    
            taxProcessor = get_tax_processor(self)
    

    If you want to change also tax deliberately, do it later how you want but before logging. The name "satchmo_order_total_calc" is not too helpful. Better is "satchmo_order_recalculate_end" or "satchmo_order_recalculate_pre_save".

    It is not exactly right that
    "Listeners may modify the order's total by using: order.sub_total, order.discount, order.shipping_sub_total, order.tax."
    Listeners can modify more things but then must be replicated more code from force_recalculate_total.

    The patch needs to rethink all, including comments.

  4. Andrea de la Huerta

    ... mhhh. Ok, if I remember correctly, I actually tried just to change the shipping_options and shipping_dict directly in the listener, but it didn't work. So that's the reason why I ended up with the returning-args-solution and updating after the signal has been sent and looking for the hardcoded-listener... and that's of course the part that makes it all messy. So if I/we can solve that issue, it would be perfect.

    Yes, you're absolutely right about the missing infos in my examples. I'll fix that next time.

    I'll have to take a good look again to everything (my old patch and your suggestions) but I'm working on other projects and have something between no time at all and absolutely no time :-) So I hope in about 2-3 weeks things will look better and I'll make me a new test-repository to test all this.

    Thanks again. Andrea

    P.S. Is there another way to communicate with you guys, without having to fill up the issues or the mailing-lists in case I just have to ask some trivial questions while working in a new patch? Chat or something like that?

  5. Hynek Cernoch

    Andrea,
    -*assigning* other object or modified original object to the parameter which you got (a new value to shipping_options or shipping_dict) has no effect outside but you can *modify* a mutable object parameter and it has an effect outside. The dictionary and the list are mutable. So you can do:

    shipping_options.append(something)
    shipping_dict[somekey] = something
    

    It is not nice or pythonic because a function should not have side effects but some listeners typically have them and if you write the right comment, it is much cleaner than what you did.

    P.S.
    I can not use chat because a have a weak memory :-) or I must often continue after longer pause and thinking I consider more healthful than talking. :-) Mailing lists can be anoying for more people then an issue tracker. Long issues are anoying only if they stay abandoned unfinished for long time. This is a privilege for people who are doing useful things. Everything above helps to estimate whether the contributor thought enough about the problem. It also helps new contributors if they casually read some old closed issues.

    If you are careful you can use a repository with more heads. After I had to much Satchmo repositories it was easier to learn how to do all development experiments only in one repository, push only one head etc. Unnecessary private heads can be removed by "hg strip". (which can be googled)

  6. Alexander Clausen

    Another thing to note is that the example signal listener might break, depending on the payment processors in use. After the user chooses a payment method, a pending payment is created. So if a user first selects payment in advance ("banktransfer"), then goes back and chooses, for example, PayPal, the pending payment from the first selection is still in the database and the check

    bank_transfer = sender.payments.filter(payment='BANK_TRANSFER')
    if back_transfer: ...
    

    will evaluate to True and grant the user the discount.

    Also, force_recalculate_total doesn't appear to be called from all payment processors, particularly the PayPal confirmation view doesn't trigger a recalculation, also resulting in a "sticky" discount.

    Just throwing these out there for others trying this "quick&dirty" approach.. :)

  7. Log in to comment