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.

Sign up for GitHub

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

Jump to bottom

Urls redux #5978

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Urls redux #5978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d926916
Stop warnings about wiki urls having slashes
smithellis Mar 8, 2024
b717167
inproduct pieces
smithellis Mar 11, 2024
9effe39
pass tests
smithellis Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions kitsune/dashboards/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ def test_visit_count_from_analytics(self, _build_request, close_old_connections)
PAGEVIEWS_BY_DOCUMENT_RESPONSE = {
"kind": "analytics#gaData",
"rows": [
["/en-US/kb/hellỗ", "27"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's update the urls per app. One PR for one app and avoid adding multiple commits per app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd ask you to reconsider - this is a test in dashboards that fails because of a setting change in the wiki urls .

This PR is for wiki url changes, and fixing what those changes break, which is tests in a four other apps. You need the wiki/urls.py changes in place so your changed tests will work.

["/en-US/kb/hellỗ/edit", "2"],
["/en-US/kb/hellỗ/history", "1"],
["/en-US/kb/there", "23"],
["/en-US/kb/doc-3", "10"],
["/en-US/kb/doc-4", "39"],
["/en-US/kb/doc-5", "40"],
["/en-US/kb/doc-5/discuss", "1"],
["/en-US/kb/hellỗ/", "27"],
["/en-US/kb/hellỗ/edit/", "2"],
["/en-US/kb/hellỗ/history/", "1"],
["/en-US/kb/there/", "23"],
["/en-US/kb/doc-3/", "10"],
["/en-US/kb/doc-4/", "39"],
["/en-US/kb/doc-5/", "40"],
["/en-US/kb/doc-5/discuss/", "1"],
["/en-US/kb/doc-5?param=ab", "2"],
["/en-US/kb/doc-5?param=cd", "4"],
],
Expand Down
57 changes: 29 additions & 28 deletions kitsune/inproduct/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@
class RedirectTestCase(TestCase):
test_urls = (
("firefox/3.6.12/WINNT/en-US/", "/en-US/"),
("mobile/4.0/Android/en-US/", "/en-US/products/mobile"),
("mobile/4.0/Android/en-US/", "/en-US/products/mobile/"),
("firefox/3.6.12/MACOSX/en-US", "/en-US/"),
("firefox/3.6.12/WINNT/fr/", "/fr/"),
("firefox/3.6.12/WINNT/fr-FR/", "/fr/"),
("firefox-home/1.1/iPhone/en-US/", "/en-US/"),
("firefox/4.0/Linux/en-US/prefs-applications", "/en-US/kb/Applications"),
("firefox/4.0/Linux/en-US/prefs-applications/", "/en-US/kb/Applications"),
("firefox/5.0/NONE/en-US/", "/en-US/does-not-exist"),
("firefox/4.0/Linux/en-US/prefs-applications", "/en-US/kb/Applications/"),
("firefox/4.0/Linux/en-US/prefs-applications/", "/en-US/kb/Applications/"),
("firefox/5.0/NONE/en-US/", "/en-US/does-not-exist/"),
("firefox/4.0/Android/en-US/foo", 404),
# Make sure Basque doesn't trigger the EU ballot logic.
("firefox/29.0/Darwin/eu/", "/eu/"),
Expand All @@ -33,36 +33,36 @@ class RedirectTestCase(TestCase):

test_eu_urls = (
("firefox/3.6.12/WINNT/en-US/eu/", "/en-US/"),
("mobile/4.0/Android/en-US/eu/", "/en-US/products/mobile"),
("mobile/4.0/Android/en-US/eu/", "/en-US/products/mobile/"),
("firefox/3.6.12/MACOSX/en-US/eu", "/en-US/"),
("firefox/3.6.12/WINNT/fr/eu/", "/fr/"),
("firefox/3.6.12/WINNT/fr-FR/eu/", "/fr/"),
("firefox-home/1.1/iPhone/en-US/eu/", "/en-US/"),
("firefox/4.0/Linux/en-US/eu/prefs-applications", "/en-US/kb/Applications"),
("firefox/4.0/Linux/en-US/eu/prefs-applications/", "/en-US/kb/Applications"),
("firefox/5.0/NONE/en-US/eu/", "/en-US/does-not-exist"),
("firefox/4.0/Linux/en-US/eu/prefs-applications", "/en-US/kb/Applications/"),
("firefox/4.0/Linux/en-US/eu/prefs-applications/", "/en-US/kb/Applications/"),
("firefox/5.0/NONE/en-US/eu/", "/en-US/does-not-exist/"),
("firefox/4.0/Android/en-US/eu/foo", 404),
# Basque is awesome.
("firefox/30.0/WINNT/eu/eu/", "/eu/"),
("firefox/4.0/Linux/eu/eu/prefs-applications", "/eu/kb/Applications"),
("firefox/4.0/Linux/eu/eu/prefs-applications", "/eu/kb/Applications/"),
)

def setUp(self):
super(RedirectTestCase, self).setUp()

# Create redirects to test with.
RedirectFactory(target="kb/Applications", topic="prefs-applications")
RedirectFactory(target="")
RedirectFactory(product="mobile", target="products/mobile")
RedirectFactory(platform="iPhone", target="")
RedirectFactory(product="mobile", platform="Android", topic="foo", target="")
RedirectFactory(version="5.0", target="does-not-exist")
RedirectFactory(platform="martian", target="https://martian.com")
RedirectFactory(product="firefox", topic="learn", target="kb/learn?utm_source=yada")
RedirectFactory(target="kb/Applications/", topic="prefs-applications")
RedirectFactory(target="/")
RedirectFactory(product="mobile", target="products/mobile/")
RedirectFactory(platform="iPhone", target="/")
RedirectFactory(product="mobile", platform="Android", topic="foo", target="/")
RedirectFactory(version="5.0", target="does-not-exist/")
RedirectFactory(platform="martian", target="https://martian.com/")
RedirectFactory(product="firefox", topic="learn", target="kb/learn/?utm_source=yada")
RedirectFactory(
product="monitor",
topic="share",
target="kb/share?utm_content=firefox-share&utm_source=blue",
target="kb/share/?utm_content=firefox-share&utm_source=blue",
)

def test_target(self):
Expand All @@ -75,44 +75,44 @@ def test_eu_target(self):

def test_external_target(self):
tests = (
("mobile/4.0/MARTIAN/en-US/", "https://martian.com"),
("mobile/4.0/MARTIAN/en-US/eu/", "https://martian.com"),
("mobile/4.0/MARTIAN/en-US/", "https://martian.com/"),
("mobile/4.0/MARTIAN/en-US/eu/", "https://martian.com/"),
)
self._targets(tests, "")

def test_with_incoming_params(self):
"""Test that incoming query parameters are preserved."""

tests = (
("firefox/112/Linux/en-US/learn", "/en-US/kb/learn", "utm_source=yada&as=u"),
("firefox/112/Linux/en-US/learn", "/en-US/kb/learn/", "utm_source=yada&as=u"),
(
"firefox/112/Linux/en-US/learn?utm_source=firefox",
"/en-US/kb/learn",
"/en-US/kb/learn/",
"utm_source=firefox&as=u",
),
(
"firefox/112/Linux/en-US/learn?utm_content=learn&utm_source=firefox",
"/en-US/kb/learn",
"/en-US/kb/learn/",
"utm_source=firefox&utm_content=learn&as=u",
),
(
"firefox/112/Linux/en-US/learn?utm_content=learn",
"/en-US/kb/learn",
"/en-US/kb/learn/",
"utm_source=yada&utm_content=learn&as=u",
),
(
"monitor/112/Linux/en-US/share",
"/en-US/kb/share",
"/en-US/kb/share/",
"utm_content=firefox-share&utm_source=blue&as=u",
),
(
"monitor/112/Linux/en-US/eu/share",
"/en-US/kb/share",
"/en-US/kb/share/",
"utm_content=firefox-share&utm_source=blue&as=u&eu=1",
),
(
"mobile/4.0/martian/en-US?utm_source=aliens",
"https://martian.com",
"https://martian.com/",
"utm_source=aliens",
),
)
Expand All @@ -136,7 +136,8 @@ def _targets(self, urls, querystring):
self.assertEqual(302, response.redirect_chain[0][1])
# Let's check the final redirect against what we expected.
final = urlparse(response.redirect_chain[-1][0])
self.assertEqual(output, final.path)
fpath = f"{final.path}/" if not final.path.endswith("/") else final.path
self.assertEqual(output, fpath)
self.assertEqual(querystring, final.query)

@mock.patch.object(Site.objects, "get_current")
Expand Down
2 changes: 1 addition & 1 deletion kitsune/postcrash/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
class SignatureTests(TestCase):
def test_get_absolute_url(self):
sig = SignatureFactory(document__slug="foo-bar")
self.assertEqual("/kb/foo-bar", sig.get_absolute_url())
self.assertEqual("/kb/foo-bar/", sig.get_absolute_url())
2 changes: 1 addition & 1 deletion kitsune/postcrash/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,6 @@ def test_known_signature(self):
response = self.client.get(url)
self.assertEqual(200, response.status_code)
self.assertEqual(
"https://example.com/kb/%s" % sig.document.slug, response.content.decode()
"https://example.com/kb/%s/" % sig.document.slug, response.content.decode()
)
self.assertEqual("text/plain", response["content-type"])
1 change: 1 addition & 0 deletions kitsune/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,7 @@ def immutable_file_test(path, url):
},
]

APPEND_SLASH = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not needed


MIDDLEWARE: tuple[str, ...] = (
"kitsune.sumo.middleware.HostnameMiddleware",
Expand Down
20 changes: 10 additions & 10 deletions kitsune/sumo/tests/test_googleanalytics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,16 @@ def test_search_ctr(self, _build_request):
PAGEVIEWS_BY_DOCUMENT_RESPONSE = {
"kind": "analytics#gaData",
"rows": [
["/en-US/kb/doc-1", "1"], # Counts as a pageview.
["/en-US/kb/doc-1/edit", "2"], # Doesn't count as a pageview
["/en-US/kb/doc-1/history", "1"], # Doesn't count as a pageview
["/en-US/kb/doc-2", "2"], # Counts as a pageview.
["/en-US/kb/doc-3", "10"], # Counts as a pageview.
["/en-US/kb/doc-4", "39"], # Counts as a pageview.
["/en-US/kb/doc-5", "40"], # Counts as a pageview.
["/en-US/kb/doc-5/discuss", "1"], # Doesn't count as a pageview
["/en-US/kb/doc-5?param=ab", "2"], # Counts as a pageview.
["/en-US/kb/doc-5?param=cd", "4"],
["/en-US/kb/doc-1/", "1"], # Counts as a pageview.
["/en-US/kb/doc-1/edit/", "2"], # Doesn't count as a pageview
["/en-US/kb/doc-1/history/", "1"], # Doesn't count as a pageview
["/en-US/kb/doc-2/", "2"], # Counts as a pageview.
["/en-US/kb/doc-3/", "10"], # Counts as a pageview.
["/en-US/kb/doc-4/", "39"], # Counts as a pageview.
["/en-US/kb/doc-5/", "40"], # Counts as a pageview.
["/en-US/kb/doc-5/discuss/", "1"], # Doesn't count as a pageview
["/en-US/kb/doc-5/?param=ab", "2"], # Counts as a pageview.
["/en-US/kb/doc-5/?param=cd", "4"],
], # Counts as a pageview.
"containsSampledData": False,
"columnHeaders": [
Expand Down
30 changes: 15 additions & 15 deletions kitsune/sumo/tests/test_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def setUp(self):
def test_image_params_page(self):
"""build_hook_params handles wiki pages."""
_, params = build_hook_params_default("t|page=Installing Firefox")
self.assertEqual("/en-US/kb/installing-firefox", params["link"])
self.assertEqual("/en-US/kb/installing-firefox/", params["link"])
assert params["found"]

def test_image_params_link(self):
Expand All @@ -146,7 +146,7 @@ def test_image_params_page_link(self):
"""_build_image_params - wiki page overrides link."""
text = "t|page=Installing Firefox|link=http://example.com"
_, params = build_hook_params_default(text)
self.assertEqual("/en-US/kb/installing-firefox", params["link"])
self.assertEqual("/en-US/kb/installing-firefox/", params["link"])

def test_image_params_align(self):
"""Align valid options."""
Expand Down Expand Up @@ -202,7 +202,7 @@ def test_get_wiki_link(self):
self.assertEqual(
{
"found": True,
"url": "/en-US/kb/installing-firefox",
"url": "/en-US/kb/installing-firefox/",
"text": "Installing Firefox",
},
_get_wiki_link("Installing Firefox", locale=settings.WIKI_DEFAULT_LANGUAGE),
Expand Down Expand Up @@ -313,27 +313,27 @@ def setUp(self):
def test_simple(self):
"""Simple internal link markup."""
link = pq_link(self.p, "[[Installing Firefox]]")
self.assertEqual("/en-US/kb/installing-firefox", link.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/", link.attr("href"))
self.assertEqual("Installing Firefox", link.text())
assert not link.hasClass("new")

def test_simple_markup(self):
text = "[[Installing Firefox]]"
self.assertEqual(
'<p><a href="/en-US/kb/installing-firefox">' + "Installing Firefox</a></p>",
'<p><a href="/en-US/kb/installing-firefox/">' + "Installing Firefox</a></p>",
self.p.parse(text).replace("\n", ""),
)

def test_link_hash(self):
"""Internal link with hash."""
link = pq_link(self.p, "[[Installing Firefox#section name]]")
self.assertEqual("/en-US/kb/installing-firefox#section_name", link.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/#section_name", link.attr("href"))
self.assertEqual("Installing Firefox", link.text())

def test_link_hash_text(self):
"""Internal link with hash and text."""
link = pq_link(self.p, "[[Installing Firefox#section name|section]]")
self.assertEqual("/en-US/kb/installing-firefox#section_name", link.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/#section_name", link.attr("href"))
self.assertEqual("section", link.text())

def test_hash_only(self):
Expand All @@ -345,12 +345,12 @@ def test_hash_only(self):
def test_link_name(self):
"""Internal link with name."""
link = pq_link(self.p, "[[Installing Firefox|this name]]")
self.assertEqual("/en-US/kb/installing-firefox", link.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/", link.attr("href"))
self.assertEqual("this name", link.text())

def test_link_with_extra_pipe(self):
link = pq_link(self.p, "[[Installing Firefox|with|pipe]]")
self.assertEqual("/en-US/kb/installing-firefox", link.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/", link.attr("href"))
self.assertEqual("with|pipe", link.text())

def test_hash_name(self):
Expand All @@ -363,14 +363,14 @@ def test_hash_name(self):
def test_link_hash_name(self):
"""Internal link with hash and name."""
link = pq_link(self.p, "[[Installing Firefox#section 3|this name]]")
self.assertEqual("/en-US/kb/installing-firefox#section_3", link.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/#section_3", link.attr("href"))
self.assertEqual("this name", link.text())

def test_link_hash_name_markup(self):
"""Internal link with hash and name."""
text = "[[Installing Firefox#section 3|this name]]"
self.assertEqual(
'<p><a href="/en-US/kb/installing-firefox#section_3">this name</a>\n</p>',
'<p><a href="/en-US/kb/installing-firefox/#section_3">this name</a>\n</p>',
self.p.parse(text),
)

Expand All @@ -397,13 +397,13 @@ def test_link_with_localization(self):
# Without an approved revision, link should go to en-US doc.
# The site should stay in fr locale (/<locale>/<en-US slug>).
link = pq(self.p.parse("[[A doc]]", locale="fr"))
self.assertEqual("/fr/kb/a-doc", link.find("a").attr("href"))
self.assertEqual("/fr/kb/a-doc/", link.find("a").attr("href"))
self.assertEqual("A doc", link.find("a").text())

# Approve a revision. Now link should go to fr doc.
ApprovedRevisionFactory(document=fr_d)
link = pq(self.p.parse("[[A doc]]", locale="fr"))
self.assertEqual("/fr/kb/une-doc", link.find("a").attr("href"))
self.assertEqual("/fr/kb/une-doc/", link.find("a").attr("href"))
self.assertEqual("Une doc", link.find("a").text())


Expand Down Expand Up @@ -477,7 +477,7 @@ def test_page_link(self):

self.assertEqual("test.jpg", img.attr("alt"))
self.assertEqual(self.img.file.url, img.attr("src"))
self.assertEqual("/en-US/kb/installing-firefox", img_a.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/", img_a.attr("href"))

def test_page_link_edit(self):
"""Link to a nonexistent wiki page."""
Expand Down Expand Up @@ -610,4 +610,4 @@ def test_frameless_link(self):
img_a = pq_img(self.p, "[[Image:test.jpg|page=Installing Firefox]]", "a")
img = img_a("img")
assert "frameless" in img.attr("class")
self.assertEqual("/en-US/kb/installing-firefox", img_a.attr("href"))
self.assertEqual("/en-US/kb/installing-firefox/", img_a.attr("href"))
2 changes: 1 addition & 1 deletion kitsune/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
admin.autodiscover()

urlpatterns = i18n_patterns(
path("kb", include("kitsune.wiki.urls")),
path("kb/", include("kitsune.wiki.urls")),
path("search/", include("kitsune.search.urls")),
path("forums/", include("kitsune.forums.urls")),
path("questions/", include("kitsune.questions.urls")),
Expand Down
2 changes: 1 addition & 1 deletion kitsune/wiki/tests/test_decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def temp(request):
req.LANGUAGE_CODE = "es"
res = temp(req)
self.assertEqual(302, res.status_code)
self.assertEqual("/en-US/kb/frequently-asked-questions", res["location"])
self.assertEqual("/en-US/kb/frequently-asked-questions/", res["location"])

def test_non_faq_locale_doesnt_redirect(self):
@check_simple_wiki_locale
Expand Down
4 changes: 2 additions & 2 deletions kitsune/wiki/tests/test_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,11 @@ def test_internal_links(self):
# Both internal links should link to the same article
self.assertEqual(
p.parse("[[%s]]" % doc.title),
'<p><a href="/en-US/kb/%s">%s</a>\n</p>' % (doc.slug, doc.title),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does this need changing?

Copy link
Contributor Author

@smithellis smithellis May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Because

'<p><a href="/en-US/kb/real-article/">Real article</a>\n</p>' != '<p><a href="/en-US/kb/real-article">Real article</a>\n</p>'
- <p><a href="/en-US/kb/real-article/">Real article</a>

'<p><a href="/en-US/kb/%s/">%s</a>\n</p>' % (doc.slug, doc.title),
)
self.assertEqual(
p.parse("[[%s]]" % redirect.title),
'<p><a href="/en-US/kb/%s">%s</a>\n</p>' % (doc.slug, doc.title),
'<p><a href="/en-US/kb/%s/">%s</a>\n</p>' % (doc.slug, doc.title),
)


Expand Down
6 changes: 3 additions & 3 deletions kitsune/wiki/tests/test_templates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@
To view the updated document, click the following link, or paste it into \
your browser's location bar:

https://testserver/en-US/kb/%(document_slug)s?utm_campaign=wiki-approved&\
https://testserver/en-US/kb/%(document_slug)s/?utm_campaign=wiki-approved&\
utm_source=notification&utm_medium=email

--
Expand Down Expand Up @@ -217,7 +217,7 @@ def test_document_fallback_with_translation_english_slug(self):
RevisionFactory(document=d2, is_approved=False)
url = reverse("wiki.document", args=[r.document.slug], locale="fr")
response = self.client.get(url, follow=True)
self.assertEqual("/fr/kb/french", response.redirect_chain[0][0])
self.assertEqual("/fr/kb/french/", response.redirect_chain[0][0])
doc = pq(response.content)
self.assertEqual(d2.title, doc("h1.sumo-page-heading").text())
# Fallback message is shown.
Expand Down Expand Up @@ -2519,7 +2519,7 @@ def test_preview_locale(self):
doc = pq(response.content)
link = doc("#doc-content a")
self.assertEqual("Prueba", link.text())
self.assertEqual("/es/kb/prueba", link[0].attrib["href"])
self.assertEqual("/es/kb/prueba/", link[0].attrib["href"])


class HelpfulVoteTests(TestCase):
Expand Down
2 changes: 1 addition & 1 deletion kitsune/wiki/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2378,7 +2378,7 @@ def test_pocket_redirect_to_kb_for_document_that_exists(self):
redirect_chain = response.redirect_chain
self.assertEqual(len(redirect_chain), 2)
self.assertEqual(redirect_chain[0], (f"/en-US/kb/pocket/1111-{doc.slug}", 302))
self.assertEqual(redirect_chain[1], ("/en-US/kb/article-title", 302))
self.assertEqual(redirect_chain[1], ("/en-US/kb/article-title/", 302))

def test_pocket_redirect_when_kb_article_doesnt_exist(self):
"""No match found, we should be sent to /products/pocket"""
Expand Down
Loading