From 7882ea1a25b64cbaec22804d48d2e311810fd714 Mon Sep 17 00:00:00 2001 From: FinnStutzenstein Date: Mon, 6 Apr 2020 14:14:00 +0200 Subject: [PATCH 1/2] Added vote weight and fixed named voting --- client/src/app/shared/models/users/user.ts | 9 +++++-- .../0012_assignmetn_vote_unique_together.py | 18 +++++++++++++ openslides/assignments/models.py | 1 + openslides/assignments/views.py | 25 +++++++++++++++++-- .../0035_motion_vote_unique_together.py | 18 +++++++++++++ openslides/motions/models.py | 1 + openslides/motions/views.py | 22 ++++++++-------- openslides/poll/views.py | 13 +++++++--- .../users/migrations/0013_user_vote_weight.py | 22 ++++++++++++++++ openslides/users/models.py | 5 ++++ openslides/users/serializers.py | 3 ++- tests/integration/motions/test_polls.py | 6 ++--- tests/unit/users/test_serializers.py | 8 +++--- 13 files changed, 125 insertions(+), 26 deletions(-) create mode 100644 openslides/assignments/migrations/0012_assignmetn_vote_unique_together.py create mode 100644 openslides/motions/migrations/0035_motion_vote_unique_together.py create mode 100644 openslides/users/migrations/0013_user_vote_weight.py diff --git a/client/src/app/shared/models/users/user.ts b/client/src/app/shared/models/users/user.ts index 89d83e540..1ec91284b 100644 --- a/client/src/app/shared/models/users/user.ts +++ b/client/src/app/shared/models/users/user.ts @@ -1,6 +1,6 @@ import { marker as _ } from '@biesbjerg/ngx-translate-extract-marker'; -import { BaseModel } from '../base/base-model'; +import { BaseDecimalModel } from '../base/base-decimal-model'; /** * Iterable pre selection of genders (sexes) @@ -14,7 +14,7 @@ export type UserAuthType = 'default' | 'saml'; * Representation of a user in contrast to the operator. * @ignore */ -export class User extends BaseModel { +export class User extends BaseDecimalModel { public static COLLECTIONSTRING = 'users/user'; public id: number; @@ -35,8 +35,13 @@ export class User extends BaseModel { public is_active?: boolean; public default_password?: string; public auth_type?: UserAuthType; + public vote_weight: number; public constructor(input?: Partial) { super(User.COLLECTIONSTRING, input); } + + protected getDecimalFields(): string[] { + return ["vote_weight"]; + } } diff --git a/openslides/assignments/migrations/0012_assignmetn_vote_unique_together.py b/openslides/assignments/migrations/0012_assignmetn_vote_unique_together.py new file mode 100644 index 000000000..2a7e822ea --- /dev/null +++ b/openslides/assignments/migrations/0012_assignmetn_vote_unique_together.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.12 on 2020-04-06 11:26 + +from django.conf import settings +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("assignments", "0011_voting_4"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="assignmentvote", unique_together={("user", "option")}, + ), + ] diff --git a/openslides/assignments/models.py b/openslides/assignments/models.py index 4b0361b7d..8c8202b68 100644 --- a/openslides/assignments/models.py +++ b/openslides/assignments/models.py @@ -245,6 +245,7 @@ class AssignmentVote(RESTModelMixin, BaseVote): class Meta: default_permissions = () + unique_together = ("user", "option") class AssignmentOptionManager(BaseManager): diff --git a/openslides/assignments/views.py b/openslides/assignments/views.py index b5ee02529..e130673a9 100644 --- a/openslides/assignments/views.py +++ b/openslides/assignments/views.py @@ -2,6 +2,7 @@ from decimal import Decimal from django.contrib.auth import get_user_model from django.db import transaction +from django.db.utils import IntegrityError from openslides.poll.views import BaseOptionViewSet, BasePollViewSet, BaseVoteViewSet from openslides.utils.auth import has_perm @@ -507,12 +508,18 @@ class AssignmentPollViewSet(BasePollViewSet): def create_votes_type_named_pseudoanonymous( self, data, poll, check_user, vote_user ): - """ check_user is used for the voted-array, vote_user is the one put into the vote """ + """ + check_user is used for the voted-array and weight of the vote, + vote_user is the one put into the vote + """ options = poll.get_options() for option_id, result in data.items(): option = options.get(pk=option_id) vote = AssignmentVote.objects.create( - option=option, user=vote_user, value=result + option=option, + user=vote_user, + value=result, + weight=check_user.vote_weight, ) inform_changed_data(vote, no_delete_on_restriction=True) inform_changed_data(option, no_delete_on_restriction=True) @@ -520,6 +527,13 @@ class AssignmentPollViewSet(BasePollViewSet): poll.voted.add(check_user) def handle_named_vote(self, data, poll, user): + try: + with transaction.atomic(): + return self.try_handle_named_vote(data, poll, user) + except IntegrityError: + raise ValidationError({"detail": "You have already voted"}) + + def try_handle_named_vote(self, data, poll, user): if user in poll.voted.all(): raise ValidationError({"detail": "You have already voted"}) @@ -532,6 +546,13 @@ class AssignmentPollViewSet(BasePollViewSet): self.create_votes_type_named_pseudoanonymous(data, poll, user, user) def handle_pseudoanonymous_vote(self, data, poll, user): + try: + with transaction.atomic(): + return self.try_handle_pseudoanonymous_vote(data, poll, user) + except IntegrityError: + raise ValidationError({"detail": "You have already voted"}) + + def try_handle_pseudoanonymous_vote(self, data, poll, user): if user in poll.voted.all(): raise ValidationError({"detail": "You have already voted"}) diff --git a/openslides/motions/migrations/0035_motion_vote_unique_together.py b/openslides/motions/migrations/0035_motion_vote_unique_together.py new file mode 100644 index 000000000..0678fba94 --- /dev/null +++ b/openslides/motions/migrations/0035_motion_vote_unique_together.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.12 on 2020-04-06 11:26 + +from django.conf import settings +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("motions", "0034_voting_2"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="motionvote", unique_together={("user", "option")}, + ), + ] diff --git a/openslides/motions/models.py b/openslides/motions/models.py index 9a78f1b38..7d53b1845 100644 --- a/openslides/motions/models.py +++ b/openslides/motions/models.py @@ -880,6 +880,7 @@ class MotionVote(RESTModelMixin, BaseVote): class Meta: default_permissions = () + unique_together = ("user", "option") class MotionOptionManager(BaseManager): diff --git a/openslides/motions/views.py b/openslides/motions/views.py index cb3fb79bb..01450660d 100644 --- a/openslides/motions/views.py +++ b/openslides/motions/views.py @@ -1,4 +1,3 @@ -from decimal import Decimal from typing import List, Set import jsonschema @@ -1223,29 +1222,32 @@ class MotionPollViewSet(BasePollViewSet): elif poll.pollmethod == MotionPoll.POLLMETHOD_YN and data not in ("Y", "N"): raise ValidationError("Data must be Y or N") - if poll.type == MotionPoll.TYPE_PSEUDOANONYMOUS: - if user in poll.voted.all(): - raise ValidationError("You already voted on this poll") - def handle_named_vote(self, data, poll, user): + if user in poll.voted.all(): + raise ValidationError({"detail": "You have already voted"}) + poll.voted.add(user) + poll.save() + option = poll.options.get() - vote, _ = MotionVote.objects.get_or_create(user=user, option=option) + vote = MotionVote.objects.create(user=user, option=option) self.handle_named_and_pseudoanonymous_vote(data, user, poll, option, vote) def handle_pseudoanonymous_vote(self, data, poll, user): + if user in poll.voted.all(): + raise ValidationError({"detail": "You have already voted"}) + poll.voted.add(user) + poll.save() + option = poll.options.get() vote = MotionVote.objects.create(user=None, option=option) self.handle_named_and_pseudoanonymous_vote(data, user, poll, option, vote) def handle_named_and_pseudoanonymous_vote(self, data, user, poll, option, vote): vote.value = data - vote.weight = Decimal("1") + vote.weight = user.vote_weight vote.save(no_delete_on_restriction=True) inform_changed_data(option) - poll.voted.add(user) - poll.save() - class MotionOptionViewSet(BaseOptionViewSet): queryset = MotionOption.objects.all() diff --git a/openslides/poll/views.py b/openslides/poll/views.py index bc7fcc643..1debc08c0 100644 --- a/openslides/poll/views.py +++ b/openslides/poll/views.py @@ -115,6 +115,7 @@ class BasePollViewSet(ModelViewSet): poll.save() @detail_route(methods=["POST"]) + @transaction.atomic def start(self, request, pk): poll = self.get_object() if poll.state != BasePoll.STATE_CREATED: @@ -126,6 +127,7 @@ class BasePollViewSet(ModelViewSet): return Response() @detail_route(methods=["POST"]) + @transaction.atomic def stop(self, request, pk): poll = self.get_object() # Analog polls could not be stopped; they are stopped when @@ -145,6 +147,7 @@ class BasePollViewSet(ModelViewSet): return Response() @detail_route(methods=["POST"]) + @transaction.atomic def publish(self, request, pk): poll = self.get_object() if poll.state != BasePoll.STATE_FINISHED: @@ -157,6 +160,7 @@ class BasePollViewSet(ModelViewSet): return Response() @detail_route(methods=["POST"]) + @transaction.atomic def pseudoanonymize(self, request, pk): poll = self.get_object() @@ -173,6 +177,7 @@ class BasePollViewSet(ModelViewSet): return Response() @detail_route(methods=["POST"]) + @transaction.atomic def reset(self, request, pk): poll = self.get_object() poll.reset() @@ -226,11 +231,11 @@ class BasePollViewSet(ModelViewSet): if poll.state != BasePoll.STATE_STARTED: raise ValidationError("You can only vote on a started poll.") if not request.user.is_present or not in_some_groups( - request.user.id, - list(poll.groups.values_list("pk", flat=True)), - exact=True, + request.user.id, + list(poll.groups.values_list("pk", flat=True)), + exact=True, ): - self.permission_denied(request) + self.permission_denied(request) def parse_vote_value(self, obj, key): """ Raises a ValidationError on incorrect values, including None """ diff --git a/openslides/users/migrations/0013_user_vote_weight.py b/openslides/users/migrations/0013_user_vote_weight.py new file mode 100644 index 000000000..21a59d7c7 --- /dev/null +++ b/openslides/users/migrations/0013_user_vote_weight.py @@ -0,0 +1,22 @@ +# Generated by Django 2.2.12 on 2020-04-06 10:34 + +from decimal import Decimal + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("users", "0012_user_auth_type"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="vote_weight", + field=models.DecimalField( + blank=True, decimal_places=6, default=Decimal("1"), max_digits=15 + ), + ), + ] diff --git a/openslides/users/models.py b/openslides/users/models.py index a1c05723a..af89cc866 100644 --- a/openslides/users/models.py +++ b/openslides/users/models.py @@ -1,4 +1,5 @@ import smtplib +from decimal import Decimal from django.conf import settings from django.contrib.auth.hashers import make_password @@ -159,6 +160,10 @@ class User(RESTModelMixin, PermissionsMixin, AbstractBaseUser): is_committee = models.BooleanField(default=False) + vote_weight = models.DecimalField( + default=Decimal("1"), max_digits=15, decimal_places=6, null=False, blank=True + ) + objects = UserManager() class Meta: diff --git a/openslides/users/serializers.py b/openslides/users/serializers.py index 7b82e2a29..e7444286c 100644 --- a/openslides/users/serializers.py +++ b/openslides/users/serializers.py @@ -25,6 +25,7 @@ USERCANSEESERIALIZER_FIELDS = ( "groups", "is_present", "is_committee", + "vote_weight", ) @@ -38,7 +39,7 @@ USERCANSEEEXTRASERIALIZER_FIELDS = USERCANSEESERIALIZER_FIELDS + ( ) -class UserFullSerializer(ModelSerializer): +class UserSerializer(ModelSerializer): """ Serializer for users.models.User objects. diff --git a/tests/integration/motions/test_polls.py b/tests/integration/motions/test_polls.py index 788e2eb6a..245b33db0 100644 --- a/tests/integration/motions/test_polls.py +++ b/tests/integration/motions/test_polls.py @@ -775,7 +775,7 @@ class VoteMotionPollNamed(TestCase): response = self.client.post( reverse("motionpoll-vote", args=[self.poll.pk]), "A" ) - self.assertHttpStatusVerbose(response, status.HTTP_200_OK) + self.assertHttpStatusVerbose(response, status.HTTP_400_BAD_REQUEST) poll = MotionPoll.objects.get() self.assertEqual(poll.votesvalid, Decimal("1")) self.assertEqual(poll.votesinvalid, Decimal("0")) @@ -783,8 +783,8 @@ class VoteMotionPollNamed(TestCase): self.assertEqual(poll.get_votes().count(), 1) option = poll.options.get() self.assertEqual(option.yes, Decimal("0")) - self.assertEqual(option.no, Decimal("0")) - self.assertEqual(option.abstain, Decimal("1")) + self.assertEqual(option.no, Decimal("1")) + self.assertEqual(option.abstain, Decimal("0")) vote = option.votes.get() self.assertEqual(vote.user, self.admin) diff --git a/tests/unit/users/test_serializers.py b/tests/unit/users/test_serializers.py index 57d15a507..bb28fefee 100644 --- a/tests/unit/users/test_serializers.py +++ b/tests/unit/users/test_serializers.py @@ -1,7 +1,7 @@ from unittest import TestCase from unittest.mock import MagicMock, patch -from openslides.users.serializers import UserFullSerializer +from openslides.users.serializers import UserSerializer from openslides.utils.rest_api import ValidationError @@ -10,7 +10,7 @@ class UserCreateUpdateSerializerTest(TestCase): """ Tests, that the validator raises a ValidationError, if not data is given. """ - serializer = UserFullSerializer() + serializer = UserSerializer() data: object = {} with self.assertRaises(ValidationError): @@ -22,7 +22,7 @@ class UserCreateUpdateSerializerTest(TestCase): Tests, that an empty username is generated. """ generate_username.return_value = "test_value" - serializer = UserFullSerializer() + serializer = UserSerializer() data = {"first_name": "TestName"} new_data = serializer.validate(data) @@ -34,7 +34,7 @@ class UserCreateUpdateSerializerTest(TestCase): Tests, that an empty username is not set in a patch request context. """ view = MagicMock(action="partial_update") - serializer = UserFullSerializer(context={"view": view}) + serializer = UserSerializer(context={"view": view}) data = {"first_name": "TestName"} new_data = serializer.validate(data) From b7b8620153d153fc53d8fd4c72f599f81fa690ab Mon Sep 17 00:00:00 2001 From: Joshua Sangmeister Date: Mon, 6 Apr 2020 16:38:07 +0200 Subject: [PATCH 2/2] removed race condition & cleanup --- client/src/app/shared/models/users/user.ts | 2 +- ...> 0012_assignment_vote_unique_together.py} | 0 openslides/assignments/views.py | 25 +++-------------- openslides/motions/views.py | 14 +++------- openslides/poll/views.py | 27 ++++++++++++++----- 5 files changed, 30 insertions(+), 38 deletions(-) rename openslides/assignments/migrations/{0012_assignmetn_vote_unique_together.py => 0012_assignment_vote_unique_together.py} (100%) diff --git a/client/src/app/shared/models/users/user.ts b/client/src/app/shared/models/users/user.ts index 1ec91284b..00179e534 100644 --- a/client/src/app/shared/models/users/user.ts +++ b/client/src/app/shared/models/users/user.ts @@ -42,6 +42,6 @@ export class User extends BaseDecimalModel { } protected getDecimalFields(): string[] { - return ["vote_weight"]; + return ['vote_weight']; } } diff --git a/openslides/assignments/migrations/0012_assignmetn_vote_unique_together.py b/openslides/assignments/migrations/0012_assignment_vote_unique_together.py similarity index 100% rename from openslides/assignments/migrations/0012_assignmetn_vote_unique_together.py rename to openslides/assignments/migrations/0012_assignment_vote_unique_together.py diff --git a/openslides/assignments/views.py b/openslides/assignments/views.py index e130673a9..6aaba3356 100644 --- a/openslides/assignments/views.py +++ b/openslides/assignments/views.py @@ -2,7 +2,6 @@ from decimal import Decimal from django.contrib.auth import get_user_model from django.db import transaction -from django.db.utils import IntegrityError from openslides.poll.views import BaseOptionViewSet, BasePollViewSet, BaseVoteViewSet from openslides.utils.auth import has_perm @@ -526,17 +525,11 @@ class AssignmentPollViewSet(BasePollViewSet): poll.voted.add(check_user) + def add_user_to_voted_array(self, user, poll): + VotedModel = AssignmentPoll.voted.through + VotedModel.objects.create(assignmentpoll=poll, user=user) + def handle_named_vote(self, data, poll, user): - try: - with transaction.atomic(): - return self.try_handle_named_vote(data, poll, user) - except IntegrityError: - raise ValidationError({"detail": "You have already voted"}) - - def try_handle_named_vote(self, data, poll, user): - if user in poll.voted.all(): - raise ValidationError({"detail": "You have already voted"}) - if poll.pollmethod == AssignmentPoll.POLLMETHOD_VOTES: self.create_votes_type_votes(data, poll, user) elif poll.pollmethod in ( @@ -546,16 +539,6 @@ class AssignmentPollViewSet(BasePollViewSet): self.create_votes_type_named_pseudoanonymous(data, poll, user, user) def handle_pseudoanonymous_vote(self, data, poll, user): - try: - with transaction.atomic(): - return self.try_handle_pseudoanonymous_vote(data, poll, user) - except IntegrityError: - raise ValidationError({"detail": "You have already voted"}) - - def try_handle_pseudoanonymous_vote(self, data, poll, user): - if user in poll.voted.all(): - raise ValidationError({"detail": "You have already voted"}) - if poll.pollmethod == AssignmentPoll.POLLMETHOD_VOTES: self.create_votes_type_votes(data, poll, user) diff --git a/openslides/motions/views.py b/openslides/motions/views.py index 01450660d..857060b95 100644 --- a/openslides/motions/views.py +++ b/openslides/motions/views.py @@ -1176,6 +1176,10 @@ class MotionPollViewSet(BasePollViewSet): return result + def add_user_to_voted_array(self, user, poll): + VotedModel = MotionPoll.voted.through + VotedModel.objects.create(motionpoll=poll, user=user) + def handle_analog_vote(self, data, poll, user): option = poll.options.get() vote, _ = MotionVote.objects.get_or_create(option=option, value="Y") @@ -1223,21 +1227,11 @@ class MotionPollViewSet(BasePollViewSet): raise ValidationError("Data must be Y or N") def handle_named_vote(self, data, poll, user): - if user in poll.voted.all(): - raise ValidationError({"detail": "You have already voted"}) - poll.voted.add(user) - poll.save() - option = poll.options.get() vote = MotionVote.objects.create(user=user, option=option) self.handle_named_and_pseudoanonymous_vote(data, user, poll, option, vote) def handle_pseudoanonymous_vote(self, data, poll, user): - if user in poll.voted.all(): - raise ValidationError({"detail": "You have already voted"}) - poll.voted.add(user) - poll.save() - option = poll.options.get() vote = MotionVote.objects.create(user=None, option=option) self.handle_named_and_pseudoanonymous_vote(data, user, poll, option, vote) diff --git a/openslides/poll/views.py b/openslides/poll/views.py index 1debc08c0..0b27719ac 100644 --- a/openslides/poll/views.py +++ b/openslides/poll/views.py @@ -2,6 +2,7 @@ from textwrap import dedent from django.contrib.auth.models import AnonymousUser from django.db import transaction +from django.db.utils import IntegrityError from rest_framework import status from openslides.utils.auth import in_some_groups @@ -184,6 +185,7 @@ class BasePollViewSet(ModelViewSet): return Response() @detail_route(methods=["POST"]) + @transaction.atomic def vote(self, request, pk): """ For motion polls: Just "Y", "N" or "A" (if pollmethod is "YNA") @@ -219,10 +221,10 @@ class BasePollViewSet(ModelViewSet): def assert_can_vote(self, poll, request): """ - Raises a permission denied, if the user is not allowed to vote. + Raises a permission denied, if the user is not allowed to vote (or has already voted). + Adds the user to the voted array, so this needs to be reverted on error! Analog: has to have manage permissions Named & Pseudoanonymous: has to be in a poll group and present - Note: For pseudoanonymous it is *not* tested, if the user has already voted! """ if poll.type == BasePoll.TYPE_ANALOG: if not self.has_manage_permissions(): @@ -231,11 +233,17 @@ class BasePollViewSet(ModelViewSet): if poll.state != BasePoll.STATE_STARTED: raise ValidationError("You can only vote on a started poll.") if not request.user.is_present or not in_some_groups( - request.user.id, - list(poll.groups.values_list("pk", flat=True)), - exact=True, + request.user.id, + list(poll.groups.values_list("pk", flat=True)), + exact=True, ): - self.permission_denied(request) + self.permission_denied(request) + + try: + self.add_user_to_voted_array(request.user, poll) + inform_changed_data(poll) + except IntegrityError: + raise ValidationError({"detail": "You have already voted"}) def parse_vote_value(self, obj, key): """ Raises a ValidationError on incorrect values, including None """ @@ -257,6 +265,13 @@ class BasePollViewSet(ModelViewSet): """ pass + def add_user_to_voted_array(self, user, poll): + """ + To be implemented by subclass. Adds the given user to the voted array of the given poll. + Throws an IntegrityError if the user already exists in the array + """ + raise NotImplementedError() + def validate_vote_data(self, data, poll, user): """ To be implemented by subclass. Validates the data according to poll type and method and fields by validated versions.