PR#100: Make the FPCA+1 requirement used by FESCo optional - elections

elections

#100 Make the FPCA+1 requirement used by FESCo optional

Merged 2 years ago by bcotton. Opened 2 years ago by bcotton.

fix_fpca_plusone into develop

Make the FPCA+1 requirement used by FESCo optional

Ben Cotton • 2 years ago

896f420

alembic/versions/523f7d5c58d7_make_fpca_1_optional.py

file added

+28

		`@@ -0,0 +1,28 @@`
		`+ """Make FPCA+1 optional`
		`+`
		`+ Revision ID: 523f7d5c58d7`
		`+ Revises: 5ecdd55b4af4`
		`+ Create Date: 2021-05-07 15:13:07.218758`
		`+`
		`+ """`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = '523f7d5c58d7'`
		`+ down_revision = '5ecdd55b4af4'`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ def upgrade():`
		`+ """Add the FPCA+1 requirement option to elections"""`
		`+ op.add_column(`
		`+ 'requires_plusone',`
		`+ sa.Column(sa.Boolean, default=False)`
		`+ )`
		`+`
		`+ def downgrade():`
		`+ """Drop the FPCA+1 requirement option"""`
		`+ op.remove_column(`
		`+ 'requires_plusone'`
		`+ )`

fedora_elections/admin.py

file modified

		`@@ -88,6 +88,7 @@`
		`url_badge=form.url_badge.data,`
		`voting_type=form.voting_type.data,`
		`max_votes=form.max_votes.data,`
		`+ requires_plusone=form.requires_plusone.data,`
		`candidates_are_fasusers=int(form.candidates_are_fasusers.data),`
		`fas_user=flask.g.fas_user.username,`
		`)`

fedora_elections/elections.py

file modified

+10 -8

		`@@ -50,13 +50,6 @@`
		`elif not flask.g.fas_user.cla_done:`
		`flask.flash("You must sign the CLA to vote", "error")`
		`return safe_redirect_back()`
		`- else:`
		`- user_groups = OIDC.user_getfield("groups")`
		`- if len(user_groups) == 0:`
		`- flask.flash(`
		`- "You need to be in one another group than CLA to vote", "error"`
		`- )`
		`- return safe_redirect_back()`

		`return f(args, *kwargs)`

		`@@ -98,8 +91,17 @@`
		`if not isinstance(election, models.Election):`
		`return election`

		`+ user_groups = OIDC.user_getfield("groups")`
		`+`
		`+ # Check if this election requires FPCA +1`
		`+ if election.requires_plusone and len(user_groups) == 0:`
		`+ flask.flash(`
		`+ "You need to be in one another group than CLA to vote", "error"`
		`+ )`
		`+ return safe_redirect_back()`
		`+`
		`+ # Or if the election is restricted to specific groups`
		`if election.legal_voters_list:`
		`- user_groups = OIDC.user_getfield("groups")`
		`if len(set(user_groups).intersection(set(election.legal_voters_list))) == 0:`
		`flask.flash(`
		`"You are not among the groups that are allowed to vote "`

fedora_elections/forms.py

file modified

		`@@ -91,6 +91,8 @@`
		`],`
		`)`

		`+ requires_plusone = wtforms.BooleanField("Election requires FPCA+1")`
		`+`
		`lgl_voters = wtforms.TextField(`
		`"Legal voters groups", [wtforms.validators.optional()]`
		`)`

fedora_elections/models.py

file modified

		`@@ -90,6 +90,7 @@`
		`max_votes = sa.Column(sa.Integer, nullable=True)`
		`candidates_are_fasusers = sa.Column(sa.Integer, nullable=False, default=0)`
		`fas_user = sa.Column(sa.Unicode(50), nullable=False)`
		`+ requires_plusone = sa.Column(sa.Boolean, default=False)`

		`def to_json(self):`
		`""" Return a json representation of this object. """`

fedora_elections/templates/_formhelpers.html

file modified

		`@@ -107,6 +107,7 @@`
		`{{ render_bootstrap_checkbox_in_row(form.candidates_are_fasusers) }}`
		`{{ render_bootstrap_checkbox_in_row(form.embargoed) }}`
		`{{ render_bootstrap_textfield_in_row(form.url_badge) }}`
		`+ {{ render_bootstrap_checkbox_in_row(form.requires_plusone, after="Require membership in any non-FPCA group") }}`
		`{{ render_bootstrap_textfield_in_row(form.lgl_voters, after="FAS groups allowed to vote on this election (CLA-done is always required)") }}`
		`{{ render_bootstrap_textfield_in_row(form.admin_grp, after="FAS groups allowed to view the result despite the embargo") }}`
		`</fieldset>`

tests/test_flask_admin.py

file modified

