Skip to content

Fix #3149 docs template link + downloads URL reversing bugs - #3187

Closed
alvynabranches wants to merge 2 commits into
python:mainfrom
alvynabranches:fix/bugs-audit-3149-downloads
Closed

alvynabranches wants to merge 2 commits into
python:mainfrom
alvynabranches:fix/bugs-audit-3149-downloads

Conversation

@alvynabranches

Copy link
Copy Markdown

Summary

Single PR with per-issue commits from a full audit (Docker test suite + static review).

Commit 1 — Closes #3149: Incorrect link to new documentation issue

CPython's issue template is documentation.yml, not documentation.md.
The old link in this repo opened a blank template. Updated in:

  • .github/ISSUE_TEMPLATE/BUG.yml
  • .github/ISSUE_TEMPLATE/DOCS.yml
  • .github/ISSUE_TEMPLATE/REQUEST.yml

Verified: https://github.com/python/cpython/tree/main/.github/ISSUE_TEMPLATE contains documentation.yml (no .md).

Commit 2 — Downloads URL reversing bugs (found via audit)

  • OS.get_absolute_url used kwarg os_slug, but route download_os_list expects slug (apps/downloads/urls.py:19). Every call raised NoReverseMatch. Fixed in apps/downloads/models.py:58, verified: now returns /downloads/windows/.
  • DownloadLatestPython2.get_redirect_url fallback used bare reverse("download") (no such name) — 500 when no Python 2 flagged latest. Fixed to download:download.
  • DownloadLatestPyManager.get_redirect_url fallback used bare reverse("downloads") — same 500. Fixed to download:download.
  • Standardized DownloadLatestPython3, DownloadLatestPrerelease, and ReleaseFeed.link fallbacks on download:download (both download: and downloads: resolve today, but download: matches the instance namespace used everywhere else).

Added regression tests:

  • test_os_get_absolute_url
  • test_latest_python2_redirects_to_downloads_when_no_latest
  • test_latest_pymanager_redirects_to_downloads_when_no_latest

Issues reviewed with no code change

Verification (per README contributing guide)

  • makemigrations --check --dry-run: No changes detected
  • ruff check: All checks passed; ruff format --check: 655 files formatted
  • Full suite via Docker (coverage run manage.py test): 1072 tests, OK (211s)
  • Coverage: 86% (threshold 75%)
  • Targeted new tests: 5 tests OK; prior downloads-only run: 122 tests OK

The CPython repo uses documentation.yml, not documentation.md.
The old link opened a blank issue template. Updated in BUG, DOCS,
and REQUEST issue templates.
OS.get_absolute_url used os_slug kwarg but route expects slug,
causing NoReverseMatch on every call. Fallback redirects in
DownloadLatestPython2 and DownloadLatestPyManager used bare
names (download/downloads) that do not exist, causing 500 when
no latest release is flagged. Standardize fallbacks and feed
link on download:download namespace.

Added regression tests for OS URL and fallback redirects.
@JacobCoffee

Copy link
Copy Markdown
Member

this does too much, should be scoped PRs please :)

@alvynabranches

Copy link
Copy Markdown
Author

Follow-up audit note (no code changes in this comment):

I swept every literal reverse()/reverse_lazy() call site in non-test code (57 distinct call shapes across apps) and executed each one under Django, plus reviewed the duplicate URL-name report and spot-checked {% url %} tags:

  • All call sites resolve. The only script flags were 3 agreements routes (order_detail, detail, custom_detail) that need UUID pks — verified each resolves with a real UUID (e.g. /agreements/orders/<uuid>/).
  • The only app-level duplicate URL name is the known download_latest_python3 alias (latest/ vs latest/python3/, under both download and getit namespaces). Both patterns serve the same view and redirect identically, so this is benign; left untouched to avoid churn. (Framework-level duplicates from DRF/tastypie/allauth/sitetree are expected.)
  • Template {% url %} tags spot-checked consistent (kwargs match route params); the full suite (1072 tests OK) renders them.

So beyond the two commits in this PR, the audit found no further verified crash-level bugs. Remaining open items (#2677 content, #2847 infra, #3185 process, #3176 covered by #3177) are documented in the PR description as needing non-code handling.

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.

Bug: Incorrect link to new documentation issue

2 participants