Merge branch 'tu_vote_decision' into 'master'

Allow Trusted Users to change their proposal decisions

See merge request archlinux/aurweb!477

aurweb/models/tu_vote.py

@@ -6,6 +6,17 @@ from aurweb.models.declarative import Base
 from aurweb.models.tu_voteinfo import TUVoteInfo as _TUVoteInfo
 from aurweb.models.user import User as _User
 
+YES_ID = 1
+NO_ID = 2
+ABSTAIN_ID = 3
+
+DECISIONS = {
+    YES_ID: "Yes",
+    NO_ID: "No",
+    ABSTAIN_ID: "Abstain",
+}
+DECISION_IDS = {v: k for k, v in DECISIONS.items()}
+
 
 class TUVote(Base):
     __table__ = schema.TU_Votes
@@ -29,10 +40,20 @@ class TUVote(Base):
             raise IntegrityError(
                 statement="Foreign key VoteID cannot be null.",
                 orig="TU_Votes.VoteID",
-                params=("NULL"))
+                params=("NULL",))
 
         if not self.User and not self.UserID:
             raise IntegrityError(
                 statement="Foreign key UserID cannot be null.",
                 orig="TU_Votes.UserID",
-                params=("NULL"))
+                params=("NULL",))
+
+        if self.Decision is None or self.Decision not in DECISIONS:
+            raise IntegrityError(
+                statement=("Decision column must be a valid key in "
+                           "aurweb.models.tu_vote.DECISIONS"),
+                orig="TU_Votes.Decision",
+                params=(self.Decision,))
+
+    def display(self) -> str:
+        return DECISIONS.get(self.Decision)

aurweb/routers/trusted_user.py

@@ -2,6 +2,7 @@ import html
 import typing
 
 from http import HTTPStatus
+from typing import Any, Dict, Optional, Tuple
 
 from fastapi import APIRouter, Form, HTTPException, Request
 from fastapi.responses import RedirectResponse, Response
@@ -10,8 +11,9 @@ from sqlalchemy import and_, func, or_
 from aurweb import db, l10n, logging, models, time
 from aurweb.auth import creds, requires_auth
 from aurweb.exceptions import handle_form_exceptions
-from aurweb.models import User
+from aurweb.models import TUVote, TUVoteInfo, User
 from aurweb.models.account_type import TRUSTED_USER_AND_DEV_ID, TRUSTED_USER_ID
+from aurweb.models.tu_vote import DECISION_IDS, DECISIONS
 from aurweb.templates import make_context, make_variable_context, render_template
 
 router = APIRouter()
@@ -152,8 +154,10 @@ async def trusted_user_proposal(request: Request, proposal: int):
     context = await make_variable_context(request, "Trusted User")
     proposal = int(proposal)
 
-    voteinfo = db.query(models.TUVoteInfo).filter(
+    with db.begin():
-        models.TUVoteInfo.ID == proposal).first()
+        voteinfo = db.query(models.TUVoteInfo).filter(
+            models.TUVoteInfo.ID == proposal).first()
+
     if not voteinfo:
         raise HTTPException(status_code=HTTPStatus.NOT_FOUND)
 
@@ -166,13 +170,43 @@ async def trusted_user_proposal(request: Request, proposal: int):
         context["error"] = "Only Trusted Users are allowed to vote."
     if voteinfo.User == request.user.Username:
         context["error"] = "You cannot vote in an proposal about you."
-    elif vote is not None:
-        context["error"] = "You've already voted for this proposal."
 
-    context["vote"] = vote
     return render_proposal(request, context, proposal, voteinfo, voters, vote)
 
 
