From b6c9b30077f0bbaacd496d1a68df0da7f1cd99ac Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Mon, 1 Apr 2024 10:14:39 -0300 Subject: [PATCH 1/8] models: add fields to allow non delegate users to declare intention to review a patch Users interested in reviewing a Patch are linked by the `planning_to_review` attribute on a Patch through the PatchReviewIntention model. Also added a field to State to indicate how long a review intention can last before it expires Signed-off-by: andrepapoti --- .../0047_patch_has_planned_review_and_more.py | 71 +++++++++++++++++++ patchwork/models.py | 16 +++++ 2 files changed, 87 insertions(+) create mode 100644 patchwork/migrations/0047_patch_has_planned_review_and_more.py diff --git a/patchwork/migrations/0047_patch_has_planned_review_and_more.py b/patchwork/migrations/0047_patch_has_planned_review_and_more.py new file mode 100644 index 00000000..c5291df5 --- /dev/null +++ b/patchwork/migrations/0047_patch_has_planned_review_and_more.py @@ -0,0 +1,71 @@ +# Generated by Django 5.0.4 on 2024-04-16 04:31 + +import datetime +import django.db.models.deletion +import django.utils.timezone +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('patchwork', '0046_patch_comment_events'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AddField( + model_name='patch', + name='has_planned_review', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='state', + name='review_intention_expiration_time', + field=models.DurationField(default=datetime.timedelta(days=30)), + ), + migrations.CreateModel( + name='PatchReviewIntention', + fields=[ + ( + 'id', + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name='ID', + ), + ), + ( + 'last_time_marked_for_review', + models.DateTimeField(default=django.utils.timezone.now), + ), + ( + 'patch', + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to='patchwork.patch', + ), + ), + ( + 'user', + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + 'unique_together': {('patch', 'user')}, + }, + ), + migrations.AddField( + model_name='patch', + name='planning_to_review', + field=models.ManyToManyField( + related_name='planning_to_review', + through='patchwork.PatchReviewIntention', + to=settings.AUTH_USER_MODEL, + ), + ), + ] diff --git a/patchwork/models.py b/patchwork/models.py index 9a619bc5..6a133777 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -245,6 +245,9 @@ class State(models.Model): slug = models.SlugField(max_length=100, unique=True) ordering = models.IntegerField(unique=True) action_required = models.BooleanField(default=True) + review_intention_expiration_time = models.DurationField( + default=datetime.timedelta(days=30) + ) def __str__(self): return self.name @@ -499,6 +502,10 @@ class Patch(SubmissionMixin): null=True, on_delete=models.CASCADE, ) + planning_to_review = models.ManyToManyField( + User, through='PatchReviewIntention', related_name='planning_to_review' + ) + has_planned_review = models.BooleanField(default=False) state = models.ForeignKey(State, null=True, on_delete=models.CASCADE) archived = models.BooleanField(default=False) hash = HashField(null=True, blank=True) @@ -729,6 +736,15 @@ class Meta: ] +class PatchReviewIntention(models.Model): + patch = models.ForeignKey(Patch, on_delete=models.CASCADE) + user = models.ForeignKey(User, on_delete=models.CASCADE) + last_time_marked_for_review = models.DateTimeField(default=tz_utils.now) + + class Meta: + unique_together = [('patch', 'user')] + + class CoverComment(EmailMixin, models.Model): cover = models.ForeignKey( Cover, From 1f81a0e3980cf796f3ddb01870bd8d12097bad21 Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Mon, 1 Apr 2024 10:26:35 -0300 Subject: [PATCH 2/8] permissions: create a PatchDetailPermission to allow non delegate users to edit it Signed-off-by: andrepapoti --- patchwork/api/patch.py | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index 443c3822..4c0ade3d 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -17,6 +17,7 @@ from rest_framework.relations import RelatedField from rest_framework.reverse import reverse from rest_framework.serializers import SerializerMethodField +from rest_framework import permissions from rest_framework import status from patchwork.api.base import BaseHyperlinkedModelSerializer @@ -30,6 +31,7 @@ from patchwork.models import Patch from patchwork.models import PatchRelation from patchwork.models import State +from patchwork.models import User from patchwork.parser import clean_subject @@ -373,6 +375,26 @@ def get_queryset(self): ) +class PatchDetailPermission(PatchworkPermission): + non_delegate_editable_fields = set(['planning_to_review']) + + def has_object_permission(self, request, view, obj): + if request.method in permissions.SAFE_METHODS: + return True + + data = request.data + + if set(data.keys()).issubset(self.non_delegate_editable_fields): + user_id = data['planning_to_review'][0]['user'] + reviewing_user = User.objects.get(id=user_id) + if request.user == reviewing_user: + return True + detail = "Only the user can declare it's own intention to reviewing a patch" + raise PermissionDenied(detail=detail) + else: + return super().has_object_permission(self, request, view, obj) + + class PatchDetail(RetrieveUpdateAPIView): """ get: @@ -385,7 +407,7 @@ class PatchDetail(RetrieveUpdateAPIView): Update a patch. """ - permission_classes = (PatchworkPermission,) + permission_classes = (PatchDetailPermission,) serializer_class = PatchDetailSerializer def get_queryset(self): From b6b4989fc304a7fb3ebb5d8805844aa585236bc0 Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Mon, 1 Apr 2024 10:41:12 -0300 Subject: [PATCH 3/8] api: Update Patch related serializers to accomodate for the new 'planning_to_review' field Signed-off-by: andrepapoti --- patchwork/api/patch.py | 65 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 12 deletions(-) diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index 4c0ade3d..f32a2f16 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -8,6 +8,7 @@ import email.parser from django.core.exceptions import ValidationError +from django.utils import timezone from django.utils.text import slugify from django.utils.translation import gettext_lazy as _ from rest_framework.exceptions import APIException @@ -29,6 +30,7 @@ from patchwork.api.embedded import UserSerializer from patchwork.api.filters import PatchFilterSet from patchwork.models import Patch +from patchwork.models import PatchReviewIntention from patchwork.models import PatchRelation from patchwork.models import State from patchwork.models import User @@ -78,12 +80,41 @@ class PatchConflict(APIException): ) +class PatchReviewIntentionSerializer(BaseHyperlinkedModelSerializer): + user = UserSerializer() + patch = PatchSerializer() + is_stale = SerializerMethodField() + + def get_is_stale(self, review_intention): + expiration_time = ( + review_intention.patch.state.review_intention_expiration_time + ) + valid_until = ( + review_intention.last_time_marked_for_review + expiration_time + ) + + return timezone.now() > valid_until + + class Meta: + model = PatchReviewIntention + fields = [ + 'id', + 'user', + 'patch', + 'last_time_marked_for_review', + 'is_stale', + ] + + class PatchListSerializer(BaseHyperlinkedModelSerializer): web_url = SerializerMethodField() project = ProjectSerializer(read_only=True) state = StateField() submitter = PersonSerializer(read_only=True) delegate = UserSerializer(allow_null=True) + planning_to_review = PatchReviewIntentionSerializer( + source='patchreviewintention_set', many=True + ) mbox = SerializerMethodField() series = SeriesSerializer(read_only=True) comments = SerializerMethodField() @@ -172,6 +203,8 @@ class Meta: 'hash', 'submitter', 'delegate', + 'planning_to_review', + 'has_planned_review', 'mbox', 'series', 'comments', @@ -190,6 +223,7 @@ class Meta: 'name', 'hash', 'submitter', + 'has_planned_review', 'mbox', 'series', 'comments', @@ -230,16 +264,11 @@ def get_headers(self, patch): def get_prefixes(self, instance): return clean_subject(instance.name)[1] - def update(self, instance, validated_data): - # d-r-f cannot handle writable nested models, so we handle that - # specifically ourselves and let d-r-f handle the rest - if 'related' not in validated_data: - return super(PatchDetailSerializer, self).update( - instance, validated_data - ) - - related = validated_data.pop('related') + def update_planning_to_review(self, instance, patchreviewintention_set): + intereted_user = patchreviewintention_set.pop()['user'] + instance.planning_to_review.add(intereted_user.id) + def update_related(self, instance, related): # Validation rules # ---------------- # @@ -280,9 +309,7 @@ def update(self, instance, validated_data): if instance.related and instance.related.patches.count() == 2: instance.related.delete() instance.related = None - return super(PatchDetailSerializer, self).update( - instance, validated_data - ) + return # break before make relations = {patch.related for patch in patches if patch.related} @@ -306,6 +333,20 @@ def update(self, instance, validated_data): instance.related = relation instance.save() + def update(self, instance, validated_data): + # d-r-f cannot handle writable nested models, so we handle that + # specifically ourselves and let d-r-f handle the rest + + if 'related' in validated_data: + related = validated_data.pop('related') + self.update_related(instance, related) + + if 'patchreviewintention_set' in validated_data: + patchreviewintention_set = validated_data.pop( + 'patchreviewintention_set' + ) + self.update_planning_to_review(instance, patchreviewintention_set) + return super(PatchDetailSerializer, self).update( instance, validated_data ) From 9890f64921de829521ab60c04902d0c7bb817138 Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Mon, 1 Apr 2024 10:42:43 -0300 Subject: [PATCH 4/8] tests: add tests for users declaring intention to review a patch Signed-off-by: andrepapoti --- patchwork/tests/api/test_patch.py | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index 2661d75c..4ca88749 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -12,6 +12,7 @@ from rest_framework import status from patchwork.models import Patch +from patchwork.models import PatchReviewIntention from patchwork.tests.api import utils from patchwork.tests.utils import create_maintainer from patchwork.tests.utils import create_patch @@ -456,3 +457,37 @@ def test_delete(self): self.client.authenticate(user=user) resp = self.client.delete(self.api_url(patch.id)) self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code) + + def test_declare_review_intention(self): + project = create_project() + state = create_state() + patch = create_patch(project=project, state=state) + user = create_user() + + self.client.authenticate(user=user) + resp = self.client.patch( + self.api_url(patch.id), + {'planning_to_review': [{'user': user.id, 'patch': patch.id}]}, + ) + + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(len(PatchReviewIntention.objects.all()), 1) + self.assertEqual( + len(PatchReviewIntention.objects.filter(user=user, patch=patch)), 1 + ) + + def test_declare_review_intention_for_different_user(self): + project = create_project() + state = create_state() + patch = create_patch(project=project, state=state) + user_a = create_user() + user_b = create_user() + + self.client.authenticate(user=user_a) + resp = self.client.patch( + self.api_url(patch.id), + {'planning_to_review': [{'user': user_b.id, 'patch': patch.id}]}, + ) + + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + self.assertEqual(len(PatchReviewIntention.objects.all()), 0) From 81ffed2b655dae0f1fe8797a44b7233851dee88b Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Mon, 1 Apr 2024 10:44:53 -0300 Subject: [PATCH 5/8] models: Override 'create' for PatchComment to automatically remove the PatchReviewIntention for the commenting user Signed-off-by: andrepapoti --- patchwork/models.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/patchwork/models.py b/patchwork/models.py index 6a133777..7214ebd8 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -822,6 +822,12 @@ def save(self, *args, **kwargs): super(PatchComment, self).save(*args, **kwargs) self.patch.refresh_tag_counts() + def create(self, *args, **kwargs): + submitter = kwargs.get('submitter') + patch = kwargs.get('patch') + PatchReviewIntention.objects.delete(user=submitter.user, patch=patch) + super(PatchComment, self).create(*args, **kwargs) + def delete(self, *args, **kwargs): super(PatchComment, self).delete(*args, **kwargs) self.patch.refresh_tag_counts() From 6005c30519cca8782588e2551b9f0f38059c02dc Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Mon, 1 Apr 2024 10:46:26 -0300 Subject: [PATCH 6/8] forms: update MuliplePatchForm to acocmodate for the new 'planning_to_review' field Signed-off-by: andrepapoti --- patchwork/forms.py | 34 +++++++++++++++++++++++++++++++++- patchwork/models.py | 5 +++-- patchwork/views/__init__.py | 6 ++++-- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/patchwork/forms.py b/patchwork/forms.py index ed06d0d1..367d83a5 100644 --- a/patchwork/forms.py +++ b/patchwork/forms.py @@ -190,7 +190,7 @@ class MultiplePatchForm(forms.Form): empty_value='*', ) - def __init__(self, project, *args, **kwargs): + def __init__(self, project, user=None, *args, **kwargs): super(MultiplePatchForm, self).__init__(*args, **kwargs) self.fields['delegate'] = OptionalModelChoiceField( queryset=_get_delegate_qs(project=project), required=False @@ -198,6 +198,19 @@ def __init__(self, project, *args, **kwargs): self.fields['state'] = OptionalModelChoiceField( queryset=State.objects.all() ) + self.user = user + if self.user: + self.fields['review_status'] = OptionalBooleanField( + choices=[ + ('*', 'no change'), + ('True', 'Planning to review'), + ('False', 'Not planning to review'), + ], + coerce=lambda x: x == 'True', + empty_value='*', + required=False, + initial='*', + ) def save(self, instance, commit=True): opts = instance.__class__._meta @@ -219,8 +232,27 @@ def save(self, instance, commit=True): if field.is_no_change(data[f.name]): continue + if f.name == 'review_status': + if data[f.name]: + self.instance.planning_to_review.add(self.user) + else: + self.instance.planning_to_review.remove(self.user) + continue + setattr(instance, f.name, data[f.name]) if commit: instance.save() return instance + + def review_status_only(self): + review_status_only = True + field_names = set(self.fields.keys()) + field_names.discard({'review_status', 'action'}) + + for field_name in field_names: + data = self.data.get(field_name, '*') + if data != '*': + review_status_only = False + + return review_status_only diff --git a/patchwork/models.py b/patchwork/models.py index 7214ebd8..24345239 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -582,7 +582,7 @@ def save(self, *args, **kwargs): self.refresh_tag_counts() - def is_editable(self, user): + def is_editable(self, user, declare_interest_only=False): if not user.is_authenticated: return False @@ -593,7 +593,8 @@ def is_editable(self, user): if self.project.is_editable(user): self._edited_by = user return True - return False + + return declare_interest_only @staticmethod def filter_unique_checks(checks): diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py index 704ab815..72c6251e 100644 --- a/patchwork/views/__init__.py +++ b/patchwork/views/__init__.py @@ -240,7 +240,9 @@ def generic_list( if data and data.get('form', '') == 'patchlistform': data_tmp = data - properties_form = MultiplePatchForm(project, data=data_tmp) + properties_form = MultiplePatchForm( + project, data=data_tmp, user=request.user + ) if request.method == 'POST' and data.get('form') == 'patchlistform': action = data.get('action', '').lower() @@ -340,7 +342,7 @@ def process_multiplepatch_form(request, form, action, patches, context): changed_patches = 0 for patch in patches: - if not patch.is_editable(request.user): + if not patch.is_editable(request.user, form.review_status_only()): errors.append( "You don't have permissions to edit patch '%s'" % patch.name ) From 909fb550210df6ec58dd14a28f40a91f6b15e8e4 Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Fri, 5 Apr 2024 12:35:06 -0300 Subject: [PATCH 7/8] serializers: Optimize query performance on PatchDetailSerializer for 'patchinterest' Signed-off-by: andrepapoti --- patchwork/api/patch.py | 1 + patchwork/tests/api/test_patch.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index f32a2f16..28d3886d 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -410,6 +410,7 @@ def get_queryset(self): 'project', 'series__project', 'related__patches__project', + 'patchreviewintention_set__user', ) .select_related('state', 'submitter', 'series') .defer('content', 'diff', 'headers') diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py index 4ca88749..8dbe741e 100644 --- a/patchwork/tests/api/test_patch.py +++ b/patchwork/tests/api/test_patch.py @@ -239,7 +239,7 @@ def test_list_bug_335(self): series = create_series() create_patches(5, series=series) - with self.assertNumQueries(5): + with self.assertNumQueries(6): self.client.get(self.api_url()) @utils.store_samples('patch-detail') From 1001da9011f06bd6329e52bcd62427eea01e0ec3 Mon Sep 17 00:00:00 2001 From: andrepapoti Date: Fri, 12 Apr 2024 00:28:43 -0300 Subject: [PATCH 8/8] web: add cron service to update 'has_planned_review' field on patches Signed-off-by: andrepapoti --- .../management/commands/update_reviewers.py | 25 +++++++++++++++++++ tools/docker/Dockerfile | 8 ++++++ tools/docker/crontab | 6 +++++ tools/docker/entrypoint.sh | 1 + 4 files changed, 40 insertions(+) create mode 100644 patchwork/management/commands/update_reviewers.py create mode 100644 tools/docker/crontab diff --git a/patchwork/management/commands/update_reviewers.py b/patchwork/management/commands/update_reviewers.py new file mode 100644 index 00000000..0b4862e0 --- /dev/null +++ b/patchwork/management/commands/update_reviewers.py @@ -0,0 +1,25 @@ +# Patchwork - automated patch tracking system +# Copyright (C) 2015 Jeremy Kerr +# +# SPDX-License-Identifier: GPL-2.0-or-later + +from django.core.management.base import BaseCommand +from patchwork.models import Patch +from patchwork.api.patch import PatchReviewIntentionSerializer + + +class Command(BaseCommand): + help = 'Updates the patch has_planned_review field' + + def handle(self, *args, **kwargs): + for patch in Patch.objects.all(): + has_planned_review = False + for ( + patch_interest + ) in patch.planning_to_review.through.objects.filter(patch=patch): + serializer = PatchReviewIntentionSerializer(patch_interest) + if not serializer.data['is_stale']: + has_planned_review = True + break + patch.has_planned_review = has_planned_review + patch.save() diff --git a/tools/docker/Dockerfile b/tools/docker/Dockerfile index bdae67a2..eaadd071 100644 --- a/tools/docker/Dockerfile +++ b/tools/docker/Dockerfile @@ -31,6 +31,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ sqlite3 \ tzdata \ pkg-config \ + cron \ && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* RUN pip install wheel tox @@ -40,8 +41,15 @@ RUN pip install wheel tox COPY requirements-dev.txt requirements-test.txt /opt/ RUN pip install -r /opt/requirements-dev.txt +COPY tools/docker/crontab /etc/cron.d/patchwork-cron +RUN crontab -u patchwork /etc/cron.d/patchwork-cron +RUN chmod u+s /usr/sbin/cron +RUN printenv >> /etc/environment + COPY tools/docker/entrypoint.sh /usr/local/bin/entrypoint.sh ENTRYPOINT ["/usr/local/bin/entrypoint.sh"] CMD ["python3", "manage.py", "runserver", "0.0.0.0:8000"] USER patchwork WORKDIR /home/patchwork/patchwork + +COPY --chown=patchwork:patchwork . . diff --git a/tools/docker/crontab b/tools/docker/crontab new file mode 100644 index 00000000..23a76e83 --- /dev/null +++ b/tools/docker/crontab @@ -0,0 +1,6 @@ +DATABASE_HOST=db +DATABASE_PORT=3306 +DATABASE_NAME=patchwork +DATABASE_USER=patchwork +DATABASE_PASSWORD=password +0 * * * * /opt/pyenv/shims/python patchwork/manage.py update_reviewers > /proc/1/fd/1 2>&1 diff --git a/tools/docker/entrypoint.sh b/tools/docker/entrypoint.sh index c78c0581..fb5147c4 100755 --- a/tools/docker/entrypoint.sh +++ b/tools/docker/entrypoint.sh @@ -102,4 +102,5 @@ if ! python manage.py migrate sessions --check -v0; then python manage.py loaddata default_projects #> /dev/null fi +cron exec "$@"