[Patchew-devel] [PATCH 6/6] add Review model
Fam Zheng
famz at redhat.com
Fri Aug 24 06:25:20 UTC 2018
On Thu, 08/23 22:00, Paolo Bonzini wrote:
> This model has a simple many-to-many relation with series, that lets
> logged-in users mark series as accepted or rejected, and use the status
> in a query.
>
> Three new search operators are added: ack (with synonyms accept and
> accepted), nack (with synonyms reject and rejected), review (with
> synonyms reviewed).
>
> Sample useful queries include the following:
>
> - "-is:merged ack:me" (what should I merge)
> - "to:pbonzini at redhat.com -is:merged -reviewed:me" (what should I review)
> - "nvme -is:merged -reviewed:fam" (what did Fam overlook :))
>
> Signed-off-by: Paolo Bonzini <pbonzini at redhat.com>
> ---
> api/migrations/0034_auto_20180822_1106.py | 32 ++++++++++
> api/models.py | 7 +++
> api/search.py | 28 +++++++++
> mods/maintainer.py | 72 +++++++++++++++++++++--
> static/css/base.css | 2 +-
> 5 files changed, 136 insertions(+), 5 deletions(-)
> create mode 100644 api/migrations/0034_auto_20180822_1106.py
>
> diff --git a/api/migrations/0034_auto_20180822_1106.py b/api/migrations/0034_auto_20180822_1106.py
> new file mode 100644
> index 0000000..d830d47
> --- /dev/null
> +++ b/api/migrations/0034_auto_20180822_1106.py
> @@ -0,0 +1,32 @@
> +# -*- coding: utf-8 -*-
> +# Generated by Django 1.11.13 on 2018-08-22 11:06
> +from __future__ import unicode_literals
> +
> +from django.conf import settings
> +from django.db import migrations, models
> +import django.db.models.deletion
> +
> +
> +class Migration(migrations.Migration):
> +
> + dependencies = [
> + migrations.swappable_dependency(settings.AUTH_USER_MODEL),
> + ('api', '0033_auto_20180803_0809'),
> + ]
> +
> + operations = [
> + migrations.CreateModel(
> + name='Review',
> + fields=[
> + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
> + ('accept', models.BooleanField()),
> + ('message', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='api.Message')),
> + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
> + ],
> + ),
> + migrations.AddField(
> + model_name='message',
> + name='reviews',
> + field=models.ManyToManyField(blank=True, through='api.Review', to=settings.AUTH_USER_MODEL),
> + ),
> + ]
> diff --git a/api/models.py b/api/models.py
> index eea924f..545013e 100644
> --- a/api/models.py
> +++ b/api/models.py
> @@ -419,6 +419,11 @@ class MessageManager(models.Manager):
> def HeaderFieldModel(**args):
> return models.CharField(max_length=4096, **args)
>
> +class Review(models.Model):
> + user = models.ForeignKey(User)
> + message = models.ForeignKey('Message')
> + accept = models.BooleanField()
> +
> class Message(models.Model):
> """ Patch email message """
>
> @@ -455,6 +460,8 @@ class Message(models.Model):
> # number of patches we've got if is_series_head
> num_patches = models.IntegerField(null=False, default=-1, blank=True)
>
> + reviews = models.ManyToManyField(User, blank=True, through=Review)
> +
> objects = MessageManager()
>
> def save_mbox(self, mbox_blob):
> diff --git a/api/search.py b/api/search.py
> index aa77f22..bcac3ac 100644
> --- a/api/search.py
> +++ b/api/search.py
> @@ -104,6 +104,18 @@ Example:
>
> ---
>
> +### Search by review state
> +
> +Syntax:
> +
> + - accept:USERNAME or ack:USERNAME - the series was marked as accepted by the user
> + - reject:USERNAME or nack:USERNAME - the series was marked as rejected by the user
> + - review:USERNAME - the series was marked as accepted or rejected by the user
> +
> +USERNAME can be "me" to identify the current user
> +
> +---
> +
> ### Reverse condition
>
> - Syntax: !TERM
> @@ -202,6 +214,12 @@ Search text keyword in the email message. Example:
> def _make_filter_result(self, term, **kwargs):
> return Q(results__name=term, **kwargs) | Q(results__name__startswith=term+'.', **kwargs)
>
> + def _make_filter_review(self, username, user, **kwargs):
> + if username == "me":
> + return Q(review__user=user, **kwargs)
> + else:
> + return Q(review__user__username=username, **kwargs)
> +
> def _make_filter(self, term, user):
> if term.startswith("age:"):
> cond = term[term.find(":") + 1:]
> @@ -240,6 +258,16 @@ Search text keyword in the email message. Example:
> return self._make_filter_result(term[8:], results__status=Result.PENDING)
> elif term.startswith("running:"):
> return self._make_filter_result(term[8:], results__status=Result.RUNNING)
> + return self._make_filter_result(cond[8:], results__status=Result.RUNNING)
> + elif term.startswith("ack:") or term.startswith("accept:") or term.startswith("accepted:"):
> + username = term[term.find(":") + 1:]
> + return self._make_filter_review(username, user, review__accept=True)
> + elif term.startswith("nack:") or term.startswith("reject:") or term.startswith("rejected:"):
> + username = term[term.find(":") + 1:]
> + return self._make_filter_review(username, user, review__accept=False)
> + elif term.startswith("review:") or term.startswith("reviewed:"):
> + username = term[term.find(":") + 1:]
> + return self._make_filter_review(username, user)
> elif term.startswith("project:"):
> cond = term[term.find(":") + 1:]
> self._projects.add(cond)
> diff --git a/mods/maintainer.py b/mods/maintainer.py
> index e25d8e2..7cb23d2 100644
> --- a/mods/maintainer.py
> +++ b/mods/maintainer.py
> @@ -12,13 +12,27 @@ from django.conf.urls import url
> from django.http import Http404, HttpResponseRedirect
> from django.urls import reverse
> from mod import PatchewModule
> -from api.models import Message
> +from api.models import Message, Review
>
> class MaintainerModule(PatchewModule):
> """ Project maintainer related tasks """
>
> name = "maintainer"
>
> + def _update_review_state(self, request, message_id, accept):
> + msg = Message.objects.find_series(message_id)
> + r = Review.objects.filter(user=request.user, message=msg)
Review.objects.get_or_create().
> + r = r[0] if r else Review(user=request.user, message=msg)
> + r.accept = accept
> + r.save()
> + return HttpResponseRedirect(request.META.get('HTTP_REFERER'))
> +
> + def _delete_review(self, request, message_id):
> + msg = Message.objects.find_series(message_id)
> + r = Review.objects.filter(user=request.user, message=msg)
> + r.delete()
> + return HttpResponseRedirect(request.META.get('HTTP_REFERER'))
> +
> def _update_merge_state(self, request, message_id, is_merged):
> s = Message.objects.find_series(message_id)
> if not s:
> @@ -35,6 +49,15 @@ class MaintainerModule(PatchewModule):
> def www_view_clear_merged(self, request, message_id):
> return self._update_merge_state(request, message_id, False)
>
> + def www_view_mark_accepted(self, request, message_id):
> + return self._update_review_state(request, message_id, True)
> +
> + def www_view_mark_rejected(self, request, message_id):
> + return self._update_review_state(request, message_id, False)
> +
> + def www_view_clear_reviewed(self, request, message_id):
> + return self._delete_review(request, message_id)
Don't these handlers want to check request.user.is_authenticated?
> +
> def www_url_hook(self, urlpatterns):
> urlpatterns.append(url(r"^mark-as-merged/(?P<message_id>.*)/",
> self.www_view_mark_as_merged,
> @@ -42,18 +65,59 @@ class MaintainerModule(PatchewModule):
> urlpatterns.append(url(r"^clear-merged/(?P<message_id>.*)/",
> self.www_view_clear_merged,
> name="clear-merged"))
> + urlpatterns.append(url(r"^mark-as-accepted/(?P<message_id>.*)/",
> + self.www_view_mark_accepted,
> + name="mark-as-accepted"))
> + urlpatterns.append(url(r"^mark-as-rejected/(?P<message_id>.*)/",
> + self.www_view_mark_rejected,
> + name="mark-as-rejected"))
> + urlpatterns.append(url(r"^clear-reviewed/(?P<message_id>.*)/",
> + self.www_view_clear_reviewed,
> + name="clear-reviewed"))
>
> def prepare_message_hook(self, request, message, detailed):
> - if not detailed:
> + if not detailed or not request.user.is_authenticated:
> return
> - if message.is_series_head and request.user.is_authenticated:
> + if message.is_series_head:
> if message.is_merged:
> message.extra_ops.append({"url": reverse("clear-merged",
> kwargs={"message_id": message.message_id}),
> - "icon": "times",
> + "icon": "eraser",
> "title": "Clear merged state"})
> else:
> message.extra_ops.append({"url": reverse("mark-as-merged",
> kwargs={"message_id": message.message_id}),
> "icon": "check",
> "title": "Mark series as merged"})
> +
> + r = Review.objects.filter(user=request.user, message=message)
r = r.first() to simplify the if conditions a bit?
> + if not r or not r[0].accept:
> + if message.is_series_head:
> + message.extra_ops.append({"url": reverse("mark-as-accepted",
> + kwargs={"message_id": message.message_id}),
> + "icon": "check",
> + "title": "Mark series as accepted"})
> + else:
> + message.extra_status.append({
> + "icon": "fa-check",
> + "html": 'The series is marked for merging'
> + })
> +
> + if not r or r[0].accept:
> + if message.is_series_head:
> + message.extra_ops.append({"url": reverse("mark-as-rejected",
> + kwargs={"message_id": message.message_id}),
> + "icon": "times",
> + "title": "Mark series as rejected"})
> + else:
> + message.extra_status.append({
> + "icon": "fa-times",
> + "html": 'The series is marked as rejected'
> + })
> +
> + print(r)
Please remember to drop this before pushing. :)
> + if r:
> + message.extra_ops.append({"url": reverse("clear-reviewed",
> + kwargs={"message_id": message.message_id}),
> + "icon": "eraser",
> + "title": "Clear review state"})
> diff --git a/static/css/base.css b/static/css/base.css
> index 7fe45e4..9e04f50 100644
> --- a/static/css/base.css
> +++ b/static/css/base.css
> @@ -137,7 +137,7 @@ h1, h2, h3, .h1, .h2, .h3 {
> .status-content > .fa {
> color: #337ab7;
> }
> -.status-content > .fa-warning {
> +.status-content > .fa-warning, .status-content > .fa-times {
> color: #CC0000;
> }
> .status-content > .fa-check {
> --
> 2.17.1
>
> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel
Fam
More information about the Patchew-devel
mailing list