+def judge_decision(request: Request, context: Dict[str, Any],
+                   voteinfo: TUVoteInfo, vote: Optional[TUVote],
+                   decision_id: int) -> Tuple[HTTPStatus, str]:
+    """ Decide if a given decision by request is valid.
+
+    A non-HTTPStatus.OK status_code value indicates that judge_decision
+    ran into an error. context's error key is set when this happens.
+
+    :param request: FastAPI Request
+    :param context: FastAPI template context
+    :param voteinfo: TUVoteInfo instance
+    :param vote: TUVote instance
+    :param decision_id: YES_ID, NO_ID, or ABSTAIN_ID
+    :return: (status_code, old_decision) tuple
+    """
+    old_decision = None
+    status_code = HTTPStatus.OK
+    if not request.user.has_credential(creds.TU_VOTE):
+        context["error"] = "Only Trusted Users are allowed to vote."
+        status_code = HTTPStatus.UNAUTHORIZED
+    elif not voteinfo.is_running():
+        context["error"] = "Voting is closed for this proposal."
+        status_code = HTTPStatus.BAD_REQUEST
+    elif voteinfo.User == request.user.Username:
+        context["error"] = "You cannot vote in an proposal about you."
+        status_code = HTTPStatus.BAD_REQUEST
+    elif vote is not None:
+        if vote.Decision is not None and vote.Decision != decision_id:
+            old_decision = DECISIONS.get(vote.Decision)
+
+    return (status_code, old_decision)
+
+
 @router.post("/tu/{proposal}")
 @handle_form_exceptions
 @requires_auth
@@ -189,39 +223,40 @@ async def trusted_user_proposal_post(request: Request, proposal: int,
     if not voteinfo:
         raise HTTPException(status_code=HTTPStatus.NOT_FOUND)
 
+    if decision not in DECISION_IDS:
+        return Response("Invalid 'decision' value.",
+                        status_code=HTTPStatus.BAD_REQUEST)
+    decision_id = DECISION_IDS.get(decision)
+
     voters = db.query(models.User).join(models.TUVote).filter(
         models.TUVote.VoteID == voteinfo.ID)
     vote = db.query(models.TUVote).filter(
         and_(models.TUVote.UserID == request.user.ID,
              models.TUVote.VoteID == voteinfo.ID)).first()
 
-    status_code = HTTPStatus.OK
+    status_code, old_decision = judge_decision(
-    if not request.user.has_credential(creds.TU_VOTE):
+        request, context, voteinfo, vote, decision_id)
-        context["error"] = "Only Trusted Users are allowed to vote."
-        status_code = HTTPStatus.UNAUTHORIZED
-    elif voteinfo.User == request.user.Username:
-        context["error"] = "You cannot vote in an proposal about you."
-        status_code = HTTPStatus.BAD_REQUEST
-    elif vote is not None:
-        context["error"] = "You've already voted for this proposal."
-        status_code = HTTPStatus.BAD_REQUEST
-
     if status_code != HTTPStatus.OK:
         return render_proposal(request, context, proposal,
                                voteinfo, voters, vote,
                                status_code=status_code)
 
-    if decision in {"Yes", "No", "Abstain"}:
-        # Increment whichever decision was given to us.
-        setattr(voteinfo, decision, getattr(voteinfo, decision) + 1)
-    else:
-        return Response("Invalid 'decision' value.",
-                        status_code=HTTPStatus.BAD_REQUEST)
-
     with db.begin():
-        vote = db.create(models.TUVote, User=request.user, VoteInfo=voteinfo)
+        # If the decision was changed, decrement the old decision.
+        if old_decision is not None:
+            setattr(voteinfo, old_decision,
+                    getattr(voteinfo, old_decision) - 1)
+
+        # In all cases, increment the new decision.
+        setattr(voteinfo, decision, getattr(voteinfo, decision) + 1)
+
+        # Create the vote if doesn't exist yet.
+        if not vote:
+            vote = db.create(models.TUVote, User=request.user,
+                             VoteInfo=voteinfo, Decision=decision_id)
+        else:
+            vote.Decision = decision_id
 
-    context["error"] = "You've already voted for this proposal."
     return render_proposal(request, context, proposal, voteinfo, voters, vote)
 
 

aurweb/schema.py

@@ -406,6 +406,7 @@ TU_Votes = Table(
     'TU_Votes', metadata,
     Column('VoteID', ForeignKey('TU_VoteInfo.ID', ondelete='CASCADE'), nullable=False),
     Column('UserID', ForeignKey('Users.ID', ondelete='CASCADE'), nullable=False),
+    Column('Decision', TINYINT(unsigned=True)),
     mysql_engine='InnoDB',
 )
 

migrations/versions/84cd90073dde_relocate_vote_decisions_into_tuvote.py

@@ -0,0 +1,61 @@
+"""relocate vote decisions into TUVote
+
+Revision ID: 84cd90073dde
+Revises: d64e5571bc8d
+Create Date: 2022-03-07 18:02:28.791103
+
+This migration allows us to give Trusted Users the ability to
+modify a vote they made on a proposal. Previously, the decision
+was tracked purely through TU_VoteInfo records, which removes
+tracking of what decisions users made. This blocks us from being
+able to modify votes, because we can't update the TU_VoteInfo
+Yes, No or Abstain columns properly.
+
+"""
+import sqlalchemy as sa
+
+from alembic import op
+from sqlalchemy.dialects.mysql import TINYINT
+
+from aurweb import db, logging, time
+from aurweb.models import TUVoteInfo
+
+logger = logging.get_logger("alembic")
+
+# revision identifiers, used by Alembic.
+revision = '84cd90073dde'
+down_revision = 'd64e5571bc8d'
+branch_labels = None
+depends_on = None
+
+TABLE = "TU_Votes"
+
+
+def upgrade():
+    decision = sa.Column("Decision", TINYINT(unsigned=True))
+    op.add_column(TABLE, decision)
+
+    # For each proposal which is running at the time this migration
+    # is applied, eradicate all related votes. We will restart the votes.
+    # In addition, reset the Submitted and End columns based on the current
+    # timestamp; essentially resetting the proposal's state.
+    utcnow = time.utcnow()
+    running_proposals = db.query(TUVoteInfo).filter(TUVoteInfo.End > utcnow)
+    with db.begin():
+        for proposal in running_proposals:
+            logger.info(f"Resetting proposal with ID {proposal.ID}: "
+                        "Yes = 0, No = 0, Abstain = 0, deleted vote records")
+
+            # Reset the Submitted and End columns.
+            length = proposal.End - proposal.Submitted
+            proposal.Submitted = utcnow
+            proposal.End = utcnow + length
+
+            proposal.Yes = proposal.No = proposal.Abstain = 0
+            db.delete_all(proposal.tu_votes)
+            logger.info(f"Proposal time range was reset: Submitted = "
+                        f"{proposal.Submitted} and End = {proposal.End}")
+
+
+def downgrade():
+    op.drop_column(TABLE, "Decision")

po/aurweb.pot

@@ -2334,3 +2334,11 @@ msgid "This action will close any pending package requests "
 "related to it. If %sComments%s are omitted, a closure "
 "comment will be autogenerated."
 msgstr ""
+
+#: templates/partials/tu/proposal/details.html
+msgid "Your vote"
+msgstr ""
+
+#: templates/partials/tu/proposal/details.html
+msgid "You can change your vote while the proposal is still running."
+msgstr ""

templates/partials/tu/proposal/details.html

@@ -2,7 +2,11 @@
 
 {% if voteinfo.is_running() %}
     <p class="vote-running" style="font-weight: bold; color: red">
-        {% trans %}This vote is still running.{% endtrans %}
+        {% trans %}This vote is still running.{% endtrans %}<br />
+        {% if vote %}
+            {{ "You've already voted for this proposal." | tr }}
+            {{ "You can change your vote while the proposal is still running." | tr }}
+        {% endif %}
     </p>
 {% endif %}
 
@@ -39,6 +43,13 @@
         </strong>
     </div>
 
+    {% if vote and vote.Decision %}
+        <div class="field">
+            {{ "Your vote" | tr }}:
+            <strong>{{ vote.display() }}</strong>
+        </div>
+    {% endif %}
+
     {% if not voteinfo.is_running() %}
         <div class="field result">
             {{ "Result" | tr }}:

test/test_trusted_user_routes.py

@@ -1,3 +1,4 @@
+import html
 import re
 
 from http import HTTPStatus
@@ -11,7 +12,7 @@ from fastapi.testclient import TestClient
 
 from aurweb import config, db, filters, time
 from aurweb.models.account_type import DEVELOPER_ID, TRUSTED_USER_ID, AccountType
-from aurweb.models.tu_vote import TUVote
+from aurweb.models.tu_vote import ABSTAIN_ID, YES_ID, TUVote
 from aurweb.models.tu_voteinfo import TUVoteInfo
 from aurweb.models.user import User
 from aurweb.testing.requests import Request
@@ -173,7 +174,7 @@ def test_tu_empty_index(client, tu_user):
     assert len(tables) == 0
 
 
-def test_tu_index(client, tu_user):
+def test_tu_index(client: TestClient, tu_user: User):
     ts = time.utcnow()
 
     # Create some test votes: (Agenda, Start, End).
@@ -197,7 +198,8 @@ def test_tu_index(client, tu_user):
         vote_record = vote_records[1]
         vote_record.Yes += 1
         vote_record.ActiveTUs += 1
-        db.create(TUVote, VoteInfo=vote_record, User=tu_user)
+        db.create(TUVote, VoteInfo=vote_record, User=tu_user,
+                  Decision=YES_ID)
 
     cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
     with client as request:
@@ -462,8 +464,8 @@ def test_tu_index_last_votes(client: TestClient, tu_user: User, tu_user2: User,
                              Submitter=tu_user)
 
         # Create a vote on it from tu_user.
-        db.create(TUVote, VoteInfo=voteinfo, User=tu_user)
+        db.create(TUVote, VoteInfo=voteinfo, User=tu_user, Decision=YES_ID)
-        db.create(TUVote, VoteInfo=voteinfo, User=tu_user2)
+        db.create(TUVote, VoteInfo=voteinfo, User=tu_user2, Decision=YES_ID)
 
     # Now, check that tu_user got populated in the .last-votes table.
     cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
@@ -575,7 +577,7 @@ def test_tu_running_proposal(client: TestClient,
 
     # Create a vote.
     with db.begin():
-        db.create(TUVote, VoteInfo=voteinfo, User=tu_user)
+        db.create(TUVote, VoteInfo=voteinfo, User=tu_user, Decision=YES_ID)
         voteinfo.ActiveTUs += 1
         voteinfo.Yes += 1
 
@@ -585,16 +587,21 @@ def test_tu_running_proposal(client: TestClient,
             "/tu", params={"id": voteinfo.ID}, cookies=cookies)
     assert response.status_code == int(HTTPStatus.OK)
 
+    # Check that we're told we've voted.
+    # Check that our vote decision is displayed.
+    content = html.unescape(response.text)
+    assert "You've already voted for this proposal." in content
+    expected = "You can change your vote while the proposal is still running."
+    assert expected in content
+    assert "Your vote:" in content
+    assert "<strong>Yes</strong>" in content
+
     # Parse our new root.
     root = parse_root(response.text)
 
-    # Check that we no longer have a voting form.
+    # Check that we still have a voting form.
     form = root.xpath('//form[contains(@class, "action-form")]')
-    assert not form
+    assert form is not None
-
-    # Check that we're told we've voted.
-    status = root.xpath('//span[contains(@class, "status")]/text()')[0]
-    assert status == "You've already voted for this proposal."
 
 
 def test_tu_ended_proposal(client, proposal):
@@ -604,11 +611,11 @@ def test_tu_ended_proposal(client, proposal):
     with db.begin():
         voteinfo.End = ts - 5  # 5 seconds ago.
 
-    # Initiate an authenticated GET request to /tu/{proposal_id}.
+    # Initiate an authenticated GET request to /tu/{voteinfo.ID}.
-    proposal_id = voteinfo.ID
     cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
+    endpoint = f"/tu/{voteinfo.ID}"
     with client as request:
-        response = request.get(f"/tu/{proposal_id}", cookies=cookies)
+        response = request.get(f"/tu/{voteinfo.ID}", cookies=cookies)
     assert response.status_code == int(HTTPStatus.OK)
 
     # Alright, now let's continue on to verifying some markup.
@@ -626,7 +633,7 @@ def test_tu_ended_proposal(client, proposal):
     result = result_node.xpath("./span/text()")[0]
     assert result.strip() == "unknown"
 
-    # Check that voting has ended.
+    # Check that the form is gone; voting has ended.
     form = root.xpath('//form[contains(@class, "action-form")]')
     assert not form
 
@@ -634,6 +641,19 @@ def test_tu_ended_proposal(client, proposal):
     status = root.xpath('//span[contains(@class, "status")]/text()')[0]
     assert status == "Voting is closed for this proposal."
 
+    # Perform a POST request and expect the same behavior.
+    data = {"decision": "Yes"}
+    with client as request:
+        resp = request.post(endpoint, data=data, cookies=cookies)
+    assert resp.status_code == HTTPStatus.BAD_REQUEST
+
+    # Repeat the same assertions as we did in the GET request.
+    root = parse_root(resp.text)
+    form = root.xpath('//form[contains(@class, "action-form")]')
+    assert not form
+    status = root.xpath('//span[contains(@class, "status")]/text()')[0]
+    assert status == "Voting is closed for this proposal."
+
 
 def test_tu_proposal_vote_not_found(client, tu_user):
     """ Test POST request to a missing vote. """
@@ -666,11 +686,13 @@ def test_tu_proposal_vote(client, proposal):
                     TUVote.User == tu_user).first()
     assert vote is not None
 
-    root = parse_root(response.text)
+    # Assert that we've gotten the message that we've voted.
-
+    content = html.unescape(response.text)
-    # Check that we're told we've voted.
+    assert "You've already voted for this proposal." in content
-    status = root.xpath('//span[contains(@class, "status")]/text()')[0]
+    expected = "You can change your vote while the proposal is still running."
-    assert status == "You've already voted for this proposal."
+    assert expected in content
+    assert "Your vote:" in content
+    assert "<strong>Yes</strong>" in content
 
 
 def test_tu_proposal_vote_unauthorized(
@@ -731,36 +753,6 @@ def test_tu_proposal_vote_cant_self_vote(client, proposal):
     assert status == "You cannot vote in an proposal about you."
 
 
-def test_tu_proposal_vote_already_voted(client, proposal):
-    tu_user, user, voteinfo = proposal
-
-    with db.begin():
-        db.create(TUVote, VoteInfo=voteinfo, User=tu_user)
-        voteinfo.Yes += 1
-        voteinfo.ActiveTUs += 1
-
-    cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
-    with client as request:
-        data = {"decision": "Yes"}
-        response = request.post(f"/tu/{voteinfo.ID}", cookies=cookies,
-                                data=data, allow_redirects=False)
-    assert response.status_code == int(HTTPStatus.BAD_REQUEST)
-
-    root = parse_root(response.text)
-    status = root.xpath('//span[contains(@class, "status")]/text()')[0]
-    assert status == "You've already voted for this proposal."
-
-    with client as request:
-        data = {"decision": "Yes"}
-        response = request.get(f"/tu/{voteinfo.ID}", cookies=cookies,
-                               data=data, allow_redirects=False)
-    assert response.status_code == int(HTTPStatus.OK)
-
-    root = parse_root(response.text)
-    status = root.xpath('//span[contains(@class, "status")]/text()')[0]
-    assert status == "You've already voted for this proposal."
-
-
 def test_tu_proposal_vote_invalid_decision(client, proposal):
     tu_user, user, voteinfo = proposal
 
@@ -880,3 +872,32 @@ def test_tu_addvote_post_bylaws(client: TestClient, tu_user: User):
     with client as request:
         response = request.post("/addvote", cookies=cookies, data=data)
     assert response.status_code == int(HTTPStatus.SEE_OTHER)
+
+
+def test_tu_change_vote(client: TestClient, tu_user: User,
+                        proposal: TUVoteInfo):
+    _, _, voteinfo = proposal
+    cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
+    endpoint = f"/tu/{voteinfo.ID}"
+
+    # First, make an Abstain vote.
+    data = {"decision": "Abstain"}
+    with client as request:
+        resp = request.post(endpoint, data=data, cookies=cookies)
+    assert resp.status_code == HTTPStatus.OK
+
+    vote = db.query(TUVote).filter(TUVote.VoteID == voteinfo.ID).first()
+    assert vote is not None
+    assert vote.Decision == ABSTAIN_ID
+    assert voteinfo.Abstain == 1
+
+    # Changed our mind! Vote yes, instead.
+    data = {"decision": "Yes"}
+    with client as request:
+        resp = request.post(endpoint, data=data, cookies=cookies)
+    assert resp.status_code == HTTPStatus.OK
+
+    # Expect that the records got changed correctly.
+    assert vote.Decision == YES_ID
+    assert voteinfo.Abstain == 0
+    assert voteinfo.Yes == 1

test/test_tu_vote.py

@@ -4,7 +4,7 @@ from sqlalchemy.exc import IntegrityError
 
 from aurweb import db, time
 from aurweb.models.account_type import TRUSTED_USER_ID
-from aurweb.models.tu_vote import TUVote
+from aurweb.models.tu_vote import YES_ID, TUVote
 from aurweb.models.tu_voteinfo import TUVoteInfo
 from aurweb.models.user import User
 
@@ -36,10 +36,12 @@ def tu_voteinfo(user: User) -> TUVoteInfo:
 
 def test_tu_vote_creation(user: User, tu_voteinfo: TUVoteInfo):
     with db.begin():
-        tu_vote = db.create(TUVote, User=user, VoteInfo=tu_voteinfo)
+        tu_vote = db.create(TUVote, User=user, VoteInfo=tu_voteinfo,
+                            Decision=YES_ID)
 
     assert tu_vote.VoteInfo == tu_voteinfo
     assert tu_vote.User == user
+    assert tu_vote.Decision == YES_ID
     assert tu_vote in user.tu_votes
     assert tu_vote in tu_voteinfo.tu_votes
 
@@ -52,3 +54,12 @@ def test_tu_vote_null_user_raises_exception(tu_voteinfo: TUVoteInfo):
 def test_tu_vote_null_voteinfo_raises_exception(user: User):
     with pytest.raises(IntegrityError):
         TUVote(User=user)
+
+
+def test_tu_vote_bad_decision_raises_exception(user: User,
+                                               tu_voteinfo: TUVoteInfo):
+    with pytest.raises(IntegrityError):
+        TUVote(User=user, VoteInfo=tu_voteinfo, Decision=None)
+
+    with pytest.raises(IntegrityError):
+        TUVote(User=user, VoteInfo=tu_voteinfo, Decision=0)

test/test_tuvotereminder.py

@@ -5,6 +5,7 @@ import pytest
 from aurweb import config, db, time
 from aurweb.models import TUVote, TUVoteInfo, User
 from aurweb.models.account_type import TRUSTED_USER_ID
+from aurweb.models.tu_vote import YES_ID
 from aurweb.scripts import tuvotereminder as reminder
 from aurweb.testing.email import Email
 
@@ -13,7 +14,8 @@ aur_location = config.get("options", "aur_location")
 
 def create_vote(user: User, voteinfo: TUVoteInfo) -> TUVote:
     with db.begin():
-        vote = db.create(TUVote, User=user, VoteID=voteinfo.ID)
+        vote = db.create(TUVote, User=user, VoteID=voteinfo.ID,
+                         Decision=YES_ID)
     return vote