Bye bye Spree :-) (or Spree v2.1 and beyond)

Tags: #<Tag:0x00007f2190728f68>

My preference is also to remove Spree as dependency. :slight_smile:

I agree with @maikel that it would be simple.

We might have to spend some time figuring out incompatible gem versions and gem version quirks, but I suspect a lot of these issues will be common with Spree so we can use the later Spree versions as reference.

I browsed through the release notes (this is not complete).

There are changes we do not need that look like they would take time to investigate and adapt for OFN (e.g. refactoring of adjustments, return authorizations).

But there are also features for the API and performance improvements which we might actually want. There are many random changes too from which I think OFN could benefit, even if they are not in the OFN wishlist.

I think it’s a shame if the team went through all the trouble of changing OFN to be compatible with the rewrites in Spree 2.0, but then not take advantage of the random improvements in later minor versions with which OFN code is already compatible.

Question: What do you think about us spending time going through the rest of Spree 2.x PRs, and picking which ones are nice to have but at the same time sensible to bring into the OFN fork of Spree? Again, even if these are not in the OFN queue. We get features and bug fixes in if they are easy to adapt, but we ignore the ones we don’t need which look like trouble. This way, we can get further value from upgrading to 2.0.

Currently, that’s my vote.

1 Like

About copying so much of Spree into the OFN code, we also need to look into the Spree license: https://github.com/spree/spree/blob/2-1-stable/license.md I don’t know how to do it, but I’m thinking we would need to get a thumbs-up from Spree, consult a lawyer, and update attribution and/or copyright.

Small example of a gray area (for me at least) is that, in this PR, we copy the spinner image from Spree into OFN. I doubt it’s technically okay, especially when we no longer use Spree.

Does anyone know this topic better?

This is great. A year ago we would have had no idea how much work it would be to kick out Spree. Now we could attempt an estimate because we learnt a lot about our dependencies.

@luisramos0 Your approach of kicking out the Spree frontend first is awesome. I always thought of Spree as one whole dependency but we can actually choose. Most of the frontend got replaced long ago. We don’t need the Spree frontend. And removing it is independent from using the rest of Spree. We don’t have to do the big exit decision now. We can do one step, observe and evaluate.

I still think that there is a lot of experience and wisdom in the modelling of Spree. Maybe we are not happy with the implementation but there are some interesting concepts. Maybe we will hang on to parts of Spree core for years to come. For example, if we implement The Network better and remove variant overrides, we could be happy using some original Spree objects like products and variants. But that seems far away at the moment.

As long as we use Spree code and we need to upgrade Ruby and Rails, it’s good to see how they dealt with these upgrades because we probably need to do something similar.

@kristinalim’s idea of cherry-picking is interesting. I don’t think it’s ideal because once we started ignoring some commits it will limit our selection of other patches. There will probably be a lot of dependencies within the code. But once we decided that we won’t go beyond a certain point, we can still look beyond that point to cherry-pick. I see that as a valid strategy later on when we decided where to quit.

Licensing is also interesting. As the license says, we need to keep the copyright notice in all Spree code. But we should be fine then. There are no other conditions for code. We should probably avoid importing binaries like images. In case of the spinner, it may well be that Spree doesn’t own it either. I wouldn’t worry about that one.

Great discussion! :heart:

----- there is a lot of experience and wisdom in the modelling of Spree

yes, I agree but I think we have learnt what had to be learnt (and copied what had to be copied) and now we can do our thing. The spree order class is 600 lines and our decorator is 400 lines, 1000 (very smart) lines of code!!! This is horrible design at the core of OFN: we need a long term solution.

I think the only way out is to copy over everything we need from order.rb and refactor to what we need. So, long term (2/3 years time), I think we should move out of spree completely.

Short term (this year and next year), I agree with investing time in pushing the upgrades forward. Re cherry-picking, it’s a really good option to have in mind, lets see!

Meanwhile in the trenches… the build is green! :heart:

link to PR

2 Likes

This is getting really interesting. The topic of licensing is certaintly one we cannot miss @kristinalim. Perhaps asking them is the best we can do now.

I just want to add that separating from Spree does not mean we stop benefiting from their knowledge. We can still read how they solved the problems we face or the patches they did, but this way we get to choose. That’s the big difference.

1 Like

In the meeting in the gathering we agreed to:

  • remove dependency to spree frontend, spree auth devise, backend and api and leave spree core for later
  • upgrade to spree v2.1
  • see if the upgrade to v2.2 is easy or not (what was the improvement in terms of adjustments?)
  • remove dependency spree_core

So, our last Spree version could be one of the versions between 2.1 and 2.4

ping @kristinalim, we just agreed on what’s discussed above. Nothing new.

@luisramos0 @maikel @Matt-Yorkley @paco @sauloperez - I just added the tag gathering-19 to this post so that it comes up when looking for notes from all sessions. If you can do so for others would be great :slight_smile: Thanks

2 Likes

Now that upgrade to spree 2.1 is almost done, I spent a few weekend hours starting the upgrade to spree 2.2:

I think this would be a straight forward upgrade if it wasn’t the “Adjustments refactoring” in spree 2.2.

This refactoring is probably very good in terms of spree BUT for OFN it will be a nightmare (and obviously a lot of time of dev and of scratching our heads).
To be more specific, I invite you to look (keep scrolling down!) at the post Ryan Biggs wrote at the time about the refactoring, it’s an essay with 20 pages!!!
https://ryanbigg.com/2013/09/order-adjustments

This materializes in mystic migrations like this one that completely break OFN. And I havent even thought about EnterpriseFees which are extra adjustments we do to orders in OFN.

Currently I am estimating this upgrade in so many headaches… and so I am thinking this is it.
On the other side, I believe we can do the copy/paste exercise (bye bye spree) pretty quickly and get the rails upgrades to rails 6 at a rather quick pace.

What do you devs think? I think we should make this decision as soon as rails 4 is live.

This completely terrifies me. I get that you are saying “on the other side things will be great” and I believe you, but this looks like a chasm that we are going to continue losing large chunk of our dev capacity in for a long time. It’s hard for me to see the other side :frowning:

There is no way to do the bye bye spree before embarking on breaking all the adjustments and fees @luisramos0?

I can be more clear and less dramatic :slight_smile:
Option 1 - upgrade to spree 2.2 and adapt to the new adjustments and stuff - after this we would have to upgrade spree again and again and say byebye spree at some later stage (this painful path includes the good stuff from spree later versions)
Option 2 - say byebye spree now (copy paste what we need from spree to OFN) and start upgrading Rails on our side. Here, painful spree upgrades become the (a lot less painful) rails upgrades. We could simply focus on rails 4.2, 5 and 6.

My question is really: do we all agree it’s time for option 2?

ah yes, then I think the answer is clear :slight_smile: I agree!

I’m missing the crucial part for this decision: What do we gain with Spree 2.2 and what do we lose?

I had a quick look at Ryan Bigg’s article but didn’t read it all. I found one section about the future changes which mostly mentions reducing call backs. This is something we want, a win.

What are the changes that don’t make sense to us which would be losses?

I think we need more information to do this decision.

Hello Maikel,

pros of keeping Spree:

  • the adjustments changes are good, we get that benefit :+1:
  • we get the spree code upgraded to rails 6 if we go up to Spree 4
  • we get all the benefits of the improvements in the spree code

