Conversation
- Renamed can_add_voters_file() to can_modify_voters() to better reflect that it controls both voter uploads and deletions - Refactored voter_delete view to reuse can_modify_voters() instead of duplicating the check logic - Updated voter list template to hide the delete [x] button when voter modifications are blocked, providing better UX - Updated template variable names: upload_disabled_reason -> modify_voters_disabled_reason for consistency
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #470 by ensuring voter deletions are restricted under the same conditions as voter uploads. The changes improve consistency and user experience by providing unified restrictions for all voter modifications once tallying has started.
Changes:
- Renamed
can_add_voters_file()method tocan_modify_voters()to reflect its broader purpose of controlling both uploads and deletions - Refactored
voter_deleteview to reuse thecan_modify_voters()check instead of only checkingencrypted_tally - Updated voter list template to conditionally hide delete [x] buttons when voter modifications are blocked
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| helios/models.py | Renamed method from can_add_voters_file() to can_modify_voters() and updated documentation to reflect broader purpose |
| helios/views.py | Updated voter_delete, voters_list_pretty, and voters_upload views to use renamed method; improved documentation and variable naming consistency |
| helios/templates/voters_list.html | Updated template to use new variable names and conditionally hide delete buttons based on can_modify_voters permission |
| helios/tests.py | Renamed existing tests for consistency and added new VoterDeleteRestrictionTests class with tests for voter deletion restrictions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2888
to
+2955
| class VoterDeleteRestrictionTests(WebTest): | ||
| """Tests for voter deletion restrictions when tallying has begun (issue #470)""" | ||
| fixtures = ['users.json'] | ||
| allow_database_queries = True | ||
|
|
||
| def setUp(self): | ||
| self.user = auth_models.User.objects.get(user_id='ben@adida.net', user_type='google') | ||
| self.election, _ = models.Election.get_or_create( | ||
| short_name='test-delete-restriction', | ||
| name='Test Delete Restriction', | ||
| description='Test', | ||
| admin=self.user | ||
| ) | ||
| if not self.election.uuid: | ||
| self.election.uuid = str(uuid.uuid4()) | ||
| self.election.save() | ||
| # Create a voter to test deletion | ||
| self.voter = models.Voter.objects.create( | ||
| uuid=str(uuid.uuid4()), | ||
| election=self.election, | ||
| voter_email='voter@test.com', | ||
| voter_name='Test Voter' | ||
| ) | ||
|
|
||
| def setup_login(self): | ||
| self.client.get("/") | ||
| session = self.client.session | ||
| session['user'] = {'type': self.user.user_type, 'user_id': self.user.user_id} | ||
| session.save() | ||
|
|
||
| def test_voter_delete_allowed_by_default(self): | ||
| """Voter deletion should be allowed before tallying starts""" | ||
| self.setup_login() | ||
| response = self.client.post("/helios/elections/%s/voters/%s/delete" % ( | ||
| self.election.uuid, self.voter.uuid)) | ||
| # Should redirect (302) on successful deletion | ||
| self.assertStatusCode(response, 302) | ||
|
|
||
| def test_voter_delete_blocked_when_tallying_started(self): | ||
| """Voter deletion should be blocked once tallying has started""" | ||
| self.setup_login() | ||
| self.election.tallying_started_at = datetime.datetime.utcnow() | ||
| self.election.save() | ||
|
|
||
| response = self.client.post("/helios/elections/%s/voters/%s/delete" % ( | ||
| self.election.uuid, self.voter.uuid)) | ||
| self.assertStatusCode(response, 403) | ||
|
|
||
| def test_voters_list_shows_delete_button_when_allowed(self): | ||
| """Voter list should show delete [x] button when deletion is allowed""" | ||
| self.setup_login() | ||
|
|
||
| response = self.client.get("/helios/elections/%s/voters/list" % self.election.uuid) | ||
| self.assertStatusCode(response, 200) | ||
| # Check for the delete link with [x] text | ||
| self.assertContains(response, '>x</a>]') | ||
|
|
||
| def test_voters_list_hides_delete_button_when_blocked(self): | ||
| """Voter list should hide delete [x] button when tallying has started""" | ||
| self.setup_login() | ||
| self.election.tallying_started_at = datetime.datetime.utcnow() | ||
| self.election.save() | ||
|
|
||
| response = self.client.get("/helios/elections/%s/voters/list" % self.election.uuid) | ||
| self.assertStatusCode(response, 200) | ||
| # Check that the delete link is not present | ||
| self.assertNotContains(response, '>x</a>]') | ||
|
|
There was a problem hiding this comment.
The VoterDeleteRestrictionTests class should include a test case for when encrypted_tally exists, similar to the test_can_modify_voters_blocked_when_encrypted_tally_exists test in VoterUploadRestrictionTests. This would ensure that voter deletion is also blocked when the election has been tallied (not just when tallying has started). Consider adding a test like test_voter_delete_blocked_when_encrypted_tally_exists that verifies the voter deletion endpoint returns 403 when election.encrypted_tally is set.
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.
modify_voters_disabled_reason for consistency