+78

		`@@ -427,6 +427,84 @@`
		`output_text,`
		`)`

		`+ def test_admin_new_election_requires_plusone(self):`
		`+ """ Test the admin_new_election function. """`
		`+ self.setup_db()`
		`+`
		`+ user = FakeUser(`
		`+ fedora_elections.APP.config["FEDORA_ELECTIONS_ADMIN_GROUP"],`
		`+ username="toshio",`
		`+ )`
		`+`
		`+ with user_set(fedora_elections.APP, user, oidc_id_token="foobar"):`
		`+ with patch(`
		`+ "fedora_elections.OIDC.user_getfield",`
		`+ MagicMock(return_value=["elections"]),`
		`+ ):`
		`+ output = self.app.get("/admin/new")`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ csrf_token = self.get_csrf(output=output)`
		`+`
		`+ # Test that we get the plusone field`
		`+ # Probably not strictly necessary, but worth including`
		`+ # for now since it's a new field`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ 'input id="requires_plusone" '`
		`+ 'name="requires_plusone" type="checkbox" ',`
		`+ output_text,`
		`+ )`
		`+`
		`+ # All good`
		`+ data = {`
		`+ "alias": "new_election2",`
		`+ "shortdesc": "new election2 shortdesc",`
		`+ "description": "new election2 description",`
		`+ "voting_type": "simple",`
		`+ "url": "https://fedoraproject.org",`
		`+ "start_date": TODAY + timedelta(days=2),`
		`+ "end_date": TODAY + timedelta(days=4),`
		`+ "seats_elected": 2,`
		`+ "candidates_are_fasusers": False,`
		`+ "embargoed": True,`
		`+ "admin_grp": "testers, , sysadmin-main,,",`
		`+ "lgl_voters": "testers, packager,,,",`
		`+ "csrf_token": csrf_token,`
		`+ "requires_plusone": True`
		`+ }`
		`+`
		`+ with mock_sends(NewElectionV1):`
		`+ output = self.app.post(`
		`+ "/admin/new", data=data, follow_redirects=True`
		`+ )`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertTrue('Election "new_election2" added' in output_text)`
		`+ self.assertTrue("There are no candidates." in output_text)`
		`+ self.assertIn(`
		`+ 'input class="form-control" id="admin_grp" '`
		`+ 'name="admin_grp" type="text" '`
		`+ 'value="sysadmin-main, testers">',`
		`+ output_text,`
		`+ )`
		`+ self.assertIn(`
		`+ 'input class="form-control" id="lgl_voters" '`
		`+ 'name="lgl_voters" type="text" '`
		`+ 'value="packager, testers">',`
		`+ output_text,`
		`+ )`
		`+ self.assertIn(`
		`+ 'input class="form-control" id="max_votes" '`
		`+ 'name="max_votes" type="text" '`
		`+ 'value="">',`
		`+ output_text,`
		`+ )`
		`+`
		`+ obj = fedora_elections.models.Election.get(self.session, "new_election2")`
		`+ self.assertNotEqual(obj, None)`
		`+ self.assertEqual(obj.requires_plusone, True)`
		`+`
		`def test_admin_edit_election(self):`
		`""" Test the admin_edit_election function. """`
		`user = FakeUser(`

tests/test_flask_elections.py

file modified

+24 -9

		`@@ -56,8 +56,30 @@`

		`output = self.app.get("/vote/test_election", follow_redirects=True)`
		`output_text = output.get_data(as_text=True)`
		`- self.assertTrue(`
		`- "The election, test_election, does not exist." in output_text`
		`+ self.assertIn(`
		`+ "The election, test_election, does not exist.", output_text`
		`+ )`
		`+`
		`+ def test_vote_require_fpca(self):`
		`+ """ Test the vote function. """`
		`+ self.setup_db()`
		`+ obj = fedora_elections.models.Election.get(self.session, "test_election3")`
		`+ self.assertNotEqual(obj, None)`
		`+ obj.requires_plusone = True`
		`+ self.session.add(obj)`
		`+ self.session.commit()`
		`+`
		`+ user = FakeUser([], username="pingou")`
		`+ with user_set(fedora_elections.APP, user, oidc_id_token="foobar"):`
		`+ with patch(`
		`+ "fedora_elections.OIDC.user_getfield", MagicMock(return_value=[])`
		`+ ):`
		`+`
		`+ output = self.app.get("/vote/test_election3", follow_redirects=True)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ "You need to be in one another group than "`
		`+ "CLA to vote", output_text`
		`)`

		`def test_vote(self):`
		`@@ -90,13 +112,6 @@`
		`output = self.app.get("/vote/test_election")`
		`self.assertEqual(output.status_code, 302)`

		`- output = self.app.get("/vote/test_election", follow_redirects=True)`
		`- output_text = output.get_data(as_text=True)`
		`- self.assertTrue(`
		`- "You need to be in one another group than "`
		`- "CLA to vote" in output_text`
		`- )`
		`-`
		`user = FakeUser(["packager"], username="pingou")`
		`with user_set(fedora_elections.APP, user, oidc_id_token="foobar"):`
		`with patch(`

bcotton commented 2 years ago

My local test environment is being uncooperative, so I'd like to get some eyes on this before I rebase it into staging to make sure I didn't miss anything obvious.

bcotton commented 2 years ago

@pingou if you have a moment, can you tell me how incredibly wrong this is? :wink:

pingou commented on line 22 of alembic/versions/523f7d5c58d7_make_fpca_1_optional.py 2 years ago

Shouldn't this be a boolean field? :)

bcotton commented 2 years ago

Shouldn't this be a boolean field? :)

Perhaps. "Candidates are FAS users" is also an integer, so that's what I went with. I'm not sure if there are historical reasons for that or if I'm just carrying a mistake forward. :-)

pingou commented 2 years ago

Shouldn't this be a boolean field? :)

Perhaps. "Candidates are FAS users" is also an integer, so that's what I went with. I'm not sure if there are historical reasons for that or if I'm just carrying a mistake forward. :-)

I'm not sure if it's one or the other, but I think a boolean field here should be sufficient

1 new commit added