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

Urls redux #5978

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

Urls redux #5978

wants to merge 3 commits into from

Conversation

smithellis
Copy link
Contributor

Eliminate URL warning/error spam at Django startup.

* Fix URLs
* Fix where URLs are imported
* Fix tests everywhere
@smithellis smithellis marked this pull request as ready for review April 26, 2024 14:51
@smithellis
Copy link
Contributor Author

This involved a lot of test changes, so warrants pretty close inspection.

Copy link
Collaborator

@akatsoulas akatsoulas left a comment

Choose a reason for hiding this comment

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

This looks good! Let's though add an issue about it and add it in the current sprint so it can be thoroughly tested

@akatsoulas
Copy link
Collaborator

Please do not merge - I am having another pass

@@ -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

@@ -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.

@@ -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

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>

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