Order update attributes refactor by jhawthorn · Pull Request #357 · solidusio/solidus

jhawthorn · 2015-09-04T21:30:30Z

No description provided.

jordan-brough · 2015-09-08T17:57:07Z

Looking great to me! The extraction of code into standalone classes seems like what we've been needing and the spec cleanups along the way are great to have also.

jordan-brough · 2015-09-08T18:02:06Z

What do you think of apply instead of update for the name of this method? Or something else like that. If I have a "FooAttributes" object, I kind expect foo_attributes.update to refresh the attributes themselves or something like that.

I like apply

jhawthorn · 2015-09-09T19:00:55Z

Disappointing discovery: if I comment out set_shipments_cost, all specs still pass 😞. Will have to try and add a feature test exposing that.

grzlus · 2015-09-14T21:33:37Z

I don't like setting variables in conditional statements. I propose 2 different wat how to handle it:

return {} if masseged_params[:order].blank? massaged_params[:order].permit(permitted_checkout_attributes)

update_params = massaged_params.fetch(:order, {}) update_params.permit(permitted_checkout_attributes)

Seems a bit bike-shedding-ish...

Also, in this case I like how the current form calls out the fact that we may want to discontinue supporting requests without parameters (which I find weird).

Yeah I'm with @jordan-brough and @jhawthorn on this. I think the assignment in the conditional makes a lot of sense here.

jordan-brough · 2015-10-02T14:25:07Z

This is looking pretty great! @jhawthorn do you have much more on your todo list here or do you think this is pretty close to ready?

jhawthorn · 2015-10-02T22:06:51Z

@jordan-brough I have just two things I want to check up on. The first you might be able to help clarify

Here https://github.com/solidusio/solidus/pull/357/files#diff-0a2f4b241f8427ca90d58eec529e0e8aR53 we assign credit_card.verification_value and there are tests which verify (though expect(..).to receive(:varification_value=)) that that is assigned. However that record is never saved, and I see no place that that would be used. If we need that value to be around at some point we need to test for it.

Second issue is to find out why we have set_shipments_cost

jhawthorn · 2015-10-05T20:49:02Z

I've added a test for set_shipments_cost. Other than my question to you @jordan-brough I believe this is ready

jordan-brough · 2015-10-05T21:37:09Z

@jhawthorn taking a look now

jordan-brough · 2015-10-05T22:12:01Z

@jhawthorn I can't see a definite use for the verification_value with existing cards. We definitely haven't used it ourselves.

You probably already saw this, but this is the commit that added this via the cvc_confirm parameter (which we now translate into verification_value in controller_helpers/payment_parameters.rb to be more consistent in what we call it). I couldn't find any corresponding PR or anything but it does seem to be meant specifically for use with existing cards.

I'd guess that someone was requiring their customers to re-supply the CVV before being able to re-use an existing card, and that the in-memory credit card was making its way to the payment processor code (e.g. in code like this) and that code expected the verification value to be there.

Summary: I don't know. When I moved that code around I was just trying to be extra cautious. I would be willing to bet that there are very few stores that use that param, if any.

jhawthorn · 2015-10-05T23:21:24Z

@jordan-brough thanks. I was mistaken and assumed that was added by us. Might as well leave it in for now, changing that would be ouside this PR's scope.

As of 3a5e16ab49687c7960d90bfbf8e05b09eff0ecd2 (which was horrible to track down) this is good to go by me. 🚢

magnusvk · 2015-10-07T13:48:54Z

I'm still not a fan of the fact that we define this twice even though we already have a module in place which we can use to DRY this up.

I'm still not a fan of changing this. :)

jordan-brough · 2015-10-07T13:51:53Z

@jhawthorn super! I'm going to take another look today and verify that it'll give us what we were looking for originally.

magnusvk · 2015-10-07T14:27:36Z

Can we just leave this in apply? I don't see the point of extracting one line of code that we only call once.

magnusvk · 2015-10-07T15:12:08Z

This looks great overall. Keen to still chat a bit more about OrderUpdateAttributes.

jordan-brough · 2015-10-09T17:16:44Z

@jhawthorn should this be a configurable class? e.g. Spree::Config.order_update_attributes_class.new(...)

and same question for PaymentCreate

I think that might be a good idea but is outside the scope of this PR

I think before we merge this PR it ought to be in a state that satisfies the original extension goal that we had in mind. Maybe we should chat sometime about how we could best do that with the current state of this PR?

I'll submit this also as a follow-up after this PR.

This inverse_of is now there implicitly, so it makes no sense to test that it isn't defined.

Originally I prefered accepting request, but now think that accepting request_env is cleaner as it doesn't depend on the structure of an ActionDispatch request.

Conflicts: core/app/models/spree/payment_create.rb

Order update attributes refactor

cbrunsdon mentioned this pull request Sep 8, 2015

More refactoring of update_from_params #329

Closed

jordan-brough reviewed Sep 8, 2015
View reviewed changes

jhawthorn changed the title ~~[WIP] Order update attributes refactor~~ Order update attributes refactor Sep 9, 2015

jhawthorn force-pushed the order_update_attributes branch from aaef027 to c1e5d26 Compare September 9, 2015 23:01

grzlus reviewed Sep 14, 2015
View reviewed changes

jhawthorn force-pushed the order_update_attributes branch 2 times, most recently from 8a24713 to 5938878 Compare September 30, 2015 17:42

jhawthorn force-pushed the order_update_attributes branch from 5938878 to 146d73c Compare October 5, 2015 20:48

jhawthorn force-pushed the order_update_attributes branch from 146d73c to 3a5e16a Compare October 5, 2015 23:20

magnusvk reviewed Oct 7, 2015
View reviewed changes

This was referenced Oct 9, 2015

Use OrderContents#update_cart in Api::PaymentsController#create #299

Closed

Auditing and Comments / Notes #429

Closed

jordan-brough reviewed Oct 9, 2015
View reviewed changes

jhawthorn and others added 25 commits November 4, 2015 14:48

Remove some stubbing on order

3e36b04

Better stubs in frontend checkout_controller_spec

593cd84

Add OrderUpdateAttributes class

0d36204

Fix use of request_env

5056902

Separate assignment from saving

9ad9b23

Create payments manually

d7028da

Split into PaymentCreate class

27960f2

Payment create spec

4d3c49c

Only assign request_env if specified

b39aa80

Add order_update_attributes_spec

ffb365f

Remove spec on inverse_of

d7b8c73

This inverse_of is now there implicitly, so it makes no sense to test that it isn't defined.

Add deprecations to apply_source_attributes

e631d5a

Always use PaymentCreate when creating payments

0486c55

Silence deprecations in specs

4166261

Make attr_readers private

1f0fbcb

Only accept request_env

d433b6c

Originally I prefered accepting request, but now think that accepting request_env is cleaner as it doesn't depend on the structure of an ActionDispatch request.

Revert to old update_from_params code

6c97551

Add caller to deprecation warning

d8c99b4

s/update/apply

bdefe88

Fix PaymentCreate

2def968

Assign order attributes before building payments

0e6aa6a

Refactor into available_cards

92b7baa

Add docs

b6960a1

Conflicts: core/app/models/spree/payment_create.rb

Use assign_attributes instead of attributes=

09520ec

Add test for set_shipment_cost in updater

1ab4d74

jhawthorn force-pushed the order_update_attributes branch from a49cedc to 1ab4d74 Compare November 4, 2015 22:53

jhawthorn added a commit that referenced this pull request Nov 5, 2015

Merge pull request #357 from jhawthorn/order_update_attributes

b3f7dd8

Order update attributes refactor

jhawthorn merged commit b3f7dd8 into solidusio:master Nov 5, 2015

jhawthorn deleted the order_update_attributes branch November 9, 2015 07:23

elia mentioned this pull request Feb 7, 2023

Allow providing an existing payment during the checkout #4909

Closed

3 tasks

Uh oh!

Uh oh!

Conversation

jhawthorn commented Sep 4, 2015

Uh oh!

jordan-brough commented Sep 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhawthorn commented Sep 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordan-brough commented Oct 2, 2015

Uh oh!

jhawthorn commented Oct 2, 2015

Uh oh!

jhawthorn commented Oct 5, 2015

Uh oh!

jordan-brough commented Oct 5, 2015

Uh oh!

jordan-brough commented Oct 5, 2015

Uh oh!

jhawthorn commented Oct 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jordan-brough commented Oct 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magnusvk commented Oct 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants