Update Gemfile for Spree 2.0 #2209

HugsDaniel · 2018-04-12T13:11:38Z

What? Why?

Closes #2219. First attempt to boot the app with Spree 2.0.

sauloperez · 2018-04-13T13:38:43Z

Gemfile


 # Our branch contains two changes
 # - Pass customer email and phone number to PayPal (merged to upstream master)
 # - Change type of password from string to password to hide it in the form
-gem 'spree_paypal_express', github: "openfoodfoundation/better_spree_paypal_express", branch: "spree-upgrade-intermediate"
-#gem 'spree_paypal_express', github: "spree-contrib/better_spree_paypal_express", branch: "1-3-stable"
+# gem 'spree_paypal_express'


I'd remove this line. If it's commented out it's because we don't need it. If we ever do, Git will have in its history.

sauloperez · 2018-04-13T13:39:24Z

app/controllers/base_controller.rb

@@ -37,7 +37,7 @@ def set_order_cycles

    # And default to the only order cycle if there's only the one
    if @order_cycles.count == 1
-      current_order(true).set_order_cycle! @order_cycles.first
+      current_order( create_order_if_necessary: true ).set_order_cycle! @order_cycles.first


no need for this extra spacing inside the parenthesis.

sauloperez · 2018-04-13T13:39:33Z

app/controllers/spree/orders_controller_decorator.rb

@@ -18,7 +18,7 @@

  # Patching to redirect to shop if order is empty
  def edit
-    @order = current_order(true)
+    @order = current_order( create_order_if_necessary: true )


here as well

sauloperez · 2018-04-13T13:39:39Z

app/controllers/spree/orders_controller_decorator.rb

@@ -78,7 +78,7 @@ def populate
    # callbacks on Spree::Adjustment and then manually invoke Spree::Order#update! on success.

    Spree::Adjustment.without_callbacks do
-      populator = Spree::OrderPopulator.new(current_order(true), current_currency)
+      populator = Spree::OrderPopulator.new(current_order( create_order_if_necessary: true ), current_currency)


sauloperez

⚠️ Let's all be aware that it'll take some PRs until we get the tests to pass again. Basically, the whole upgrade process so we will have to apply this exception meanwhile.

Although we don't know how big the fixes introduced between 2.0.4 and 2.0.13 are, jumping from roughly 1.3.99 to 2.0.13 seems too big of a change to do in one go. Unless more issues like this one come up that force us to upgrade to a newer version I believe it's best to be conservative. You'll find an in-depth explanation in https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-2.0-upgrade#getting-to-version-204- A previous effort was done to upgrade to v2.0.13 in openfoodfoundation#2209

mkllnk

Sorry, I'm late to the party, just getting familiar with the Spree upgrade process. In order to avoid merge conflicts later on, I would like to suggest to split changes like this into two parts. Part one is pure re-factoring that can be applied to the stable master branch (not spree upgrade). The purpose is to contain Spree dependency in a small confined area. Part two is a contribution for the Spree upgrade where we can change only a few lines. This pull request is a great example. We see a lot of changes like:

-    @order = current_order(true)
+    @order = current_order(create_order_if_necessary: true)

Instead of this change, I would create our own wrapper. In this case we can create two methods on the application controller:

current_order_or_new
current_order_if_exists

In part one, these two methods just call current_order(true) or current_order(false). A lot of controllers are touched for this part, but we can easily merge it into master and improve it's readability already.

In part two, we can adapt to the Spree upgrade by touching only the two methods in the application controller without touching all the other ones.

What do you think guys? Should we do it?

enricostano · 2018-04-19T08:57:17Z

@mkllnk yes, during the Spree upgrade (but not only) we constantly struggle to find the less disruptive strategy and the "wrapper" thing is something we're taking in account all the time. The idea is to take advantage of the Spree upgrade to define our own OFN business logic abstraction and then internally we can plug into Spree or make our own thing.

In any case, yes. This PR could have included that OFN abstraction but in this specific case we didn't find a better reason than possible merge conflicts. But we'll need to question ourselves in any PR and ask other devs how to achieve it.

sauloperez · 2018-04-20T12:59:48Z

Sorry @mkllnk, I don't understand either what you mean with a wrapper nor the explanation about the two parts.

I'm spending a bit of time while working on #2205 to find out a way to make this whole dependency on Spree a bit easier to manage.

mkllnk · 2018-04-23T01:14:19Z

@sauloperez I meant that we are changing a lot of calls to Spree's current_order here. My idea was to create our own version of current_order and name it slightly differently. Then we call our own method everywhere. We can do this change in the master branch to separate our code from Spree's code a bit more. In the Spree upgrade, we just need to change our implementation of our current_order method in one place without changing all the other files. That would prevent merge conflicts. With this way of encapsulation it's easier to deal with API changes in Spree.

kirstenalarsen · 2018-04-23T04:42:33Z

separate our code - YEAH!!!

…

On 23 Apr 2018, at 11:14 AM, Maikel ***@***.***> wrote: @sauloperez <https://github.com/sauloperez> I meant that we are changing a lot of calls to Spree's current_order here. My idea was to create our own version of current_order and name it slightly differently. Then we call our own method everywhere. We can do this change in the master branch to separate our code from Spree's code a bit more. In the Spree upgrade, we just need to change our implementation of our current_order method in one place without changing all the other files. That would prevent merge conflicts. With this way of encapsulation it's easier to deal with API changes in Spree. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2209 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACxryLwv3n9yLYU0yuCX6rPjbyi3nr7Pks5trSrsgaJpZM4TRwzU>.

sauloperez · 2018-04-23T09:58:58Z

that's an interesting idea @mkllnk. That's essentially implementing an adapter. I haven't considered the nice side effect of cutting down the possibility of merge conflict and that's a major win!

This might not be the place to have an in-depth discussion about OOP design but I've been considering the possibility of building our own extendible_spree gem (hosted in rubygems) to implement well-designed hooks that allow extending it in a clean way, like we need in OFN, as opposed to the hackish and unmaintainable class_eval approach they favour.

I feel like we need a series of design/architecture session to better address the spree upgrade.

daniellemoorhead · 2018-04-27T06:59:47Z

I feel like we need a series of design/architecture session to better address the spree upgrade.

@sauloperez I say set up a call and invite all the people (aka @oeoeaio @mkllnk alongside of you guys working on Spree already). I think it's super important, and worth running potentially fortnightly catchups with our timezone to talk through things. IMO 😀

sauloperez · 2018-04-27T07:34:25Z

totally agree @daniellemoorhead . I want to prepare first a code proposal to base our discussions on.

sauloperez · 2018-05-09T10:20:19Z

We finally had that call. Read about it in the Team catch ups wiki page.

HugsDaniel added 2 commits April 11, 2018 16:56

update gemfile to spree 2.0.13

f3f6708

Remove ssl deprecated configuration

46775ab

HugsDaniel changed the title ~~Spree upgrade~~ Update gemfile for Spree 2.0 Apr 12, 2018

HugsDaniel changed the title ~~Update gemfile for Spree 2.0~~ Update Gemfile for Spree 2.0 Apr 12, 2018

enricostano added the pr-wip label Apr 12, 2018

HugsDaniel added 2 commits April 12, 2018 19:29

Refactor Order#current_order argument

ae93520

Removing redundant curly braces

03173d4

sauloperez suggested changes Apr 13, 2018

View reviewed changes

Removing useless spaces

d81b6be

sauloperez approved these changes Apr 13, 2018

View reviewed changes

sauloperez merged commit f7a5019 into openfoodfoundation:2-0-stable Apr 16, 2018

This was referenced Apr 16, 2018

Remove comment in gemfile #2226

Merged

Upgrade dependencies to make OFN work with Spree 2.0 #2219

Closed

HugsDaniel mentioned this pull request Apr 17, 2018

Refactor Order#current_order extension #2207

Closed

sauloperez mentioned this pull request Apr 18, 2018

Upgrade Spree to 2.0.4 instead of 2.0.13 #2235

Merged

mkllnk reviewed Apr 19, 2018

View reviewed changes

HugsDaniel deleted the spree_upgrade branch June 4, 2018 06:41

sauloperez mentioned this pull request Jun 5, 2018

Spree 2.0: Fix GET "/" #2358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Gemfile for Spree 2.0 #2209

Update Gemfile for Spree 2.0 #2209

HugsDaniel commented Apr 12, 2018 •

edited by sauloperez

Loading

sauloperez Apr 13, 2018

sauloperez Apr 13, 2018

sauloperez Apr 13, 2018

sauloperez Apr 13, 2018

sauloperez left a comment

mkllnk left a comment •

edited

Loading

enricostano commented Apr 19, 2018

sauloperez commented Apr 20, 2018

mkllnk commented Apr 23, 2018

kirstenalarsen commented Apr 23, 2018 via email

sauloperez commented Apr 23, 2018 •

edited

Loading

daniellemoorhead commented Apr 27, 2018 •

edited

Loading

sauloperez commented Apr 27, 2018

sauloperez commented May 9, 2018

Update Gemfile for Spree 2.0 #2209

Update Gemfile for Spree 2.0 #2209

Conversation

HugsDaniel commented Apr 12, 2018 • edited by sauloperez Loading

What? Why?

sauloperez Apr 13, 2018

Choose a reason for hiding this comment

sauloperez Apr 13, 2018

Choose a reason for hiding this comment

sauloperez Apr 13, 2018

Choose a reason for hiding this comment

sauloperez Apr 13, 2018

Choose a reason for hiding this comment

sauloperez left a comment

Choose a reason for hiding this comment

mkllnk left a comment • edited Loading

Choose a reason for hiding this comment

enricostano commented Apr 19, 2018

sauloperez commented Apr 20, 2018

mkllnk commented Apr 23, 2018

kirstenalarsen commented Apr 23, 2018 via email

sauloperez commented Apr 23, 2018 • edited Loading

daniellemoorhead commented Apr 27, 2018 • edited Loading

sauloperez commented Apr 27, 2018

sauloperez commented May 9, 2018

HugsDaniel commented Apr 12, 2018 •

edited by sauloperez

Loading

mkllnk left a comment •

edited

Loading

sauloperez commented Apr 23, 2018 •

edited

Loading

daniellemoorhead commented Apr 27, 2018 •

edited

Loading