feat: make full_id URL the canonical URL, pk redirects - #150
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (20)
WalkthroughProjects now support canonical 8-character ChangesCanonical project URL flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wafer_space/projects/views.py (1)
89-104: Approve with observation:get_object()called twice per request.The redirect logic correctly ensures audit logging happens before redirecting by calling the parent dispatch first. However,
get_object()is called at line 93 and again insidesuper().dispatch()(via the mixin), resulting in two queries per pk-based request. While not a correctness issue, consider storing the project in a temporary variable if performance becomes a concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wafer_space/projects/tests/test_views.py(2 hunks)wafer_space/projects/urls.py(2 hunks)wafer_space/projects/views.py(4 hunks)wafer_space/templates/projects/admin_summary.html(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,pyi}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint errors must be fixed, never suppressed. Never add
# noqa,# type: ignore, or similar without explicit user permission. Always runmake lint-fix && make lint && make type-checkbefore committing.
Files:
wafer_space/projects/tests/test_views.pywafer_space/projects/urls.pywafer_space/projects/views.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Never create circular imports. Maintain layer separation: Models never import tasks/views/services; Services can import models and tasks; Views can import models and services; Tasks can import models only. Never use local imports as a workaround for circular dependencies.
Avoid print statements. Uselogging.getLogger(__name__)instead (Ruff rule T201).
Lines must not exceed 88 characters (Ruff rule E501). Break long lines into multiple lines.
Never use f-strings directly in exception messages (Ruff rule EM102). Assign the message to a variable first before using it in the exception.
Avoid boolean positional arguments (Ruff rule FBT002/3). Use keyword-only arguments instead:def fn(*, flag=True).
Never hardcode passwords directly in code (Ruff rule S105). Use constants likeTEST_PASSWORD = '...'instead.
All public functions must have type hints. Usefrom __future__ import annotationsfor forward references. Fix mypy errors, don't ignore them.
Use specific exception handling with exception chaining. Catch specific exception types and useraise NewException(msg) from excto chain exceptions. Never use bareexcept:or overly broadexcept Exception:.
Celery task names inCELERY_BEAT_SCHEDULEmust reference the actual decorated function name (e.g.,'app.tasks.process_check_queue'), not a variable assignment. Task registration comes from the @shared_task decorator, not Python variable assignments.
Files:
wafer_space/projects/tests/test_views.pywafer_space/projects/urls.pywafer_space/projects/views.py
**/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.py: Use factory-boy for creating test data, not pytest fixtures. Write one assertion concept per test. Mirror test file structure to source:wafer_space/users/models.py→wafer_space/users/tests/test_models.py.
All browser tests run headless by default. Never use the--visibleflag in automated tests (blocked with error). UseWebDriverWait, nevertime.sleep(). Screenshots are auto-captured on failure totests/browser/screenshots/. Use Page Object pattern for browser tests.
Files:
wafer_space/projects/tests/test_views.py
🧬 Code graph analysis (2)
wafer_space/projects/urls.py (1)
wafer_space/projects/views.py (1)
ProjectDetailByFullIdView(206-241)
wafer_space/projects/views.py (2)
wafer_space/projects/models.py (2)
Project(56-234)full_id(155-162)wafer_space/projects/mixins.py (1)
dispatch(85-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: pytest
🔇 Additional comments (8)
wafer_space/templates/projects/admin_summary.html (1)
172-172: LGTM!The hyperlink improves navigation by making project names clickable. The pk-based URL will automatically redirect to the canonical full_id URL when the project has been assigned to a shuttle.
wafer_space/projects/tests/test_views.py (2)
1485-1485: LGTM!The
follow=Trueparameter is necessary to follow the redirect from the pk-based URL to the canonical full_id URL. The inline comment clearly documents the reason for this change.
1518-1518: LGTM!Consistent with the previous test update, ensuring the test follows the redirect to the canonical URL.
wafer_space/projects/urls.py (2)
4-4: LGTM!The
re_pathimport is necessary for the regex-based full_id URL pattern.
23-28: LGTM!The canonical URL pattern correctly matches the 8-character manufacturing ID format (e.g., G801ABCD). The regex pattern
[A-Z0-9]{8}enforces uppercase alphanumeric characters, which aligns with the project_id model validation. The route ordering prevents conflicts with other patterns likecreate/.wafer_space/projects/views.py (3)
17-17: LGTM!The new imports are necessary for the canonical URL functionality:
Http404for raising 404 errors in full_id lookupSHUTTLE_ID_LENGTHfor parsing the full_id stringHTTP_SUCCESS_THRESHOLDfor checking response status before redirectingAlso applies to: 31-31, 37-37
79-83: LGTM!The docstring clearly documents the new redirect behavior for pk-based access.
218-241: LGTM!The
get_objectmethod correctly parses the full_id into shuttle_name and project_id components and performs proper validation with clear error messages.
There was a problem hiding this comment.
Pull request overview
This PR introduces canonical URLs for projects using their 8-character manufacturing ID (full_id), making projects accessible via /projects/G801ABCD/ instead of only via UUID. It implements automatic redirects from the old pk-based URLs to the new canonical URLs when projects are assigned to a shuttle.
Key changes:
- New URL pattern using regex to match 8-character full_id format
- Automatic redirect from pk URLs to full_id URLs for projects with manufacturing IDs
- Updated tests to follow redirects where needed
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| wafer_space/projects/urls.py | Adds regex URL pattern for full_id-based project access |
| wafer_space/projects/views.py | Implements redirect logic in ProjectDetailView and new ProjectDetailByFullIdView class |
| wafer_space/templates/projects/admin_summary.html | Updates project name links to use pk-based URL (will redirect to full_id) |
| wafer_space/projects/tests/test_views.py | Adds follow=True to tests that will encounter redirects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0e6f171 to
63af819
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wafer_space/projects/views.py (1)
213-216: CRITICAL: full_id detail view bypasses auth/authorization and audit loggingThe combination of
ProjectDetailView.dispatchandProjectDetailByFullIdView.dispatchcurrently allows unauthenticated users to view projects via the full_id URL and also skips audit logging:
ProjectDetailByFullIdView.dispatchcallsDetailView.dispatchdirectly, which bypassesLoginRequiredMixinandProjectOwnerOrStaffMixinin the MRO (no login requirement, no owner/staff checks, no audit logs).- In
ProjectDetailView.dispatch, when accessed via pk:
super().dispatch(...)returns a 302 login redirect for anonymous users (fromLoginRequiredMixin),- this 302 is considered “successful” (
status_code < HTTP_SUCCESS_THRESHOLD),- the code then issues a new redirect to
projects:detail_by_full_id,- the full_id URL is then served by
ProjectDetailByFullIdView.dispatch, which usesDetailView.dispatchand no mixins → full project detail for anonymous users.This is the security bypass and audit gap that prior reviews called out.
You can fix this while keeping the intent (canonical full_id URL) with two targeted changes:
- Make full_id view use the standard mixin dispatch (no bypass):
class ProjectDetailByFullIdView(ProjectDetailView): @@ - def dispatch(self, request, *args, **kwargs): - """Skip the pk redirect logic from parent - we're already at canonical URL.""" - # Call grandparent's dispatch directly to skip ProjectDetailView's redirect - return DetailView.dispatch(self, request, *args, **kwargs) + def dispatch(self, request, *args, **kwargs): + """Use normal mixin dispatch; pk redirect logic is not triggered for full_id.""" + # ProjectDetailView.dispatch only redirects when `pk` is in kwargs, which + # is not the case for this view, so we can safely skip just that override + # while preserving LoginRequiredMixin and ProjectOwnerOrStaffMixin. + return super(ProjectDetailView, self).dispatch(request, *args, **kwargs)(Alternatively, you can delete this override entirely and let the inherited
ProjectDetailView.dispatchhandle full_id requests since they never includepk.)
- Do not convert login redirects into canonical full_id redirects:
- # Only redirect if parent dispatch succeeded (not 403/404) - if parent_response.status_code < HTTP_SUCCESS_THRESHOLD: + # Only redirect when the detail view actually rendered (HTTP 200), + # not for login redirects or other 3xx responses. + if parent_response.status_code == 200: return redirect( "projects:detail_by_full_id", full_id=project.full_id )Together, these changes restore the expected behavior:
- Anonymous users hitting the pk URL still get redirected to login.
- Anonymous users hitting the full_id URL are also forced through
LoginRequiredMixin.- Owner/staff checks and audit logging via
ProjectOwnerOrStaffMixin.dispatchremain intact for both pk and full_id access paths.Also applies to: 89-105
🧹 Nitpick comments (2)
wafer_space/projects/tests/test_views.py (1)
1485-1485: Following redirects in slot-visibility tests is appropriateUsing
follow=Truehere matches the new behavior where pk-based URLs redirect to the canonical full_id URL, so asserting on the final 200 + content still works. If you want to harden coverage later, consider an additional focused test that:
- calls
client.get(url, follow=False)on the pk URL,- asserts the 302 status and that
response["Location"]points atprojects:detail_by_full_id.For these tests specifically, the change looks good as-is.
Also applies to: 1518-1518
wafer_space/projects/views.py (1)
218-241: full_id lookup logic is solid; minor optional refinements onlyThe
get_objectimplementation forProjectDetailByFullIdViewlooks correct:
- Uses
SHUTTLE_ID_LENGTH + PROJECT_ID_LENGTHinstead of hardcoding 8.- Splits
full_idcleanly intoshuttle_nameandproject_id.- Raises
Http404with pre-built messages (EM102-safe) and usesget_object_or_404with explicit filters.Two small, optional tweaks you could consider (not blocking):
- Normalize
full_idto uppercase here (full_id = full_id.upper()) to be robust against any future URL pattern changes that might relax the regex.- Add a brief comment tying the length check to the URL regex (so future maintainers remember to keep them in sync with the route pattern).
Functionally this block is good as written.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wafer_space/projects/tests/test_views.py(2 hunks)wafer_space/projects/urls.py(2 hunks)wafer_space/projects/views.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wafer_space/projects/urls.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,pyi}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint errors must be fixed, never suppressed. Never add
# noqa,# type: ignore, or similar without explicit user permission. Always runmake lint-fix && make lint && make type-checkbefore committing.
Files:
wafer_space/projects/tests/test_views.pywafer_space/projects/views.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Never create circular imports. Maintain layer separation: Models never import tasks/views/services; Services can import models and tasks; Views can import models and services; Tasks can import models only. Never use local imports as a workaround for circular dependencies.
Avoid print statements. Uselogging.getLogger(__name__)instead (Ruff rule T201).
Lines must not exceed 88 characters (Ruff rule E501). Break long lines into multiple lines.
Never use f-strings directly in exception messages (Ruff rule EM102). Assign the message to a variable first before using it in the exception.
Avoid boolean positional arguments (Ruff rule FBT002/3). Use keyword-only arguments instead:def fn(*, flag=True).
Never hardcode passwords directly in code (Ruff rule S105). Use constants likeTEST_PASSWORD = '...'instead.
All public functions must have type hints. Usefrom __future__ import annotationsfor forward references. Fix mypy errors, don't ignore them.
Use specific exception handling with exception chaining. Catch specific exception types and useraise NewException(msg) from excto chain exceptions. Never use bareexcept:or overly broadexcept Exception:.
Celery task names inCELERY_BEAT_SCHEDULEmust reference the actual decorated function name (e.g.,'app.tasks.process_check_queue'), not a variable assignment. Task registration comes from the @shared_task decorator, not Python variable assignments.
Files:
wafer_space/projects/tests/test_views.pywafer_space/projects/views.py
**/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.py: Use factory-boy for creating test data, not pytest fixtures. Write one assertion concept per test. Mirror test file structure to source:wafer_space/users/models.py→wafer_space/users/tests/test_models.py.
All browser tests run headless by default. Never use the--visibleflag in automated tests (blocked with error). UseWebDriverWait, nevertime.sleep(). Screenshots are auto-captured on failure totests/browser/screenshots/. Use Page Object pattern for browser tests.
Files:
wafer_space/projects/tests/test_views.py
🧬 Code graph analysis (1)
wafer_space/projects/views.py (2)
wafer_space/projects/models.py (2)
Project(56-234)full_id(155-162)wafer_space/projects/mixins.py (1)
dispatch(85-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest
63af819 to
5d7963a
Compare
5d7963a to
f3fad69
Compare
46a71b1 to
a6ca7ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wafer_space/projects/models.py (1)
61-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
PROJECT_ID_LENGTHinstead of hardcoding the length in the message.To prevent the error message from becoming stale if the required ID length changes, consider using the
PROJECT_ID_LENGTHconstant here. As per coding guidelines, assigning the f-string to themsgvariable first ensures compliance with the rule against using f-strings directly in exception initializations.♻️ Proposed refactor
if len(value) != PROJECT_ID_LENGTH: - msg = "Project ID must be exactly 4 characters" + msg = f"Project ID must be exactly {PROJECT_ID_LENGTH} characters" raise ValidationError(msg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wafer_space/projects/models.py` around lines 61 - 63, Update the validation message in the project ID length check to interpolate the existing PROJECT_ID_LENGTH constant instead of hardcoding 4, assigning the resulting f-string to msg before raising ValidationError.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wafer_space/notifications/views.py`:
- Around line 74-78: Remove the blanket contextlib.suppress(Exception) wrapper
around the notification redirect logic. In the notification view, either let
unexpected failures from hasattr, the project relation, or
project.get_absolute_url propagate, or catch only the specific expected
relation/URL exceptions while preserving the redirect for valid ProjectFile
notifications and the existing fallback behavior.
In `@wafer_space/projects/tests/test_views.py`:
- Around line 430-431: Replace the direct Shuttle.objects.create call in the
affected tests with ShuttleFactory, passing the existing name and description
values; apply this consistently to all test shuttles in the file while
preserving the current test data and ProjectFactory usage.
In `@wafer_space/projects/views.py`:
- Around line 104-124: Add explicit type annotations to the view override
methods dispatch and get_object, covering their request/argument parameters and
return types in the style expected by the project’s mypy configuration. Preserve
the existing redirect, permission, and superclass behavior.
---
Nitpick comments:
In `@wafer_space/projects/models.py`:
- Around line 61-63: Update the validation message in the project ID length
check to interpolate the existing PROJECT_ID_LENGTH constant instead of
hardcoding 4, assigning the resulting f-string to msg before raising
ValidationError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b01ba358-04ab-4be2-81ab-5685913842bc
📒 Files selected for processing (20)
tests/browser/test_admin_project_access.pywafer_space/notifications/views.pywafer_space/projects/forms.pywafer_space/projects/models.pywafer_space/projects/tests/test_shuttle_integration.pywafer_space/projects/tests/test_validators.pywafer_space/projects/tests/test_views.pywafer_space/projects/urls.pywafer_space/projects/views.pywafer_space/projects/views_compliance.pywafer_space/shuttles/models.pywafer_space/shuttles/templates/shuttles/assignment_dashboard.htmlwafer_space/shuttles/tests/test_validators.pywafer_space/templates/projects/admin_summary.htmlwafer_space/templates/projects/compliance_certification_form.htmlwafer_space/templates/projects/manufacturability_check_status.htmlwafer_space/templates/projects/project_confirm_delete.htmlwafer_space/templates/projects/project_file_submit_url.htmlwafer_space/templates/projects/project_form.htmlwafer_space/templates/projects/project_list.html
| with contextlib.suppress(Exception): | ||
| # For ProjectFile, redirect to the project detail page | ||
| if hasattr(notification.content_object, "project"): | ||
| return redirect( | ||
| "projects:detail", | ||
| pk=notification.content_object.project.pk, | ||
| ) | ||
| project = notification.content_object.project | ||
| return redirect(project.get_absolute_url()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline wafer_space/notifications/views.py \
--match mark_notification_read --view expanded
rg -n -C3 'GenericForeignKey|content_object|related.*project' wafer_spaceRepository: wafer-space/platform.wafer.space
Length of output: 14052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' wafer_space/notifications/views.py
printf '\n---\n'
rg -n "def get_absolute_url|class ProjectFile|class Project" wafer_space/projects -n -C 3Repository: wafer-space/platform.wafer.space
Length of output: 18446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '359,430p' wafer_space/projects/models.py
printf '\n---\n'
sed -n '762,860p' wafer_space/projects/models.py
printf '\n---\n'
python3 - <<'PY'
from pathlib import Path
text = Path("wafer_space/projects/models.py").read_text()
for needle in ["class ProjectFile", "project = models.ForeignKey", "get_absolute_url(self)"]:
print(f"## {needle}")
idx = text.find(needle)
print(idx)
PYRepository: wafer-space/platform.wafer.space
Length of output: 6365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 "content_object\s*=|Notification\(|notify|ProjectFile" wafer_space/notifications wafer_space/projects wafer_space/shuttlesRepository: wafer-space/platform.wafer.space
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 "content_object\s*=|Notification\.objects\.create|NotificationFactory|notification.*ProjectFile|ProjectFileFactory" wafer_space/notifications wafer_space/projects --glob '!**/migrations/**' --glob '!**/docs/**'Repository: wafer-space/platform.wafer.space
Length of output: 25746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' wafer_space/notifications/services.py
printf '\n---\n'
sed -n '1,220p' wafer_space/notifications/tests/test_views.pyRepository: wafer-space/platform.wafer.space
Length of output: 4374
Remove the blanket exception suppression in the notification redirect
contextlib.suppress(Exception) here hides bugs in hasattr(...) and project.get_absolute_url() and always falls back to the list on unexpected failures. Catch only the specific relation/URL exceptions you expect, or drop the wrapper entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wafer_space/notifications/views.py` around lines 74 - 78, Remove the blanket
contextlib.suppress(Exception) wrapper around the notification redirect logic.
In the notification view, either let unexpected failures from hasattr, the
project relation, or project.get_absolute_url propagate, or catch only the
specific expected relation/URL exceptions while preserving the redirect for
valid ProjectFile notifications and the existing fallback behavior.
Source: Coding guidelines
| shuttle = Shuttle.objects.create(name="G881", description="List run") | ||
| ProjectFactory(user=user, shuttle=shuttle) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'factories.py' wafer_space -0 |
xargs -0 rg -n -C2 'class ShuttleFactory|DjangoModelFactory'Repository: wafer-space/platform.wafer.space
Length of output: 4208
🏁 Script executed:
#!/bin/bash
sed -n '420,620p' wafer_space/projects/tests/test_views.py
printf '\n---\n'
sed -n '1,120p' wafer_space/shuttles/tests/factories.pyRepository: wafer-space/platform.wafer.space
Length of output: 9057
Use ShuttleFactory for these test shuttles. wafer_space/shuttles/tests/factories.py already defines ShuttleFactory, so replace the direct Shuttle.objects.create(...) calls in this file with the factory for consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wafer_space/projects/tests/test_views.py` around lines 430 - 431, Replace the
direct Shuttle.objects.create call in the affected tests with ShuttleFactory,
passing the existing name and description values; apply this consistently to all
test shuttles in the file while preserving the current test data and
ProjectFactory usage.
Source: Coding guidelines
| def dispatch(self, request, *args, **kwargs): | ||
| """Redirect to the canonical full_id URL when accessed by pk. | ||
|
|
||
| Only authenticated, authorised users are redirected. Everyone else | ||
| falls through to ``super()`` and gets the response they would have | ||
| got without this view: anonymous users the login redirect, and | ||
| unauthorised users a 403. Redirecting them instead would disclose | ||
| the project's existence and its manufacturing ID to someone who is | ||
| not allowed to see it, which the permission mixin exists to prevent. | ||
| """ | ||
| if "pk" in kwargs and request.user.is_authenticated: | ||
| project = self.get_object() | ||
| if project.full_id and self.test_func(): | ||
| # Carry the query string over: the canonical URL should answer | ||
| # the same request the pk URL was asked, not a truncated one. | ||
| url = project.get_absolute_url() | ||
| query_string = request.META.get("QUERY_STRING", "") | ||
| if query_string: | ||
| url = f"{url}?{query_string}" | ||
| return redirect(url) | ||
| return super().dispatch(request, *args, **kwargs) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and the mentioned line ranges.
git ls-files wafer_space/projects/views.py
wc -l wafer_space/projects/views.py
sed -n '1,180p' wafer_space/projects/views.py
echo '---'
sed -n '220,280p' wafer_space/projects/views.pyRepository: wafer-space/platform.wafer.space
Length of output: 9108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,140p' wafer_space/projects/views.py | cat -n
echo '---'
sed -n '228,270p' wafer_space/projects/views.py | cat -n
echo '---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("wafer_space/projects/views.py").read_text()
for name in ["dispatch", "get_object"]:
for m in re.finditer(rf"^\s*def {name}\(([^)]*)\):", text, re.M):
print(name, "signature:", m.group(0))
PYRepository: wafer-space/platform.wafer.space
Length of output: 4768
Add type hints to these view overrides
dispatch and get_object are still unannotated; add explicit parameter and return types so the new public methods stay covered by mypy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wafer_space/projects/views.py` around lines 104 - 124, Add explicit type
annotations to the view override methods dispatch and get_object, covering their
request/argument parameters and return types in the style expected by the
project’s mypy configuration. Preserve the existing redirect, permission, and
superclass behavior.
Source: Coding guidelines
Adds support for accessing projects by their 8-character manufacturing ID (e.g., G801ABCD) in addition to the UUID primary key. The full_id URL redirects to the project detail page. This makes it easier to share project links using the shorter manufacturing ID that appears on physical chips. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changes the URL routing so that: - /projects/<full_id>/ (e.g., G801ABCD) is now the canonical URL - /projects/<uuid:pk>/ redirects to the full_id URL if project has one - Projects without full_id (no shuttle assignment) still work via pk Also adds the Name field link in admin summary page to project detail. Updates affected tests to follow redirects where needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The pk->full_id redirect ran before any permission check. Defining dispatch() on ProjectDetailView puts it ahead of LoginRequiredMixin in the MRO, and LoginRequiredMixin.dispatch returns handle_no_permission() without calling super(), so the login gate never got the chance to run. An anonymous caller holding a pk got a 302 to /projects/G801ABCD/, learning both that the project exists and its manufacturing ID; an unknown pk 404'd instead of redirecting to login, disclosing existence. Before this branch all three cases returned the same login redirect. Redirect only authenticated, authorised users; everyone else falls through to super() and gets the response the permission mixin intends. This also drops the status_code < HTTP_SUCCESS_THRESHOLD guard, which could not do what its comment claimed: PermissionDenied and Http404 are raised, not returned, so the guard never saw a 403/404 -- the only non-2xx it ever caught was the anonymous login redirect, which it then discarded in favour of leaking the full_id. Not calling super().dispatch() on the redirect leg means the mixin never runs there, so the leg no longer creates a duplicate audit log on its own. skip_success_audit_log only existed to suppress that, and is reverted: mixins.py is now untouched by this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191GNhqKS4cyZdnMnbZewFt
The branch added a URL, a view and a redirect without tests of its own, only adjusting existing ones to follow the new redirect. Cover the canonical URL (owner 200, staff 200, non-owner 403, anonymous login redirect, unknown id 404, and that lowercase/wrong-length ids do not route), the redirect itself (302 to the full_id URL, no redirect when the project has no shuttle), and that staff pk access records exactly one audit log attributed to ProjectDetailByFullIdView. Two of these pin the disclosure fix and fail against the previous implementation: anonymous access to a pk must reach the login page without revealing the full_id, and an unknown pk must be indistinguishable from a known one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191GNhqKS4cyZdnMnbZewFt
The branch introduced a canonical URL but gave callers no way to ask for it, so every link and redirect still had to spell out the pk route by hand. Add the standard Django hook, returning the manufacturing-ID URL once the project is on a shuttle and falling back to the pk URL while it has no full_id. This puts the "which URL is canonical?" rule in one place, so callers stop having to know that full_id exists or when it is populated. Placed after save() per the Django Style Guide ordering ruff enforces (DJ012): get_absolute_url comes before custom methods. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191GNhqKS4cyZdnMnbZewFt
Nothing actually linked to the canonical URL: all 10 template links and all 10 redirect/reverse call sites still emitted the pk route, so the "canonical" URL was reached almost exclusively via the extra 302 hop the redirect exists to serve as a fallback for old bookmarks. Route them all through get_absolute_url(). Links now point at the manufacturing-ID URL directly, and the pk redirect goes back to being what it was meant to be: a fallback, not the main path. No call site names a detail route any more -- projects:detail and projects:detail_by_full_id now appear only inside get_absolute_url(). Also stop the redirect dropping the query string. It rebuilt the target from the route alone, so any parameters on a pk URL were silently discarded on the way to the canonical one. Nothing reads GET params on this view today, so this fixes a trap rather than a live bug: the canonical URL should answer the request the pk URL was asked. The redirect stays a 302: full_id derives from a mutable shuttle FK, so a permanently cached 301 would strand anyone whose project is reassigned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191GNhqKS4cyZdnMnbZewFt
str.isalnum() and str.isdigit() are Unicode-aware, but the canonical URL
pattern is ASCII-only ([A-Z0-9]{8}). The gap let IDs through that no
amount of reversing could turn into a URL:
"ÀBCD".isalnum() -> True and "ÀBCD".upper() == "ÀBCD", so it
cleared the uppercase check too
fullwidth "88" .isdigit() -> True and int() reads it as 88, so it
cleared the shuttle range check too
Project.save() calls full_clean(), so the validator was the only gate,
and ProjectForm.clean_project_id duplicated the same weak check (it
uppercases first, which turns "àbcd" into a passing "ÀBCD"). A project
saved this way then raised NoReverseMatch from get_absolute_url() -- not
only on its own page but on the project list, the admin summary and the
shuttle assignment dashboard, so one user's input could 500 pages for
every member of staff, with no UI to undo it.
Check ASCII explicitly, matching validate_crowd_supply_order_id, which
had already learned this. Error messages and their order are unchanged:
"abcd" still reports "must be uppercase", not "must be alphanumeric".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0191GNhqKS4cyZdnMnbZewFt
Routing every row's link through get_absolute_url() made the list read
full_id, which touches the shuttle FK -- one extra query per project.
Measured with 20 projects: 25 -> 45 queries for an owner, 5 -> 25 for
staff, exactly one shuttle SELECT per row.
Select it. The owner path was also missing select_related("user"), which
is why it started 20 queries worse than staff, so both paths now come in
at 5 -- better than before this branch rather than merely even.
Guard it with a test asserting the count does not change when rows are
added, which is what an N+1 actually is. Pinning an absolute number would
just rot. Missing that guard is why this regression got through.
Also assert the disclosure tests check disclosure: one claimed a
non-owner "learns no full_id" while only checking for 403, and the other
never confirmed the known-pk response was itself a login redirect, which
was the half of "indistinguishable" that mattered.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0191GNhqKS4cyZdnMnbZewFt
a6ca7ff to
ca30dfc
Compare
Makes the 8-character manufacturing ID (e.g.
G801ABCD) the canonical URL for a project. Everything links to it directly; the UUIDpkURL redirects to it as a fallback for old bookmarks.What's here
urls.py— adds/projects/<full_id>/(detail_by_full_id). The[A-Z0-9]{8}pattern is provably disjoint from the UUIDpkroute and from every literal route (create/,check-project-id/, …), so ordering between them is cosmetic.models.py—Project.get_absolute_url(): the manufacturing-ID URL once the project is on a shuttle, else the pk URL. This is the single place that knows which URL is canonical.views.py—ProjectDetailByFullIdViewresolvesfull_idintoshuttle__name+project_id(covered by aUniqueConstraint, so the lookup is single-row and indexed).ProjectDetailView.dispatch()redirectspk→ canonical, preserving the query string.redirect()/reverse()sites now go throughget_absolute_url(). No call site names a detail route any more.Security fix during review
The first cut of the redirect ran before any permission check, turning the detail view into an existence oracle:
Before this branch, all such cases returned an identical login redirect.
Why it happened: defining
dispatch()onProjectDetailViewplaces it ahead ofLoginRequiredMixinin the MRO, andLoginRequiredMixin.dispatchreturnshandle_no_permission()without callingsuper(). So the login gate never got the chance to run — the override sat above it.Fix: only authenticated, authorised users are redirected; everyone else falls through to
super()and gets the response the permission mixin intends (login redirect, or 403). Two regression tests pin this and fail against the previous implementation.This also removed a guard that could not do what its comment claimed:
PermissionDeniedandHttp404are raised, not returned — Django converts them to responses after the view returns, so this guard never saw a 403/404. The only non-2xx it ever caught was the anonymous login redirect (302 < 400), which it then discarded in favour of leaking the full_id.Because the redirect leg no longer calls
super().dispatch(), the permission mixin never runs there and can't create a duplicate audit log — so theskip_success_audit_logflag this branch had added is gone andmixins.pyis untouched.Canonical URL is now actually used
Previously nothing linked to the canonical URL, so it was reached almost exclusively via the extra 302 hop. All 20 call sites now emit it directly:
project_list,admin_summary,manufacturability_check_status(×2),project_form,project_confirm_delete,project_file_submit_url,compliance_certification_form(×2),shuttles/assignment_dashboardviews.py(2 ×get_success_url, 4 ×redirect),views_compliance.py(×3),notifications/views.pyprojects:detail/projects:detail_by_full_idnow appear only insideget_absolute_url().Query strings preserved
The redirect rebuilt its target from the route alone, silently discarding any parameters. Nothing reads GET params on this view today, so this closed a trap rather than a live bug. 302 is deliberate (not 301):
full_idderives from a mutableshuttleFK, so a permanently cached redirect would strand anyone whose project is reassigned.Test coverage added
TestProjectDetailByFullIdViewTestProjectDetailByFullIdViewTestProjectDetailByFullIdViewTestProjectDetailPkRedirecttest_anonymous_is_sent_to_login_and_learns_no_full_idtest_anonymous_gets_login_redirect_for_unknown_pktest_redirect_preserves_query_stringProjectDetailByFullIdViewtest_staff_pk_access_logs_once_against_canonical_viewget_absolute_urlprefers full_id, falls back to pkTestProjectShuttlePropertiesVerification
make lint,make type-check,make testand the djlint pre-commit hooks all clean locally (1486 passed, 3 skipped). No# noqa/# type: ignoreadded.Summary by CodeRabbit
New Features
Bug Fixes
Performance