Controllers structure - a cleanup strategy

Tech
1

This is the current controllers structure in OFN:

20200712_210704
20200712_2107041836×3264 1.75 MB

If you ignore history, it’s a total mess! If you know that half of the things were stuck in Spree for a long time, it makes sense that this is where we ended up.
But now that we are independent of Spree, there are quite a few things we can improve here :tada:

I think we can target a very simple strucuture:

  • keep the API side of things as is
  • ApplicationController - code shared between frontoffice controllers and backoffice controllers
  • BaseController - code for FrontOffice controllers
  • Admin::BaseController - code for BackOffice controllers

The steps to get there could be:

  • rename Spree::Admin::BaseController to Admin::BaseController
  • merge Spree::BaseController with ApplicationController and make sure that doesn’t break the frontoffice controllers that inherit from BaseController
  • make Controllers under StoreController inherit from BaseController instead and remove StoreController
  • there are quite a few methods in ApplicationController that can be moved to BaseController because they are frontoffice specific…

Would be good to get feedback about this.

1 Like
2

It makes sense but I would like to answer a few questions first:

  • What logic does StoreController implement? why was it needed in the first place? Do its children need something in particular?
  • Does Spree::BaseController have any logic that might clash with OFN’s BaseController where front-office ones inherit from?
3

image

It’s basically for frontoffice controllers that need the current_order.

I dont think so, it’s just helpers.
This is how it will look like when brought from spree_core to OFN in PR 5733. link

4

ok, after this: https://github.com/openfoodfoundation/openfoodnetwork/pull/6279
project complete!

This is how it looks like after this last PR (if you dont understand: it’s much better now!!):

20210115_221327
20210115_2213274000×3000 2.86 MB

2 Likes
5

Thanks a lot for pushing this forward @luisramos0! we can archive this then, right?

6

yes, this is done :slight_smile: