-
Notifications
You must be signed in to change notification settings - Fork 378
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
fix: return to ballotpopup #7858
Conversation
holloway
commented
Aug 26, 2024
•
edited
Loading
edited
- update the modal template route to take a param, rather than using request.path
- ensure every place that uses the ballot_icon template tag is added to the allow list of valid 'return to' route handlers.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7858 +/- ##
==========================================
- Coverage 88.78% 88.78% -0.01%
==========================================
Files 296 303 +7
Lines 41320 41430 +110
==========================================
+ Hits 36687 36784 +97
- Misses 4633 4646 +13 ☔ View full report in Codecov by Sentry. |
fyi @jennifer-richards: @rjsparks pointed out that the 'return to' param when coming from a modal doesn't make sense. Bootstrap modals are configured to scrape a modal template route (eg
|
I think there are more, and the current behavior of the ballot modal is odd for pages that aren't included - it just dims the page the grid appears on, but the modal fails to appear. Examples:
I think a careful traceback of any view that can include the ballot icon grid is what's going to have to happen, or the callback guard needs to be relaxed. Could we review the threat model that leads to the current verification list? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, but I think a valid return point was missed. There are a couple minor bits of cleanup I suggest.
ietf/doc/tests_ballot.py
Outdated
@@ -1457,7 +1457,7 @@ class ReturnToUrlTests(TestCase): | |||
def test_invalid_return_to_url(self): | |||
self.assertRaises( | |||
Exception, | |||
lambda: parse_ballot_edit_return_point('/doc/', 'draft-ietf-opsawg-ipfix-tcpo-v6eh', '998718'), | |||
lambda: parse_ballot_edit_return_point('/', 'draft-ietf-opsawg-ipfix-tcpo-v6eh', '998718'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't noticed this pattern before, but you can avoid the lambda by using self.assertRaises()
as a context manager:
with self.assertRaises(Exception):
parse_ballot_edit_return_point(...)
I don't insist on changing this, but I find it a little easier to understand at a glance.
Should change Exception
to ValueError
though - I think that what comes back in every case, and it's better to be specific when possible. (And sorry for not catching this on an earlier review...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. I didn't have a chance to double check that all the places search_result_row.html
is included are in the allowed view list.
@jennifer-richards fyi here's my notes on tracing back from
(those views are in the allowed list) |