Skip to content
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

Implement Page.rebuild! so that invalid url cache is cleared on re-sort #1702

Closed
wants to merge 1 commit into from

Conversation

joemsak
Copy link
Contributor

@joemsak joemsak commented May 25, 2012

All tests passing on my end, and confirmed this fixed the problem I was having

When re-sorting pages, the URL in the front-end was not updating to the correct path

Now it does with this .rebuild! class method

@joemsak
Copy link
Contributor Author

joemsak commented May 25, 2012

This possibly fixes the other issue properly without having to use page id in the admin edit URLs HOWEVER:

Since the page tree is not returned / refreshed, that would still have to be addressed to put friendly URLs back into the admin

@travisbot
Copy link

This pull request passes (merged 174545f into e32ba57).

@ugisozols
Copy link
Member

I must be missing something but how would this plain implementation of rebuild! fix cached page urls? I mean where is it called when you reorder pages?

Although it would be really cool to have some specs for this.

@joemsak
Copy link
Contributor Author

joemsak commented May 25, 2012

Called at the end of update_positions

https://github.com/resolve/refinerycms/blob/master/core/lib/refinery/crud.rb#L291

The problem being addressed was that after re-sorting, pages retained their old URLs in the front end (though they appeared in the right place in the menu)

Calling this method made that issue go away for us

all.each do |page|
invalidate_child_cached_urls(page)
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic seems a bit funny.. you're calling all and then looping through each page and its children when they're already contained inside the all result set.

@parndt
Copy link
Member

parndt commented May 26, 2012

@joemsak @ugisozols @robyurkowski I've opened #1706 to replace this pull request because I wanted to suggest an alternative which would make more sense to me..

@parndt parndt closed this May 26, 2012
@joemsak
Copy link
Contributor Author

joemsak commented May 26, 2012

oh yea duh :P

On Friday, May 25, 2012 at 10:23 PM, Philip Arndt wrote:

@joemsak @ugisozols @robyurkowski I've opened #1706 to replace this pull request because I wanted to suggest an alternative which would make more sense to me..


Reply to this email directly or view it on GitHub:
https://github.com/resolve/refinerycms/pull/1702#issuecomment-5944056

ugisozols added a commit that referenced this pull request Jun 2, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants