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

Fix #3218 regression on sitemap: don't duplicate locale in url #3271

Merged
merged 1 commit into from
Dec 11, 2016

Conversation

bricesanchez
Copy link
Member

Fix #3218 regression: don't duplicate locale in url and refactor the builder to be more readable

Since this PR, i have problems on sitemap:

[...]
<url>
<loc>
http://localhost:3000/en/en/rate_requests/new
</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en/forms</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en/about-us</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en/en/jobs</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en/en</loc>
<lastmod>2016-07-20</lastmod>
</url>
[...]

After the fix:

[...]
<url>
<loc>http://localhost:3000/en/rate_requests/new</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en/forms</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en/about-us</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en/jobs</loc>
<lastmod>2016-09-01</lastmod>
</url>
<url>
<loc>http://localhost:3000/en</loc>
<lastmod>2016-07-20</lastmod>
</url>
[...]

and refactor the builder to be more readable
@bricesanchez
Copy link
Member Author

@sintro Could you test this PR ?

@sintro
Copy link

sintro commented Dec 1, 2016

Just checked, your patch works fine for my cases, so it breaks nothing. But I also did not have such problems, probably, you get it when the path of page has manually set /language/ part? I think now with this PR everything is fine!

One thing about this sitemap generation: what about filtering out the pages with that locales, which don't have translations for that language (for example, empty body or title or both)?

@bricesanchez
Copy link
Member Author

Just checked, your patch works fine for my cases, so it breaks nothing. But I also did not have such problems, probably, you get it when the path of page has manually set /language/ part? I think now with this PR everything is fine!

The sitemap problem was on extensions urls, no pages url.

One thing about this sitemap generation: what about filtering out the pages with that locales, which don't have translations for that language (for example, empty body or title or both)?

It's already done by the in_menu scope:

@sintro
Copy link

sintro commented Dec 1, 2016

It's already done by the in_menu scope

Strangely, but I always get all of the pages with all of frontend_locales... Even for pages, which even does not have that locale symbol in Pages section in admin panel (so, there are no title and no body).

@mhtremblay
Copy link

Is there something missing to merge this PR?

@sintro
Copy link

sintro commented Dec 5, 2016

@mhtremblay everything is fine. At least, new sitemap.xml.erb is better than older.

@mhtremblay
Copy link

Can someone merge this PR plz?

# exclude sites that are external to our own domain.
page_url = if page.url.is_a?(Hash)
# This is how most pages work without being overriden by link_url
page.url.merge({:only_path => false, locale: locale})

Choose a reason for hiding this comment

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

page.url.merge({only_path: false, locale: locale})

@parndt parndt merged commit ebc3f91 into master Dec 11, 2016
@parndt parndt deleted the bugfix/regression-3218-sitemap branch December 11, 2016 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants