Better Routing System #98

Merged
merged 2 commits into from Nov 19, 2013

Conversation

Projects
None yet
5 participants
Member

iloveitaly commented Jan 9, 2013

The current routing system for the static pages seemed very inefficient to me.

I looked at what spree_essentials_cms is doing and integrated its method into spree_static_content.

Code is much simpler and should result in some performance enhancements.

Owner

JDutil commented Jan 9, 2013

@iloveitaly could you add specs for this change? Like a spec to ensure the slugs / is set properly for the different conditions.

Member

iloveitaly commented Jan 9, 2013

Added specs in b875d23. Any other specific cases you want me to handle?

Owner

JDutil commented Jan 9, 2013

I'd like to see a spec to ensure this works properly in the future:
https://github.com/spree/spree_static_content/pull/98/files#L2R31

with and without: not_using_foreign_link?

Owner

JDutil commented Jan 15, 2013

Should probably also add a migration to update existing records to have a leading / otherwise this won't be backwards compatible for old apps upgrading.

Member

iloveitaly commented Jan 15, 2013

Good point – it needs a migration. Should have time in the next two weeks to the specs and migration together.

laurens commented Jan 17, 2013

What about the remove_spree_mount_point code?

Before, when Spree is mounted say under /spree, the static_content gem would take care that pages do not need to include that mount point in their slug.

Owner

JDutil commented Jan 17, 2013

@laurens good point on the mount point of spree. Would like to see specs for that as well to ensure it works properly for root and other mount points.

laurens commented Jan 17, 2013

@JDutil Curious, how to test a different mount point. Would this require a second dummy app?

@laurens laurens commented on the diff Jan 17, 2013

config/routes.rb
class Spree::StaticPage
def self.matches?(request)
- slug = StaticPage::remove_spree_mount_point(request.fullpath)
- pages = Spree::Page.arel_table
- Spree::Page.visible.by_slug(slug).exists?
- end
-end
-
-class Spree::StaticRoot
- def self.matches?(request)
- path = StaticPage::remove_spree_mount_point(request.fullpath)
- path.nil? && Spree::Page.visible.by_slug(path.to_s).first
+ return false if request.path =~ /(^\/+(admin|account|cart|checkout|content|login|pg\/|orders|products|s\/|session|signup|shipments|states|t\/|tax_categories|user)+)/
+ !Spree::Page.visible.find_by_slug(request.path).nil?
@laurens

laurens Jan 17, 2013

We should use the exists? finder here like exists?( :slug => request.path ) for performance reasons, I think. Otherwise the whole record is fetched upon every request.

@JDutil

JDutil Feb 26, 2013

Owner

Good call @iloveitaly could you update this as well so that it is more efficient.

laurens commented Jan 18, 2013

@iloveitaly Based on your work, I have addressed the mount point issue in 4c92acf.
I do not know how to write a test for that though, since it would require changes in the test_app.

@laurens laurens added a commit to laurens/spree_static_content that referenced this pull request Jan 18, 2013

@laurens laurens Added tests for dealing with slugs without prefix. 38be433

@laurens laurens added a commit to laurens/spree_static_content that referenced this pull request Jan 18, 2013

@laurens laurens Added tests for dealing with slugs without prefix. 7ebafc9
Owner

JDutil commented Jan 18, 2013

@laurens I'd need to look into it some. I thought I had seen a test for similar situation in either spree itself or one of the extensions I typically dig into. However if it does actually require modifying the dummy app for a single spec I may be in a similar situation as I am with the SSL pull request I have open on Spree where I'm still trying to determine a best way to handle such scenarios.

I'm just hesitant to make such a large refactoring of the routing for this extension without tests to ensure it continues working as expected now and in the future.

Member

iloveitaly commented Feb 19, 2013

@JDutil Is this iloveitaly/spree_static_content@3404b06 sufficient for testing not_using_foreign_link? + slug prepending? Is this enough for local link slug testing iloveitaly/spree_static_content@67cd0f4?

Do you have any info on testing multiple mount points? Aside from testing multiple mount points, is there anything else that needs to be done to get this merged?

Owner

JDutil commented Feb 26, 2013

@iloveitaly I'm not familiar with how to test the mount points actually, and think it may be overly complicated. I think you would have to dynamically modify the dummy apps route file, which could be messy. Have you tested things manually at different mount points? May just verify it works manually to confirm if it's too much of a pain to do a spec.

As for testing the not_using_foreign_link piece it would certainly be nice to add that spec as well. What I meant though was that I'd like to see a spec ensuring that the / is added to the slug when update_positions_and_slug is run.

Other than that I think the only other thing we discussed was to add a migration to ensure all existing pages get their slug updated with the leading /

JDutil referenced this pull request Apr 6, 2013

Closed

Bad routes problems #108

Owner

peterberkenbosch commented Nov 18, 2013

Is this still relevant? If so please reopen.

Owner

peterberkenbosch commented Nov 18, 2013

This topic comes up more then once, so reopening this one to work it out further.

@peterberkenbosch peterberkenbosch merged commit 82578e9 into spree-contrib:master Nov 19, 2013

Owner

peterberkenbosch commented Nov 19, 2013

closed in 799e041 thanks!

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