Skip to content

Commit

Permalink
[FIX] website: redirect to case insensitive URL if not exact match
Browse files Browse the repository at this point in the history
Before this commit, if a link to a page was not correct because of a
case mismatch, it would simply land on a 404 page.
While it's correct, as URL are case sensitive, it leads to a few bad UX
flow at the admin/editor level:
- Create a link in your page (on a text or a button eg), type an URL
  which does not exists (to create it after) like /Page
- Click on the link/button you just made, you are redirected to /Page
  which display a 404 with the "Create page" option (correct)
- When you click on that button, it will actually create a page with
  /page URL, leading to a mismatch between the URL you created and the
  page URL.
  Your link/button will still lead to a 404 URL as it points to /Page.

Since it's just a fallback when an exact URL match is not found, it
should not break anything and should not have bad impact at any level
(seo/speed etc).
Indeed:
- It's done through a 302 redirect
- `_serve_page()` is already a fallback case, so it will only make
  the `website.redirect` and 404 cases a bit slower due to the extra
  search query.

The only possible scenario seems to be if the user (mind the uppercase):
- Created a /Page page
- Created a redirect from /page to /another-page

In this case, /page won't land on /another-page but on /Page.
This flow seems unlikely and is not actually wrong either way.
At least, it certainly is less important than ensuring a case
insensitive fallback.

Finally, note that another solution would have been to either:
- Force page URL to lower case.
  -> This is not stable friendly, people might be relying on this to
     create pages with different casing:
     `/Batman-VII-The-Dark-Knight-Whatevers`, while not recommended,
     doesn't sounds idiot.
     On top of not being stable friendly, we probably want to keep
     offering this possibility
- Redirect all URLs to lowercase endpoints.
  -> This is obviously not stable and not Odoo's jobs. It should be
     something decided by the sysadmin and done at nginx (etc) level.

task-3110294
opw-3104030

closes odoo#109812

Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
  • Loading branch information
rdeodoo committed Jan 30, 2023
1 parent b933fcf commit 639cfc7
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 4 deletions.
17 changes: 13 additions & 4 deletions addons/website/models/ir_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,22 @@ def _get_translation_frontend_modules_name(cls):
@classmethod
def _serve_page(cls):
req_page = request.httprequest.path
page_domain = [('url', '=', req_page)] + request.website.website_domain()

published_domain = page_domain
def _search_page(comparator='='):
page_domain = [('url', comparator, req_page)] + request.website.website_domain()
return request.env['website.page'].sudo().search(page_domain, order='website_id asc', limit=1)

# specific page first
page = request.env['website.page'].sudo().search(published_domain, order='website_id asc', limit=1)
page = _search_page()

# case insensitive search
if not page:
page = _search_page('=ilike')
if page:
logger.info("Page %r not found, redirecting to existing page %r", req_page, page.url)
return request.redirect(page.url)

# redirect withtout trailing /
# redirect without trailing /
if not page and req_page != "/" and req_page.endswith("/"):
return request.redirect(req_page[:-1])

Expand Down
7 changes: 7 additions & 0 deletions addons/website/tests/test_page.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,3 +271,10 @@ def test_homepage_not_slash_url(self):
root_html = html.fromstring(r.content)
canonical_url = root_html.xpath('//link[@rel="canonical"]')[0].attrib['href']
self.assertEqual(canonical_url, website.domain + "/")

def test_page_url_case_insensitive_match(self):
r = self.url_open('/page_1')
self.assertEqual(r.status_code, 200, "Reaching page URL, common case")
r2 = self.url_open('/Page_1', allow_redirects=False)
self.assertEqual(r2.status_code, 302, "URL exists only in different casing, should redirect to it")
self.assertTrue(r2.headers.get('Location').endswith('/page_1'), "Should redirect /Page_1 to /page_1")

0 comments on commit 639cfc7

Please sign in to comment.