PR#65: Add support for an 'I Voted' Badge - elections

elections

#65 Add support for an 'I Voted' Badge

Merged 5 years ago by pingou. Opened 5 years ago by bcotton.

https://pagure.io/forks/bcotton/elections.git add_voted_badge into develop

Update the field name and correct the nullability

Ben Cotton • 5 years ago

e049a94

Add an alembic migration script for the voted_badge field in elections

Pierre-Yves Chibon • 5 years ago

ea85cd1

Add support for requesting an 'I Voted' badge.

Ben Cotton • 5 years ago

3d093f9

alembic/versions/5ecdd55b4af4_add_badge_support_to_elections.py

file added

+27

		`@@ -0,0 +1,27 @@`
		`+ """Add badge support to elections`
		`+`
		`+ Revision ID: 5ecdd55b4af4`
		`+ Revises: 2b8f5a6f10a4`
		`+ Create Date: 2019-03-18 12:21:59.536380`
		`+`
		`+ """`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = '5ecdd55b4af4'`
		`+ down_revision = '2b8f5a6f10a4'`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ def upgrade():`
		`+ """ Add the url_badge column to the Elections table. """`
		`+ op.add_column(`
		`+ 'elections',`
		`+ sa.Column('voted_badge', sa.Unicode(250), nullable=True)`
		`+ )`
		`+`
		`+`
		`+ def downgrade():`
		`+ """ Drop the url_badge column from the Elections table. """`
		`+ op.drop_column('elections', 'url_badge')`

fedora_elections/admin.py

file modified

		`@@ -74,6 +74,7 @@`
		`end_date=form.end_date.data,`
		`seats_elected=form.seats_elected.data,`
		`embargoed=int(form.embargoed.data),`
		`+ url_badge=form.url_badge.data,`
		`voting_type=form.voting_type.data,`
		`max_votes=form.max_votes.data,`
		`candidates_are_fasusers=int(form.candidates_are_fasusers.data),`

fedora_elections/elections.py

file modified

+12 -4

		`@@ -173,7 +173,7 @@`
		`and candidate.short_name not in ['csrf_token', 'action']`
		`]`
		`process_vote(candidates, election, votes, revote)`
		`- flask.flash("Your vote has been recorded. Thank you!")`
		`+ say_thank_you(election)`
		`return safe_redirect_back()`

		`if form.action.data == 'preview':`
		`@@ -229,7 +229,7 @@`
		`and candidate.short_name not in ['csrf_token', 'action']`
		`]`
		`process_vote(candidates, election, votes, revote, cand_name)`
		`- flask.flash("Your vote has been recorded. Thank you!")`
		`+ say_thank_you(election)`
		`return safe_redirect_back()`

		`if form.action.data == 'preview':`
		`@@ -269,7 +269,7 @@`
		`and candidate.short_name not in ['csrf_token', 'action']`
		`]`
		`process_vote(candidates, election, votes, revote, value=1)`
		`- flask.flash("Your vote has been recorded. Thank you!")`
		`+ say_thank_you(election)`
		`return safe_redirect_back()`

		`if form.action.data == 'preview':`
		`@@ -307,7 +307,7 @@`
		`and candidate.short_name not in ['csrf_token', 'action']`
		`]`
		`process_vote(candidates, election, votes, revote, cand_name)`
		`- flask.flash("Your vote has been recorded. Thank you!")`
		`+ say_thank_you(election)`
		`return safe_redirect_back()`

		`if form.action.data == 'preview':`
		`@@ -350,3 +350,11 @@`
		`)`
		`SESSION.add(new_vote)`
		`SESSION.commit()`
		`+`
		`+`
		`+ def say_thank_you(election):`
		`+ thank_you = "Your vote has been recorded. Thank you!"`
		`+ if election.url_badge:`
		`+ thank_you = thank_you + '<br><a href="' + \`
		`+ election.url_badge + '" target=_new>Claim your I Voted badge</a>.'`
		`+ flask.flash(thank_you)`

fedora_elections/forms.py

file modified

		`@@ -74,6 +74,12 @@`

		`embargoed = wtforms.BooleanField('Embargo results?', default=True)`

		`+ url_badge = wtforms.TextField(`
		`+ 'Badge URL (optional)', [`
		`+ wtforms.validators.Optional(),`
		`+ wtforms.validators.URL(),`
		`+ wtforms.validators.Length(max=250)])`
		`+`
		`lgl_voters = wtforms.TextField(`
		`'Legal voters groups', [wtforms.validators.optional()])`

fedora_elections/models.py

file modified

		`@@ -86,6 +86,7 @@`
		`seats_elected = sa.Column(sa.Integer, nullable=False, default=1)`
		`embargoed = sa.Column(sa.Integer, nullable=False, default=0)`
		`voting_type = sa.Column(sa.Unicode(100), nullable=False, default=u'range')`
		`+ url_badge = sa.Column(sa.Unicode(250), nullable=True)`
		`max_votes = sa.Column(sa.Integer, nullable=True)`
		`candidates_are_fasusers = sa.Column(`
		`sa.Integer, nullable=False, default=0)`

fedora_elections/templates/_formhelpers.html

file modified

		`@@ -112,6 +112,7 @@`
		`{{ render_bootstrap_textfield_in_row(form.seats_elected) }}`
		`{{ 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_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>`

no initial comment

bcotton commented 5 years ago

Replacing PR #60 with an updated version to address feedback and send it to the correct branch.

This incorporates my feature addition as well as @pingou's alembic migration code.

pingou commented 5 years ago

Hm, there are two merge commits in this PR? Could you remove them?

Also you used the remote PR procedure for this? :open_mouth:

pingou commented on line 22 of alembic/versions/5ecdd55b4af4_add_badge_support_to_elections.py 5 years ago

Looks like I've made a typo: Unicde -> Unicode :)

pingou commented 5 years ago

Ok, appart from that typo it seems to work fine.

There is one item I think we should add (but I can do it in a later PR), it's the ability to claim the badge w/o voting. You have to be on the page where you vote but you don't actually have to vote to get the badge.

rebased onto 3d093f9

5 years ago

bcotton commented 5 years ago

@pingou Commits cleaned up and typo corrected.

I'm not sure how I feel about claiming the badge without casting a ballot. I'm not necessarily opposed, I'm just not sold on it.

pingou commented 5 years ago

I believe the idea was to prevent people from linking who voted on what based on results vs when people were claiming badge or something.
To be honest I'm fuzzy on the reason but I remember it was an explicit demand for nuancier. I'm not very much sold on it, but I also don't see the arm.

I have another UI fix (unrelated to this feature) so I was thinking to have the two in one PR, so we can go ahead and get this one in.

What do you think?

bcotton commented 5 years ago

Works for me