Release 1.165.2 - #3935
Merged
Merged
Release 1.165.2#3935
Conversation
…#3923) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Asad Ali <asad.ali@arbisoft.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenAPI ChangesShow/hide changes |
Comment on lines
+438
to
+442
| return HttpResponseRedirect( | ||
| reverse( | ||
| "admin:ecommerce_fulfilledorder_change", args=(order.id,) | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
Bug: The exception handler for a failed refund incorrectly redirects to the disabled ecommerce_fulfilledorder_change view instead of the changelist view.
Severity: LOW
Suggested Fix
Change the HttpResponseRedirect target in the exception block of AdminRefundOrderView.post() from reverse("admin:ecommerce_fulfilledorder_change", ...) to reverse("admin:ecommerce_fulfilledorder_changelist"). This will make it consistent with other error handling in the view. Also, add a test case to cover this exception path.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: ecommerce/admin.py#L438-L442
Potential issue: When a refund fails with an exception in `AdminRefundOrderView.post()`,
the user is redirected to the `ecommerce_fulfilledorder_change` page. However,
`FulfilledOrderAdmin` explicitly disables this view by setting `has_change_permission`
to `False`. This causes an unnecessary and confusing redirect sequence where the user is
sent to a forbidden page, which then likely redirects them to the changelist view. While
the error message is preserved, the initial redirect target is incorrect and
inconsistent with other error handling paths in the same view that correctly redirect to
the changelist. This exception handling path is also not covered by any tests.
Did we get this right? 👍 / 👎 to inform future reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sentry[bot]
Dan Subak
Zaman Afzal
Chris Chudzicki
Tobias Macey