#8444 Add Canonical Link For Tool Sections

v1.14.0
closed
Guillermo Cruz
None
General
Dave Brondsema
2022-09-23
2022-06-28
Guillermo Cruz
No

Tools like Tickets, Wikis, Discussions, etc. should include a canonical link. Also on sections with pagination include page=x in the root url and rel="next" rel="next" tags

Discussion

  • Dave Brondsema

    Dave Brondsema - 2022-07-11
    • minor: ForgeTracker/forgetracker/templates/tracker/milestone.html has import 'allura:templates/jinja_master/lib.html twice
    • when on a page=0 url, then I don't see any rel="next" tag. Remember page=0 means first page in allura! 😁
    • when on a page=1 url then the <link rel="prev" href="/p/test/tickets/milestone/1.0/"/> doesn't have the fully-qualified URL with domain
      • also when on a ticket search then this rel="prev" tag loses the ?q=... part of its URL
    • the def querystring helper is a little complicated, can we have a docstring to explain what it does? Like "make a new URL based on the current request and new params" if that is a correct explanation 😄 Maybe even a unit test too? (you can make a fake request with Request.blank
    • I think there's a mismatch between passing count into lib.pagination_meta_tags( and that macro taking the parameter as last_page. The count of items is not equal to the last page number. Need to divide by the limit size I think to convert count into last_page.
    • probably should include these tags on the main ticket list and the blog main view too.
     

  • Guillermo Cruz - 2022-07-13

    Oh, that is correct there is a miscalculation on the current page and last page. I fixed it.
    - Also added a test for querystring helper and a docstring
    - Fixed full qualified URL for links
    - Added the pagination and canonical url to Blog index,search page and Tickets index page
    - Fixed duplicated import

     

  • Dave Brondsema

    Dave Brondsema - 2022-07-14

    On pages like discussion search, the q parameter is not being included in the canonical tag. I think it should be kept. And on ticket search there can be filter and sort params too.

    Are there any other pages with "important" url parameters that should be kept? I can't think of any others right now.

    Getting some test failures:

    ======================================================================
ERROR: forgewiki.tests.functional.test_root.TestRootController.test_page_label_count
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/src/allura/ForgeWiki/forgewiki/tests/functional/test_root.py", line 485, in test_page_label_count
    assert('page=4' in next['href'])
TypeError: 'NoneType' object is not subscriptable

======================================================================
ERROR: forgewiki.tests.functional.test_root.TestRootController.test_search
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/var/local/env-allura/lib/python3.7/site-packages/mock/mock.py", line 1346, in patched
    return func(*newargs, **newkeywargs)
  File "/src/allura/ForgeWiki/forgewiki/tests/functional/test_root.py", line 95, in test_search
    r = self.app.get('/wiki/search/?q=test')
  File "/src/allura/AlluraTest/alluratest/validation.py", line 341, in get
    resp = super().get(*args, **kw)
  File "/src/allura/AlluraTest/alluratest/validation.py", line 284, in get
    return super().get(*args, **kwargs)
  File "/var/local/env-allura/lib/python3.7/site-packages/webtest/app.py", line 325, in get
    expect_errors=expect_errors)
  File "/var/local/env-allura/lib/python3.7/site-packages/webtest/app.py", line 620, in do_request
    res = req.get_response(app, catch_exc_info=True)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1323, in send
    application, catch_exc_info=True)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1291, in call_application
    app_iter = application(self.environ, start_response)
  File "/var/local/env-allura/lib/python3.7/site-packages/webtest/lint.py", line 196, in lint_app
    iterator = application(environ, start_response_wrapper)
  File "/src/forge-classic/ForgeSF/forgesf/middleware.py", line 57, in __call__
    return self.app(environ, start_response)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/support/registry.py", line 253, in __call__
    app_iter = self.application(environ, start_response)
  File "/var/local/env-allura/lib/python3.7/site-packages/ming/odm/middleware.py", line 29, in __call__
    result = self.app(environ, start_response)
  File "/src/allura/Allura/allura/lib/custom_middleware.py", line 457, in __call__
    return self.app(environ, start_response)
  File "/src/allura/Allura/allura/lib/custom_middleware.py", line 64, in __call__
    return self.app(environ, start_response)
  File "/var/local/env-allura/lib/python3.7/site-packages/ew/middleware.py", line 69, in __call__
    result = self.app(environ, start_response)
  File "/src/allura/Allura/allura/lib/custom_middleware.py", line 266, in __call__
    return resp(environ, start_response)
  File "/src/allura/Allura/allura/config/middleware.py", line 231, in AlluraGlobalsMiddleware
    return app(environ, start_response)
  File "/src/allura/Allura/allura/lib/custom_middleware.py", line 223, in __call__
    return self._app(environ, session_start_response)
  File "/src/timermiddleware/timermiddleware/__init__.py", line 256, in __call__
    resp = req.get_response(self.app)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1327, in send
    application, catch_exc_info=False)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1291, in call_application
    app_iter = application(self.environ, start_response)
  File "/src/allura/Allura/allura/lib/custom_middleware.py", line 156, in __call__
    status, headers, app_iter, exc_info = call_wsgi_application(self.app, environ)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/support/middlewares.py", line 20, in _call_wsgi_application
    app_iter = application(environ, _start_response)
  File "/src/allura/Allura/allura/lib/custom_middleware.py", line 432, in __call__
    return self.app(environ, remember_login_start_response)
  File "/var/local/env-allura/lib/python3.7/site-packages/beaker/middleware.py", line 156, in __call__
    return self.wrap_app(environ, session_start_response)
  File "/var/local/env-allura/lib/python3.7/site-packages/switchboard/middleware.py", line 25, in __call__
    resp = req.get_response(self.app)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1327, in send
    application, catch_exc_info=False)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1291, in call_application
    app_iter = application(self.environ, start_response)
  File "/src/forge-classic/ForgeSF/forgesf/middleware.py", line 88, in __call__
    return self.app(env, start_response)
  File "/src/sftheme/allura/sftheme/deleted_project_redirects.py", line 71, in __call__
    resp = req.get_response(self.app)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1327, in send
    application, catch_exc_info=False)
  File "/var/local/env-allura/lib/python3.7/site-packages/webob/request.py", line 1291, in call_application
    app_iter = application(self.environ, start_response)
  File "/src/forge-classic/ForgeSF/forgesf/middleware.py", line 202, in __call__
    return self.app(environ, start_response)
  File "/src/sftheme/allura/sftheme/sftheme_main.py", line 457, in __call__
    return self.app(env, start_response)
  File "/var/local/env-allura/lib/python3.7/site-packages/newrelic/api/error_trace.py", line 73, in wrapper
    return wrapped(*args, **kwargs)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/wsgiapp.py", line 120, in __call__
    response = self.wrapped_dispatch(controller, environ, context)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/wsgiapp.py", line 285, in _dispatch
    return controller(environ, context)
  File "/var/local/env-allura/lib/python3.7/site-packages/newrelic/api/function_trace.py", line 154, in literal_wrapper
    return wrapped(*args, **kwargs)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/controllers/dispatcher.py", line 119, in __call__
    response = self._perform_call(context)
  File "/src/allura/Allura/allura/controllers/basetest_project_root.py", line 130, in _perform_call
    return super()._perform_call(context)
  File "/src/allura/Allura/allura/lib/base.py", line 40, in _perform_call
    return super()._perform_call(context)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/controllers/dispatcher.py", line 108, in _perform_call
    r = self._call(action, params, remainder=remainder, context=context)
  File "/src/allura/Allura/allura/lib/patches.py", line 123, in _call
    return old_controller_call(self, controller, *args, **kwargs)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/controllers/decoratedcontroller.py", line 137, in _call
    response = self._render_response(context, action, output)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/controllers/decoratedcontroller.py", line 247, in _render_response
    template_name=template_name, **render_params)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/render.py", line 208, in render
    kwargs['result'] = render_function(template_name, tg_vars, **kwargs)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/renderers/jinja.py", line 115, in __call__
    cache_expire=cache_expire)
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/render.py", line 274, in cached_template
    return render_func()
  File "/var/local/env-allura/lib/python3.7/site-packages/tg/renderers/jinja.py", line 110, in render_template
    return Markup(template.render(**template_vars))
  File "/var/local/env-allura/lib/python3.7/site-packages/jinja2/environment.py", line 1301, in render
    self.environment.handle_exception()
  File "/var/local/env-allura/lib/python3.7/site-packages/jinja2/environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/src/allura/ForgeWiki/forgewiki/templates/wiki/search.html", line 19, in top-level template code
    {% extends 'forgewiki:templates/wiki/master.html' %}
  File "/src/allura/ForgeWiki/forgewiki/templates/wiki/master.html", line 22, in top-level template code
    {% import 'allura:templates/jinja_master/lib.html' as lib with context %}
  File "/src/allura/Allura/allura/templates/jinja_master/master.html", line 72, in top-level template code
    {% block head %}
  File "/src/allura/ForgeWiki/forgewiki/templates/wiki/search.html", line 26, in block 'head'
    {{ lib.pagination_meta_tags(request, page, count, limit) }}
  File "/var/local/env-allura/lib/python3.7/site-packages/jinja2/runtime.py", line 777, in _invoke
    rv = self._func(*arguments)
  File "/src/allura/Allura/allura/templates/jinja_master/lib.html", line 898, in template
    {% if results_count and current_page+1 <  h.ceil(results_count/limit) -%}
TypeError: '<' not supported between instances of 'int' and 'MagicMock'

======================================================================
ERROR: forgetracker.tests.functional.test_root.TestFunctionalController.test_search_canonical
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/src/allura/ForgeTracker/forgetracker/tests/functional/test_root.py", line 1409, in test_search_canonical
    assert ('page=4' in next['href'])
TypeError: 'NoneType' object is not subscriptable
     

  • Guillermo Cruz - 2022-07-15
    • Fixed failing tests.
    • For the canonical link I added the helper h.querystring to generate the url and include the url params except limit, could this be the right approach?.
     

  • Dave Brondsema

    Dave Brondsema - 2022-07-15

    Yea I think that is a good approach so we keep any other parameters. Its the safest for now so we don't drop a parameter by mistake. It does mean if there is a junk parameter like ?asf=asf somehow it'll still be in the canonical. If that turns out to be an issue, we can revisit then.

    One last issue I found, if you are on page=1, then the rel=prev tag has page=0 in the URL but it be better to not to say page=0 and just leave that part off. Also the pagination links do the same thing, in page_list.html with the paginator library, is it possible for them to omit when page=0?

     

  • Guillermo Cruz - 2022-07-18

    Removed page value from url string for canonical and also for link rel="prev" meta tag

     

  • Dave Brondsema

    Dave Brondsema - 2022-07-26
    • status: open --> closed
    • Reviewer: Dave Brondsema
     

  • Dave Brondsema

    Dave Brondsema - 2022-09-23
    • Milestone: unreleased --> v1.14.0
     

Log in to post a comment.