cons of keeping with Spree:

  • compared to the huge investment of time these spree upgrades represent, the benefits of the PROs above are ridiculously small (we still have to upgrade rails on the OFN code anyway)

  • in terms of investment past and future:

    • past - I have spent 9 months working on the spree 2.0 upgrade and spree 2.1 took a month for me and maybe a few weeks for Matt
    • future - we have 13 spree upgrades to rails 6 (I think it’s a good assumption they will be between 1 month and 9 months in size): spree 2.2, 2.3 (rails 4.1), 2.4, 3.0 (rails 4.2), 3.1, 3.2 (rails 5.0), 3.3 (rails 5.1), 3.4, 3.5, 3.6(rails 5.2), 3.7 (rails 5.2), 4.0 (rails 6) and 4.1
  • if we say byebye spree now (I think it’s a fairly quick task similar to removing spree_backend #4050) we can then focus on upgrading rails: 4.1, 4.2, 5.0, 5.1, 5.2, 6.0

  • in terms of investment on rails upgrades - I’d say these 6 rails upgrades to rails 6 are at the very least half the (very large) size of the 13 spree upgrades above. By leaving Spree now we are cutting the upgrade to rails 6 time in half - I think loosing the improvements of the spree code from 2.2. to 4.1 is a comparatively low price we should pay.

  • also, after we leave Spree, we will immediately have a much better code base without decorators that we can refactor and improve, unlike the current core code that is difficult to refactor due to being dependent on Spree.

In a way this is a project management decision to get to rails 6 faster (or just to make it possible). Seems pretty obvious to me now that the faster we jump off spree, the faster we get to rails 6.

I believe I would choose also option 2. But I think @maikel’s approach is right and I wonder how many Spree improvements are hidden inside each Spree upgrade.
But not only around technical changes, maybe also for the general UI? If I remember correctly the big backend UI improvement comes with Spree 3. Would this be something worth to copy-paste or it is totally not worth it?

Rachel we only depend on spree_core that means we would only benefit from the changes to the core of spree.
Fortunately we have already left spree_backend and our messy deface customizations of it so we will not have to handle the adaptation to that :tada:

I did a short analysis of the work to remove spree in https://github.com/openfoodfoundation/openfoodnetwork/issues/4826
I think this is slightly bigger than removing spree_backend. I think spree_backend was a month of work, I think removing spree_core will take 1.5 months.
I also analyzed how difficult the upgrades to rails 4.1 and rails 4.2 will be, they look rather simple. I’d say rails 4.1 is less than a month and rails 4.2 maybe a month (I’m a bit unsure how much trouble will “nils on serialized attributes” and “order of tests” cause us).
This is just to give us some timescales to manage this work:

  • reference - upgrade to spree 2.0 - 12 months
  • reference - remove spree_backend - 4 weeks
  • reference - stripeSCA work done so far - 4 weeks
  • reference - upgrade spree 2.0 to spree 2.1 - 8 weeks
  • remove spree core - 6 weeks
  • upgrade from rails 4.0 to rails 4.1 - 3 weeks
  • upgrade from rails 4.1 to rails 4.2 - 4 weeks

Disclaimer: #guesstimates

I am adding here something that I think is important. It’s important to explain why the jump from rails 4.0 to 4.2 is important and in a way ends the massive tech debt problem we have right now:

  • Rails 4.0 was last patched in Jan2015
  • Rails 4.1 was last patched in July 2016
  • Rails 4.2 was last patched in March 2019 (EDIT: security patch rails 4.2.11.3 was released in May 2020)

@danielle asked me to chime in here so I’m doing so. I’ll preface this by acknowledging that I’m not a regular contributor to this code base so feel free to take or leave my opinions :slight_smile:

IMO, a code base should not use external libraries for its core business logic. It makes development seem quicker initially, but is a maintainability nightmare. There will inevitably be customisations required which deviate from the feature set of the external library, and that will lead to the kind of monkey-patching (“decorators”) present in OFN’s code base. OFN is an online store, so the functionality in Spree is very much part of its core business logic. This logic belongs directly in the code base for reasons of maintainability and clarity.

(I think there are strong parallels with the trade-offs discussed here: https://dashbit.co/blog/a-new-authentication-solution-for-phoenix)

So to me, the obvious choice is to vendor the Spree code in the OFN codebase and take ownership of it. You can remove all the bits of Spree that aren’t used by OFN, and get rid of all the monkey-patching which will make it a lot easier to navigate the code base. Yes, you’ll miss out on any refactoring or improvements done in future Spree versions, but who is to say that those things are actually relevant to OFN anyway? You can focus future refactorings on those which provide the most value to OFN’s code base rather than Spree’s.

2 Likes

It’s great to see you agree with this approach Jon! Thanks a lot for taking the time to comment.

Quick progress update here. This Tuesday I went for the rails 4.1 upgrade on top of spree 2.1. The build looks good and I made some progress fixing some initial issues.

To do this I relaxed the rails 4.0 requirement in spree 2.1 so that we could bump it to rails 4.1 in OFN.

The main point is that I realized we don’t need to do/finish “bye bye spree” BEFORE the upgrade to rails 4.2. We can go for the rails 4.1 and rails 4.2 upgrade without moving all spree code to our side. We can move the spree code to OFN gradually and meanwhile get the rails upgrades done :tada:
This is much better because of the time to market (bye bye spree estimate is 6 weeks), this means we are probably 3 dev weeks away from rails 4.1 and another 4 weeks away from rails 4.2.

2 Likes