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

Nullify duplicate slugs under the same parent after reodering pages. #2092

Merged
merged 1 commit into from
Dec 21, 2012

Conversation

parndt
Copy link
Member

@parndt parndt commented Dec 20, 2012

Fixes #2070

@parndt
Copy link
Member Author

parndt commented Dec 20, 2012

Updated with drop(1) by the brilliant @gwagener

@parndt
Copy link
Member Author

parndt commented Dec 20, 2012

This could also be done using:

def nullify_duplicate_slugs_under_the_same_parent!
  table = Page.translation_class.arel_table
  Page.includes(:translations).group(:parent_id, table[:slug]).having(table[:slug].count.gt(1)).count(&:slug).each do |(parent_id, slug), count|
    Page.by_slug(slug).where(:parent_id => parent_id).drop(1).each do |page|
      page.slug = nil
      page.save
    end
  end
end

@parndt
Copy link
Member Author

parndt commented Dec 20, 2012

I'm afraid that we're also going to have problems with slugs in locales other than the current locale too >_>

@gwagener
Copy link
Contributor

It could be done with an arel table and I assume the performance would be better because it's doing more of the filtering in the database. However, if the performance gain isn't noticeable I think the way it is is easier to read.

With the arel table do you need to pass count into the block?

@parndt
Copy link
Member Author

parndt commented Dec 21, 2012

OK I've dealt with the locale issues and force pushed. Good to go 👍

ugisozols added a commit that referenced this pull request Dec 21, 2012
Nullify duplicate slugs under the same parent after reodering pages.
@ugisozols ugisozols merged commit 5a025ce into master Dec 21, 2012
@ugisozols ugisozols deleted the issue_2070 branch December 21, 2012 09:22
@ugisozols
Copy link
Member

Thanks @gwagener for Array#drop - seems like a really handy method to know about.

@parndt
Copy link
Member Author

parndt commented Dec 21, 2012

I smell a blog post @ugisozols

ugisozols added a commit that referenced this pull request Dec 21, 2012
protected
def nullify_duplicate_slugs_under_the_same_parent!
t_slug = translation_class.arel_table[:slug]
joins(:translations).group(:locale, :parent_id, t_slug).having(t_slug.count.gt(1)).count.

Choose a reason for hiding this comment

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

ERROR: column "refinery_pages.lft" must appear in the GROUP BY clause or be used in an aggregate functio
Correct could be:

def nullify_duplicate_slugs_under_the_same_parent!
    t_slug = translation_class.arel_table[:slug]
    joins(:translations).group(:locale, :parent_id, t_slug, :lft).having(t_slug.count.gt(1)).count.
        each do |(locale, parent_id, slug), count|
      by_slug(slug, :locale => locale).where(:parent_id => parent_id).drop(1).each do |page|
        page.slug = nil # kill the duplicate slug
        page.save # regenerate the slug
      end
    end
  end

Copy link
Member Author

Choose a reason for hiding this comment

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

@viglesias can you open a pull request with a failing test case if you're having issues please?

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.

Possible bug > Pages with same slug allowed to be put under the same hierarchy level
4 participants