New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split core into frontend+models #2225

Closed
wants to merge 213 commits into
base: master
from

Conversation

Projects
None yet
@radar
Member

radar commented Nov 20, 2012

There's been a number of situations now where people have wanted to customise the frontend of Spree so much that they've really just wanted the models. In this case, to include the frontend components with spree_core doesn't make logical sense if all people want is the models. Therefore, I've split up core into two different modules: "core" and "frontend".

Core is literally just the core of Spree. You need this in order to operate Spree. It contains the models, the mailers and nearly everything that previously lived in lib/spree/core. If you want Spree without all fat, this is it.

Frontend is the frontend of Spree, and it now has a solid dependency on API. The benefit of this dependency is so that we can gain numerous boons from using the API ourselves, such as using it for product and taxon searching in the backend. There are more things than these two basic things we could do with it.

Promo has been merged into Core and Frontend. It made no sense to have this as a separate part of the system when a) we were bundling it with everything by default anyway b) a lot of stores want/need promotions and c) the hacks that Promo had into the Order model and CheckoutController were getting ridiculous. So that's been tidied up now.

There are quite a large number of commits here (which I will be rebasing down soonish) and although I don't expect anybody to read through them all and follow my descent into madness and re-emergence, I would like some feedback on what else we could do in this pull request.

It's my intention that this forms the basis of the work for Spree 2.0.

@parndt brings up a good point that it's similar to Travis's architecture. Those are smart people working there.


Support for Ruby 1.8 has been intentionally deprecated

There are a number of gems (money, paperclip and truncate_html off the top of my head) which have either removed Ruby 1.8 support completely, or support it in a diminished capacity. This PR removes 1.8 capabilities from Spree and upgrades gem dependencies as they need to be for Ruby 1.9 goodness. We won't get a chance to force Ruby 1.9 until the next major version of Spree, and personally I'm not willing to wait that long.


TODO

  • Use API for product searching in order form 2a4c7a4
  • Use API for product searching in promotion form 2a4c7a4
  • Use API for option types in admin/product form 0e3d951
  • Use API for taxon lookup in admin/product form 5f7fdd7
  • Split out backend from frontend
  • Allow frontend + backend locales to be different (#1946)
  • Use API for product searching in promotion rule form
  • Fix order_updater_specs in Core
  • Fix zones_controller spec in API
  • [frontend] Use API for states lookup in checkout form 4f77ff0805a3e7ebc6e13329db39ed7b5e3e8839
  • [backend] Use API for states lookup in customer details form
  • Add tests for different shipping method functionality, taken out with ac28b4e34cd583f86bf913b1a9b8b710bd3a5b95 -- done with 5c7145f
  • Find where Spree.routes.product_search is being used and replace it with API end point. d55266f
  • Add tests for different promotion functionality, taken out with ef24cd2 -- done with f81ab2b
@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Nov 20, 2012

Member

👍 for merging Promo into Core. Are you thinking of merging API as well as suggested in #2090

Member

JDutil commented Nov 20, 2012

👍 for merging Promo into Core. Are you thinking of merging API as well as suggested in #2090

@msevestre

This comment has been minimized.

Show comment
Hide comment
@msevestre

msevestre Nov 20, 2012

+1. I think merging Promo in Core is a great idea. I've been waiting for
that for a long time

On Tue, Nov 20, 2012 at 3:37 PM, Jeff Dutil notifications@github.comwrote:

[image: 👍] for merging Promo into Core. Are you thinking of merging
API as well as suggested in #2090#2090


Reply to this email directly or view it on GitHubhttps://github.com//pull/2225#issuecomment-10556734.

msevestre commented Nov 20, 2012

+1. I think merging Promo in Core is a great idea. I've been waiting for
that for a long time

On Tue, Nov 20, 2012 at 3:37 PM, Jeff Dutil notifications@github.comwrote:

[image: 👍] for merging Promo into Core. Are you thinking of merging
API as well as suggested in #2090#2090


Reply to this email directly or view it on GitHubhttps://github.com//pull/2225#issuecomment-10556734.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Nov 20, 2012

Member

I love how clean the core/app and frontend/app directories are, great job.

I still think there's an argument for splitting the admin controllers, views and routes out further. Larger installs don't want the admin UI running on the same servers as the frontend for a couple of reasons:

A) Security - if the admin was running on it's own box, it could be firewall'd to controll IP access to it.

B) Memory - overtime the app server's memory footprint grows, at lot of this can be contributed to backend usage. Considering the backend has a LOT more controllers and views than the frontend, removing it could provide more "room" for the frontend.

Finally, we definitely do need to revisit the Spree::Responder functionality, it's is used in a couple of extension / stores for sure.

Member

BDQ commented Nov 20, 2012

I love how clean the core/app and frontend/app directories are, great job.

I still think there's an argument for splitting the admin controllers, views and routes out further. Larger installs don't want the admin UI running on the same servers as the frontend for a couple of reasons:

A) Security - if the admin was running on it's own box, it could be firewall'd to controll IP access to it.

B) Memory - overtime the app server's memory footprint grows, at lot of this can be contributed to backend usage. Considering the backend has a LOT more controllers and views than the frontend, removing it could provide more "room" for the frontend.

Finally, we definitely do need to revisit the Spree::Responder functionality, it's is used in a couple of extension / stores for sure.

@johanb

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb Nov 27, 2012

Contributor

👍 I agree also with @BDQ that splitting up admin could be beneficial. I personally have no need for it though.

Contributor

johanb commented Nov 27, 2012

👍 I agree also with @BDQ that splitting up admin could be beneficial. I personally have no need for it though.

radar added a commit that referenced this pull request Dec 14, 2012

[promo] Fix product rule
There's a couple of things to note in this commit.

The first is that I've removed tokeninput.js. We *were* using it for autocomplete, but we're now using select2 for that.

There's now a product searching 'API endpoint' in Core. This is a 'necessary evil' for the product searching to work, but will be removed when the changes from #2225 are merged.
The select2 helpers within capybara_ext have been massaged a bit to correctly scope calls to their relevant elements, using Capybara's API more rather than depending on ugly JavaScript and sleep statements, and gotten closer to what I'd call sane code.

In promo, the user picker code has been moved out into a new file at user_picker.js, and the product picker lives at product_picker.js. This is because dumping all your code in one file is what we did back in PHP-land, *not* what we're going to do in Spree.

The user picker JS code is now also run when we create a new promotion rule for a user. This means that you won't need to refresh the page to see the field any more. Consider it a freebie.

Finally, we're no longer testing the promotion rule for products using just model calls. We're actually stepping through the flow, as a user would, which is much better.

Conflicts:

	core/lib/spree/core/testing_support/capybara_ext.rb
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 5, 2013

Member

Alright, I think this is just about ready now.

There's a couple of failing tests still to go within the backend component:

rspec ./spec/controllers/spree/admin/products_controller_spec.rb:10 # Spree::Admin::ProductsController#index can find a product by SKU
rspec ./spec/requests/admin/products/option_types_spec.rb:58 # Option Types can remove an option value from an option type

Which I think are related to @BDQ's changes around the use of accessible_by within Admin::ResourceController. I've emailed him and asked him if he could take a look at what's causing these to break. If he doesn't have time, no sweat. I'll look at them tomorrow.

There's nothing else to do that besides to get @schof approval to merge into master.

🎉

Member

radar commented Feb 5, 2013

Alright, I think this is just about ready now.

There's a couple of failing tests still to go within the backend component:

rspec ./spec/controllers/spree/admin/products_controller_spec.rb:10 # Spree::Admin::ProductsController#index can find a product by SKU
rspec ./spec/requests/admin/products/option_types_spec.rb:58 # Option Types can remove an option value from an option type

Which I think are related to @BDQ's changes around the use of accessible_by within Admin::ResourceController. I've emailed him and asked him if he could take a look at what's causing these to break. If he doesn't have time, no sweat. I'll look at them tomorrow.

There's nothing else to do that besides to get @schof approval to merge into master.

🎉

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Feb 5, 2013

Member

@radar - I can't reproduce those fails using either 1.9.3 or 1.8.7:

https://gist.github.com/BDQ/4713670

Member

BDQ commented Feb 5, 2013

@radar - I can't reproduce those fails using either 1.9.3 or 1.8.7:

https://gist.github.com/BDQ/4713670

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Feb 5, 2013

Member

Looking forward to see this merge !

Member

huoxito commented Feb 5, 2013

Looking forward to see this merge !

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 5, 2013

Member

@BDQ: Which branch are you running that in?

EDIT: Oops. build.sh wasn't set up to run tests for backend+frontend. Let me fix that.

Member

radar commented Feb 5, 2013

@BDQ: Which branch are you running that in?

EDIT: Oops. build.sh wasn't set up to run tests for backend+frontend. Let me fix that.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 6, 2013

Member

@BDQ: This was the solution to the problem: 4280239.

Backend tests now passing on my local machine. Waiting for my CI server and Travis to say they're green too.

Member

radar commented Feb 6, 2013

@BDQ: This was the solution to the problem: 4280239.

Backend tests now passing on my local machine. Waiting for my CI server and Travis to say they're green too.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Feb 6, 2013

Contributor

You shouldn't really intern user input to a symbol....

Contributor

parndt commented on 4280239 Feb 6, 2013

You shouldn't really intern user input to a symbol....

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 6, 2013

Member

params[:action] is not user-inputtable. Ever.

Member

radar replied Feb 6, 2013

params[:action] is not user-inputtable. Ever.

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 6, 2013

Member
Started GET "/?action=foo&foo=bar" for 127.0.0.1 at 2013-02-06 12:02:36 +1100
Processing by Spree::HomeController#index as */*
  Parameters: {"foo"=>"bar"}
Member

radar replied Feb 6, 2013

Started GET "/?action=foo&foo=bar" for 127.0.0.1 at 2013-02-06 12:02:36 +1100
Processing by Spree::HomeController#index as */*
  Parameters: {"foo"=>"bar"}
@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Feb 7, 2013

Contributor

+1 on removing Ruby 1.8 support.. it's about time.. especially for commerce 1.9.3+ seems pretty important as 1.8 is old and can be buggy. Time to move on!

Contributor

parndt commented Feb 7, 2013

+1 on removing Ruby 1.8 support.. it's about time.. especially for commerce 1.9.3+ seems pretty important as 1.8 is old and can be buggy. Time to move on!

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 7, 2013

Member

And that's a wrap!

Member

radar commented Feb 7, 2013

And that's a wrap!

@radar radar closed this Feb 7, 2013

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Feb 7, 2013

Member

Well done @radar, great to see this finally merged!

Member

BDQ commented Feb 7, 2013

Well done @radar, great to see this finally merged!

@sbounmy

This comment has been minimized.

Show comment
Hide comment
@sbounmy

sbounmy Feb 7, 2013

Contributor

@radar nice work ! Any specific guidelines for extensions to support split_core ? I am thinking about spree_store_credits, spree_affiliate, spree_minicart,...

Contributor

sbounmy commented Feb 7, 2013

@radar nice work ! Any specific guidelines for extensions to support split_core ? I am thinking about spree_store_credits, spree_affiliate, spree_minicart,...

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Feb 13, 2013

Member

No specific guidelines right now. We're currently working on some new
documentation and I'm hoping I can slot this in there somewhere :)

On Thu, Feb 7, 2013 at 9:19 PM, Bounmy Stéphane notifications@github.comwrote:

@radar https://github.com/radar nice work ! Any specific guidelines for
extensions to support split_core ? I am thinking about spree_store_credits,
spree_affiliate, spree_minicart,...


Reply to this email directly or view it on GitHubhttps://github.com//pull/2225#issuecomment-13229580.

Member

radar commented Feb 13, 2013

No specific guidelines right now. We're currently working on some new
documentation and I'm hoping I can slot this in there somewhere :)

On Thu, Feb 7, 2013 at 9:19 PM, Bounmy Stéphane notifications@github.comwrote:

@radar https://github.com/radar nice work ! Any specific guidelines for
extensions to support split_core ? I am thinking about spree_store_credits,
spree_affiliate, spree_minicart,...


Reply to this email directly or view it on GitHubhttps://github.com//pull/2225#issuecomment-13229580.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment