Free Shipping Bug

[Rob LaRubbio]

I'm trying to track down a bug in getpaid that seems to allow a user to get free shipping.  I've been able to get to a point were I can reproduce it, now I'm looking for pointers or help on why it is occuring.

I've got a simple setup of Plone 3.1.6 with GetPaid 0.7.9 (I'm using the svn checkout of the buildout).  I've marked a couple of items as shippable and have written my own shipping addon that talks to USPS.

The free shipping issues happens when a user walks through the checkout process up to the 'checkout-review-pay' step.  At this point if the user hits the 'Back' button provided on the page (not the browser back), they return to the 'checkout-select-shipping' step with a hidden element that holds their previous shipping choice.  This input element is the same as the input radio select on the page.  This shouldn't be a problem since I would expect to get both the hidden value and the radio select in an array, and it looks like the code expects that as well since the getShippingMethodCode() does this:

    if isinstance( service_code, list):
        service_code = service_code[-1]

  however it seems I'm getting an empty array instead of an array with two values.  For some clarity the checkout-shipping-select html essentially looks like this:

    <form action="https://nwei.org/@@getpaid-checkout-wizard"
          method="post" enctype="multipart/form-data">
         <input type="hidden" name="shipping_method_code" value="usps.D-16"></div> 

         <I removed all the other hidden inputs>

         <input checked type="radio"
                     name="shipping_method_code"
                     value="usps.D-16" /> 
               
          <input type="submit" class="button context"
                     id="form.actions.continue"
                     name="form.actions.continue" value="Continue" />
    </form>

As far as I can tell the code should work.  I'm not clear on why self.request.get('shipping_method_code') is [] on the second submit instead of ['usps.D-16', 'usps.D-16']?  Also if I submit the form, then hit 'back' again it works ok since there is no hidden element for an empty array added.

Anyway if anyone has some suggestions on where to look I would really appreciate it.  Thanks.

-Rob

[Juan Carlos Coruña]

Hi,

I'm also trying to track down the same bug. It's a strange behaviour one time it works ok and the next time it fails, and so on...

I have used the pdb and I followed the code till PloneGetPaid/_patch.py and there I get lost. I suspect the bug is in _patch.py, but I cannot assure it. Maybe this can be clue for others trying this. I'll continue to track down this bug. If I make some progress I will inform about it.

2009/6/23 Rob LaRubbio <laru...@gmail.com>

[Juan Carlos Coruña]

Hi,

I think I understand the strange behaviour. I'll try to explain it:

The first time you select a shipping the request.form['shipping_method_code'] doesn't exist, hence it will be created with the value you have selected in the wizard (request.form['shipping_method_code'] == 'usps.D-16'). In the next wizard getpaid gets the shipping cost using the value from the request. This time it works. When you go back from this wizard to the previous one (to the one you selected the shipping) the request.form['shipping_method_code'] exists already with the previous selected value. Now you select the shipping and continue. Getpaid doesn't create the  request.form['shipping_method_code'] because it already exists, so it APPENDS (creating a list) the new value (request.form['shipping_method_code'] == ['usps.D-16', 'usps.D-16']). In the next wizard Getpaid tries to get the shipping cost using the value from the request, but this time resulting in NO success, because the value is a list of two strings and doesn't match with any of the shipping methods. So getpaid sets the cost to 0 and the shipping method to none. The next time you go back the request.form hasn't the key shipping_method_code, so it works as in the first time.

I don't know where getpaid accomplishes all this, but I have a patch that correct this strange behaviour. I know it's not the perfect solution, but for now it works. Maybe someone that knows getpaid better than me can correct the bug in the origin.

Here is the patch:

--- Products/PloneGetPaid/checkout.py.orig      2009-06-23 23:42:42.000000000 +0000
+++ Products/PloneGetPaid/checkout.py   2009-06-23 23:25:11.000000000 +0000
@@ -714,6 +714,8 @@
         and returns a list of them for the template to display and the user to choose among.

         """
+        try: del(self.request.form['shipping_method_code'])
+        except: pass
         siteroot = getToolByName(self.context, "portal_url").getPortalObject()
         ship_service_names = IGetPaidManagementOptions(siteroot).shipping_services

2009/6/23 Juan Carlos Coruña <ogg...@gmail.com>

[Rob LaRubbio]

Sorry for the slow reply on this.  My gmail stopped forwarding to my normal email so I missed this.

After digging into this I came to the same conclusions and an almost identical patch.  The problem goes a little deeper then the variable being a list since there is code in the checkout process to handle a list:

def getShippingMethod( order, service_code ):
    """ decode a shipping code, and return the shipping method to be used or None """
    if not service_code:
        return None

    if isinstance( service_code, list):
        service_code = service_code[-1]

What I couldn't figure out is why the list is actually getting deleted at some point in the processing.  As far as I can tell the request variables are held in two places in the request.  Once as key/value pairs directly on the request, and in a dictionary called form on the request.  It looks like some parts use just request and others use request.form so there is code that copies values from request.form to request and that is blanking out the value.  What I can't figure out is why the value is blank in one but not the other. 

So long story short I went with a similar patch to yours:

def make_hidden_input(*args, **kwargs):
    '''Construct a set of hidden input elements, with marshalling markup.

    If there are positional arguments, they must be dictionaries.
    They are combined with the dictionary of keyword arguments to form
    a dictionary of query names and values.

    Query names (the keys) must be strings.  Values may be strings,
    integers, floats, or DateTimes, and they may also be lists or
    namespaces containing these types.  All arguments are marshalled with
    complex_marshal().
    '''

    # There has to be a better way, but I haven't found it.  Essentially
    # Allowing this to be a hidden variable results in an array coming
    # back in the request when the user starts to move through the process
    # again.  Somewere that populated array is replaced with an empty array
    # setting the shipping cost to 0.  This only happens if the site has
    # shipping set up and the user hits 'Back' from the review and pay step
    if kwargs.has_key('form.shipping_method_code'):
        del kwargs['form.shipping_method_code']

I'll be checking in my 'fix' shortly. 

However I notice that the Products.PloneGetPaid repository seems to have changed.  I'll check my mails to see if it was intentional, but when I did an update this morning I got what looks like a new src and trunk directory in src/Products.PloneGetPaid.  Both seem to contain the complete set of code, so now I seem to have three copies.  (src/Products.PloneGetPaid/src, src/Products.PloneGetPaid/trunk & src/Products.PloneGetPaid/Products)

-Rob

[Rob LaRubbio]

I just wanted to start this thread up again.  This issue exists in the bug tracker as issue #274.  Also since I sent this mail I rolled back my fix since it actually had the effect of always producing free shipping.

Juan, I"ll add your patch to my local buildout and test it.  If anyone has thoughts or ideas for the real fix I'd love to hear them.

-Rob

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

[Rob LaRubbio]

Juan,  I added your patch into my local buildout, but it looks like it results in shipping always being free.  Apparently setupShippingOptions in CheckoutSelectShipping (Producst.PloneGetPaid/browser/checkout.py line 738) is called both before the shipping choices are shown, and after it is selected.  

I've noticed that the shipping plugin is called multiple times.  That is a separate issue, but it really slows down the checkout process, especially if your shipping module makes use of a remote web service (like USPS or UPS).

If anyone else has ideas or suggestions I'd love to hear them.  Thanks

-Rob

[Juan Carlos Coruña]

Rob, you are right, the patch doesn't work. I remember it worked.

If you change the patch this way it should work. Change the try...except clause with:

ship_method_code = self.request.form.get('form.shipping_method_code')
if type(ship_method_code) == type([]):
  self.request.form['form.shipping_method_code'] = \
    self.request.form['form.shipping_method_code'][-1]

I don't like this solution. We should find the place where getpaid appends the shipping_method to the existing form attribute form.shipping_method_code.

2010/1/13 Rob LaRubbio <laru...@gmail.com>

--
GetPaid for Plone: http://www.plonegetpaid.com (overview info) | http://code.google.com/p/getpaid (code and issue tracker)

You received this message because you are subscribed to the Google Groups "getpaid-dev" group.
To post to this group, send email to getpa...@googlegroups.com
To unsubscribe from this group, send email to getpaid-dev...@googlegroups.com

For more options, visit this group at

http://groups.google.com/group/getpaid-dev?hl=en?hl=en

[Rob LaRubbio]

Yep, that change looks better.  (although I agree the real issue is still in the code) I'll check it in and rebundle a new version.  Thanks.

-Rob

2010/1/14 Juan Carlos Coruña <ogg...@gmail.com>

[David Glick]

Rob LaRubbio wrote:
> Yep, that change looks better. (although I agree the real issue is
> still in the code) I'll check it in and rebundle a new version. Thanks.

Darn, you're too fast... we need new releases of
getpaid.SalesforcePloneFormGenAdapter and
getpaid.SalesforceOrderRecorder too. I can make them if you give me
access on pypi (davisagli)
thanks,
David

[Rob LaRubbio]

Crap, I just bundled getpaid.recipe.release.  

I've given you permissions on the two Salesforce packages.  Let me know if you want permissions on the getpaid recipe.  I can also bundle the new version of that if you want.

-Rob

--

[David Glick]

Rob LaRubbio wrote:
> I've given you permissions on the two Salesforce packages. Let me
> know if you want permissions on the getpaid recipe. I can also bundle
> the new version of that if you want.

Thanks. I'll try to get to this tomorrow.
David

[David Glick]

On Thu, Jan 21, 2010 at 2:44 PM, Rob LaRubbio <laru...@gmail.com> wrote:

Crap, I just bundled getpaid.recipe.release.  

I've given you permissions on the two Salesforce packages.  Let me know if you want permissions on the getpaid recipe.  I can also bundle the new version of that if you want.

I finally made a new release of getpaid.SalesforceOrderRecorder this evening. Turns out the changes in getpaid.SalesforcePloneFormGenAdapter were already released. I updated the versions of both in the recipe, but I'll let you or Lucie release it.

thanks,

David 

[danielle davout]

oups !
in Products.PloneGetPaid 0.8.6 no more pt and zcml files ...