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

Add missing key-terms slug to DefinedTerm URL tag #6000

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

Scotchester
Copy link
Contributor

@Scotchester Scotchester commented Sep 14, 2020

Our contacts at Google alerted us to an oversight of mine when I implemented our DefinedTerm markup: the URL for each term that goes in the <link itemprop="url" href="{URL}"> tag was missing the key-terms/ part of the URL. Turns out, our use of RoutablePageMixin to generate the Key Terms pages meant that the page.full_url tag was not including that key-terms/ part of the URL.

I tried using the routablepageurl template tag, but that only returns a root-relative URL, whereas we need an absolute URL, and it required hardcoding a category='key-terms' kwarg, anyway, so we might as well just hardcode it this way.

/cc @kelleyholden

How to test this PR

  1. Visit http://localhost:8000/consumer-tools/auto-loans/answers/key-terms/, inspect the markup for a term, and see that the <link itemprop="url" …> tag's href attribute does not include the key-terms/ path segment
  2. Pull branch
  3. Reload the page and see that the key-terms/ is now included

Checklist

  • PR has an informative and human-readable title
    • PR titles are used to generate the change log in releases; good ones make that easier to scan.
    • Consider prefixing, e.g., "Mega Menu: fix layout bug", or "Docs: Update Docker installation instructions".
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines

Our use of RoutablePageMixin to generate the Key Terms pages meant that 
the `page.full_url` tag was not including the `key-terms/` part of the 
URL. I tried using the `routablepageurl` template tag, but that only 
returns a root-relative URL, whereas we need an absolute URL, and it 
required hardcoding a `category='key-terms'` kwarg, anyway, so we might 
as well just hardcode it this way.
@Scotchester Scotchester requested review from a team and higs4281 September 14, 2020 15:11
@Scotchester
Copy link
Contributor Author

Thanks for the assist, @higs4281! Updated it to insert the slug in the term URL.

@Scotchester Scotchester merged commit d797a19 into main Sep 14, 2020
@Scotchester Scotchester deleted the fix-key-terms-schema branch September 14, 2020 17:45
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.

2 participants