elixirschool / school_house

The new era of Elixir School now powered by @phoenixframework
https://elixirschool.com/
Apache License 2.0
152 stars 48 forks source link

Locale menu generates broken links #85

Closed jaimeiniesta closed 3 years ago

jaimeiniesta commented 3 years ago

When working at validation I was surprised to see that Rocket Validator had found 4,273 web pages in the site so I decided to have a look at the list of web pages found.

Captura de pantalla 2021-07-23 a las 10 35 48

The web page URLS definitely look weird:

Captura de pantalla 2021-07-23 a las 10 36 34

"announcemtat", "announcemvit", "phonoix", "phoplix"... That seems to be caused by the way we're generating the URLs for the pages in other languages, changing "en" by the language code:

When a page can't be found, the site is still responding with 200 OK instead of 404 Not Found as it should, which explains why Rocket Validator picked the web page and still validated the content:

➔ curl -i https://beta.elixirschool.com/blog/elixirconf-announcemskt
HTTP/2 200 
date: Fri, 23 Jul 2021 08:43:18 GMT

So in order to fix this we need to:

1.- Fix locale menu URLs 2.- Return 404 on pages not found

I'll work on that.

jaimeiniesta commented 3 years ago

OK to generate the URLs for all the languages we're doing this:

https://github.com/elixirschool/school_house/blob/master/lib/school_house_web/views/html_helpers.ex#L16-L18

Which basically finds the first occurrence of the current language, and replaces that for the rest of locales.

This works fine when the URL contains the language, that is, for the routes that are within the scope :locale part:

https://github.com/elixirschool/school_house/blob/master/lib/school_house_web/router.ex#L29

But some URLs like the Blog, are outside the locale scope. So changing "en" by "pl" puts us in that broken link situation.

What should we do with that?

I think we should make the locale a first-class citizen (even if there's no translation for all content). This way instead of /blog we'll have /en/blog which can be translated to other locale.

I'd also make the string substitution more specific, instead of "en" => "pl", make it "/en/" => "/pl/".

If we don't want to move all the pages within the locale scope then maybe making that string substitution smarter (as above) will be enough.

jaimeiniesta commented 3 years ago

I have a draft PR where I've fixed the locale menu - it leaves the URL unchanged if there's no locale part in it. Which makes it useless in those URLs, so I think we should hide the locale menu when there's no locale in the URL, or make the locale present in all the URLs.