GET requests for checkout

[Ethan Rowe]

I'm looking at Spree::Checkout to show some proof-of-concept stuff based
on an earlier thread[1], and while looking noted that GET requests are
unsafe.

From lib/spree/checkout.rb, starting around line 18:
> # and now do the over-write, saving any new changes as we go
> @order.update_attributes(params[:order])

This happens to @order within the checkout action regardless of the HTTP
request type.

It seems to me that the block of code from line 10 or so through line 20
ought to be relocated within the "if request.post?" block. A GET
request typically ought not be changing the state of the thing it's
requesting.

Comments?

Thanks.
- Ethan
--
Ethan Rowe
End Point Corporation
et...@endpoint.com

[Ethan Rowe]

Ethan Rowe wrote:
> I'm looking at Spree::Checkout to show some proof-of-concept stuff based
> on an earlier thread[1], and while looking noted that GET requests are
> unsafe.

Gah. Sorry about that.
[1]
http://groups.google.com/group/spree-user/browse_thread/thread/a355cbed136c7864

[Sean Schofield]

> It seems to me that the block of code from line 10 or so through line 20
> ought to be relocated within the "if request.post?" block.  A GET
> request typically ought not be changing the state of the thing it's
> requesting.
>
> Comments?

That's a good point. I think we were differentiating between GET and
POST with POST being for the final submission. The stuff we are
sending as GET should still update the order, its just that we should
probably use a different controller method to POST to instead.

Finally got Shoulda working (there was a key omission[1] from the
README that had me stuck.) Once I get the test written for the
duplicate credit cards maybe we can build these tests out and start
messing with the checkout logic.

> - Ethan

Sean

[1] http://github.com/railsdog/shoulda/commit/632b0452d813957a92bc08873158e020d3db3af8

[Ethan Rowe]

Sean Schofield wrote:
>> It seems to me that the block of code from line 10 or so through line 20
>> ought to be relocated within the "if request.post?" block. A GET
>> request typically ought not be changing the state of the thing it's
>> requesting.
>>
>> Comments?
>
> That's a good point. I think we were differentiating between GET and
> POST with POST being for the final submission. The stuff we are
> sending as GET should still update the order, its just that we should
> probably use a different controller method to POST to instead.

What information is provided through get requests? Since the purpose of
a get request is to be a simple "read", if there's data exchanged from
client to server for storage purposes ought that not be a post (or a
Rails faux put)?

In any case, I just walked through the checkout process and reviewed the
logs afterward; the only GET to the checkout action was the initial
entry into it, which provided nothing in params[:order]. All subsequent
requests to the checkout action were POST requests. I didn't try every
possible path through (different user registration choices, using
billing address for shipping, etc.), but I don't think those particulars
would change the result.

So, the @order.update_attributes(params[:order]) thing shouldn't
actually happen in GET given the present use from what I see. But
that's somewhat coincidental; placing anything in the :order param of
the GET request would cause an attempt to update attributes. To be a
RESTful process (or, just leaving aside REST and staying true to the
semantics of HTTP), the conditional update_attributes call "ought" to be
left out of a non-POST request.

> Finally got Shoulda working (there was a key omission[1] from the
> README that had me stuck.) Once I get the test written for the
> duplicate credit cards maybe we can build these tests out and start
> messing with the checkout logic.

Earlier this week I started some refactoring along the lines of what I
discussed in an earlier thread[1] at:
http://github.com/ethanrowe/spree/tree/checkout_refactor

The point here was to show in code a first step of what that thread
proposed, as I don't think discussion of it will be very clear or
productive unless the discussion is conducted in Ruby. :)

That branch has only a very modest change at present, but it notably
introduces unit testing for lib/spree/checkout.rb. At present it's
really only focusing on the things I've touched in my refactoring thus
far. But it's a start. I'll try to do some more on it today and post
again here to explain what it's about.

[1]
http://groups.google.com/group/spree-user/browse_thread/thread/a355cbed136c7864

[Paul Callaghan]

I've just commited a potential fix to #463 (on a branch in my fork) which addresses some of these issues, particularly simplifying what happens with cards and shipments, and giving a clean separation between GET and POST/PUT functionality. Ethan's improvements can be added on top of this.

Please see http://support.spreecommerce.com/issues/show/463

Comments please?

Paul

[Ethan Rowe]

Paul, I started reviewing the changes, but they're fairly extensive and
touching on crucial pieces of order operations. So I want to free up a
larger chunk of time (beyond the 10 minutes I had this moment) to give
it a proper, thoughtful look. I'll try to get back to this a little
later today.

Thanks.
- Ethan

[Sean Schofield]

Same here. I'll need more time to review. While we're discussing
REST I was thinking we might want to consider a Checkout model and
controller to make things more RESTful.

Order has_one :checkout.
Order has_one :billing_address :through => :checkout
...

In any event, we should discuss more and then write up a ton of tests
for testing our final answer as well as future refactorings.

Thoughts?

Sean