\ No newline at end of file
+
diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py
index 2c21cd3d..431e84f3 100644
--- a/pgcommitfest/commitfest/views.py
+++ b/pgcommitfest/commitfest/views.py
@@ -14,12 +14,13 @@
from email.mime.text import MIMEText
from email.utils import formatdate, make_msgid
import json
+import hmac
import urllib
from pgcommitfest.mailqueue.util import send_mail, send_simple_mail
from pgcommitfest.userprofile.util import UserWrapper
-from .models import CommitFest, Patch, PatchOnCommitFest, PatchHistory, Committer
+from .models import CommitFest, Patch, PatchOnCommitFest, PatchHistory, Committer, CfbotBranch, CfbotTask
from .models import MailThread
from .forms import PatchForm, NewPatchForm, CommentForm, CommitFestFilterForm
from .forms import BulkEmailForm
@@ -186,6 +187,10 @@ def commitfest(request, cfid):
orderby_str = 'lastmail, created'
elif sortkey == 3:
orderby_str = 'num_cfs DESC, modified, created'
+ elif sortkey == 4:
+ orderby_str = 'p.id'
+ elif sortkey == 5:
+ orderby_str = 'p.name, created'
else:
orderby_str = 'p.id'
sortkey = 0
@@ -213,7 +218,24 @@ def commitfest(request, cfid):
(poc.status=ANY(%(openstatuses)s)) AS is_open,
(SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_authors cpa ON cpa.user_id=auth_user.id WHERE cpa.patch_id=p.id) AS author_names,
(SELECT string_agg(first_name || ' ' || last_name || ' (' || username || ')', ', ') FROM auth_user INNER JOIN commitfest_patch_reviewers cpr ON cpr.user_id=auth_user.id WHERE cpr.patch_id=p.id) AS reviewer_names,
-(SELECT count(1) FROM commitfest_patchoncommitfest pcf WHERE pcf.patch_id=p.id) AS num_cfs
+(SELECT count(1) FROM commitfest_patchoncommitfest pcf WHERE pcf.patch_id=p.id) AS num_cfs,
+(
+ SELECT row_to_json(t) as cfbot_results
+ from (
+ SELECT
+ count(*) FILTER (WHERE task.status = 'COMPLETED') as completed,
+ count(*) FILTER (WHERE task.status in ('CREATED', 'SCHEDULED', 'EXECUTING')) running,
+ count(*) FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) failed,
+ count(*) total,
+ string_agg(task.task_name, ', ') FILTER (WHERE task.status in ('ABORTED', 'ERRORED', 'FAILED')) as failed_task_names,
+ branch.commit_id IS NULL as needs_rebase,
+ branch.apply_url
+ FROM commitfest_cfbotbranch branch
+ LEFT JOIN commitfest_cfbottask task ON task.branch_id = branch.branch_id
+ WHERE branch.patch_id=p.id
+ GROUP BY branch.patch_id
+ ) t
+)
FROM commitfest_patch p
INNER JOIN commitfest_patchoncommitfest poc ON poc.patch_id=p.id
INNER JOIN commitfest_topic t ON t.id=p.topic_id
@@ -298,19 +320,23 @@ def global_search(request):
})
-def patch_redirect(request, patchid):
- last_commitfest = PatchOnCommitFest.objects.select_related('commitfest').filter(patch_id=patchid).order_by('-commitfest__startdate').first()
- if not last_commitfest:
- raise Http404("Patch not found")
- return HttpResponseRedirect(f'/{last_commitfest.commitfest_id}/{patchid}/')
+def patch_legacy_redirect(request, cfid, patchid):
+ # Previously we would include the commitfest id in the URL. This is no
+ # longer the case.
+ return HttpResponseRedirect(f'/patch/{patchid}/')
-def patch(request, cfid, patchid):
- cf = get_object_or_404(CommitFest, pk=cfid)
- patch = get_object_or_404(Patch.objects.select_related(), pk=patchid, commitfests=cf)
- patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate')
+def patch(request, patchid):
+ patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+ cf = patch.current_commitfest()
+
+ patch_commitfests = PatchOnCommitFest.objects.select_related('commitfest').filter(patch=patch).order_by('-commitfest__startdate').all()
+
committers = Committer.objects.filter(active=True).order_by('user__last_name', 'user__first_name')
+ cfbot_branch = getattr(patch, 'cfbot_branch', None)
+ cfbot_tasks = patch.cfbot_tasks.order_by('position') if cfbot_branch else []
+
# XXX: this creates a session, so find a smarter way. Probably handle
# it in the callback and just ask the user then?
if request.user.is_authenticated:
@@ -333,6 +359,8 @@ def patch(request, cfid, patchid):
'cf': cf,
'patch': patch,
'patch_commitfests': patch_commitfests,
+ 'cfbot_branch': cfbot_branch,
+ 'cfbot_tasks': cfbot_tasks,
'is_committer': is_committer,
'is_this_committer': is_this_committer,
'is_reviewer': is_reviewer,
@@ -346,9 +374,9 @@ def patch(request, cfid, patchid):
@login_required
@transaction.atomic
-def patchform(request, cfid, patchid):
- cf = get_object_or_404(CommitFest, pk=cfid)
- patch = get_object_or_404(Patch, pk=patchid, commitfests=cf)
+def patchform(request, patchid):
+ patch = get_object_or_404(Patch, pk=patchid)
+ cf = patch.current_commitfest()
prevreviewers = list(patch.reviewers.all())
prevauthors = list(patch.authors.all())
@@ -438,9 +466,9 @@ def _review_status_string(reviewstatus):
@login_required
@transaction.atomic
-def comment(request, cfid, patchid, what):
- cf = get_object_or_404(CommitFest, pk=cfid)
+def comment(request, patchid, what):
patch = get_object_or_404(Patch, pk=patchid)
+ cf = patch.current_commitfest()
poc = get_object_or_404(PatchOnCommitFest, patch=patch, commitfest=cf)
is_review = (what == 'review')
@@ -535,8 +563,10 @@ def comment(request, cfid, patchid, what):
@login_required
@transaction.atomic
-def status(request, cfid, patchid, status):
- poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cfid, patch__id=patchid)
+def status(request, patchid, status):
+ patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+ cf = patch.current_commitfest()
+ poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cf.id, patch__id=patchid)
if poc.is_closed:
# We allow modification of patches in closed CFs *only* if it's the
@@ -570,8 +600,10 @@ def status(request, cfid, patchid, status):
@login_required
@transaction.atomic
-def close(request, cfid, patchid, status):
- poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cfid, patch__id=patchid)
+def close(request, patchid, status):
+ patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
+ cf = patch.current_commitfest()
+ poc = get_object_or_404(PatchOnCommitFest.objects.select_related(), commitfest__id=cf.id, patch__id=patchid)
if poc.is_closed:
# We allow modification of patches in closed CFs *only* if it's the
@@ -668,8 +700,7 @@ def close(request, cfid, patchid, status):
@login_required
@transaction.atomic
-def reviewer(request, cfid, patchid, status):
- get_object_or_404(CommitFest, pk=cfid)
+def reviewer(request, patchid, status):
patch = get_object_or_404(Patch, pk=patchid)
is_reviewer = request.user in patch.reviewers.all()
@@ -688,7 +719,6 @@ def reviewer(request, cfid, patchid, status):
@login_required
@transaction.atomic
def committer(request, cfid, patchid, status):
- get_object_or_404(CommitFest, pk=cfid)
patch = get_object_or_404(Patch, pk=patchid)
committer = list(Committer.objects.filter(user=request.user, active=True))
@@ -713,8 +743,7 @@ def committer(request, cfid, patchid, status):
@login_required
@transaction.atomic
-def subscribe(request, cfid, patchid, sub):
- get_object_or_404(CommitFest, pk=cfid)
+def subscribe(request, patchid, sub):
patch = get_object_or_404(Patch, pk=patchid)
if sub == 'un':
@@ -727,6 +756,12 @@ def subscribe(request, cfid, patchid, sub):
return HttpResponseRedirect("../")
+def send_patch_email(request, patchid):
+ patch = get_object_or_404(Patch, pk=patchid)
+ cf = patch.current_commitfest()
+ return send_email(request, cf.id)
+
+
@login_required
@transaction.atomic
def send_email(request, cfid):
@@ -788,6 +823,137 @@ def _user_and_mail(u):
})
+@transaction.atomic
+def cfbot_ingest(message):
+ """Ingest a single message status update message receive from cfbot. It
+ should be a Python dictionary, decoded from JSON already."""
+
+ cursor = connection.cursor()
+
+ branch_status = message["branch_status"]
+ patch_id = branch_status["submission_id"]
+ branch_id = branch_status["branch_id"]
+
+ try:
+ patch = Patch.objects.get(pk=patch_id)
+ except Patch.DoesNotExist:
+ # If the patch doesn't exist, there's nothing to do. This should never
+ # happen in production, but on the test system it's possible because
+ # not it doesn't contain the newest patches that the CFBot knows about.
+ return
+
+ # Every message should have a branch_status, which we will INSERT
+ # or UPDATE. We do this first, because cfbot_task refers to it.
+ # Due to the way messages are sent/queued by cfbot it's possible that it
+ # sends the messages out-of-order. To handle this we we only update in two
+ # cases:
+ # 1. The created time of the branch is newer than the one in our database:
+ # This is a newer branch
+ # 2. If it's the same branch that we already have, but the modified time is
+ # newer: This is a status update for the current branch that we received
+ # in-order.
+ cursor.execute("""INSERT INTO commitfest_cfbotbranch (patch_id, branch_id,
+ branch_name, commit_id,
+ apply_url, status,
+ created, modified)
+ VALUES (%s, %s, %s, %s, %s, %s, %s, %s)
+ ON CONFLICT (patch_id) DO UPDATE
+ SET status = EXCLUDED.status,
+ modified = EXCLUDED.modified,
+ branch_id = EXCLUDED.branch_id,
+ branch_name = EXCLUDED.branch_name,
+ commit_id = EXCLUDED.commit_id,
+ apply_url = EXCLUDED.apply_url,
+ created = EXCLUDED.created
+ WHERE commitfest_cfbotbranch.created < EXCLUDED.created
+ OR (commitfest_cfbotbranch.branch_id = EXCLUDED.branch_id
+ AND commitfest_cfbotbranch.modified < EXCLUDED.modified)
+ """,
+ (
+ patch_id,
+ branch_id,
+ branch_status["branch_name"],
+ branch_status["commit_id"],
+ branch_status["apply_url"],
+ branch_status["status"],
+ branch_status["created"],
+ branch_status["modified"])
+ )
+
+ # Now we check what we have in our database. If that contains a different
+ # branch_id than what we just tried to insert, then apparently this is a
+ # status update for an old branch and we don't care about any of the
+ # contents of this message.
+ branch_in_db = CfbotBranch.objects.get(pk=patch_id)
+ if branch_in_db.branch_id != branch_id:
+ return
+
+ # Most messages have a task_status. It might be missing in rare cases, like
+ # when cfbot decides that a whole branch has timed out. We INSERT or
+ # UPDATE.
+ if "task_status" in message:
+ task_status = message["task_status"]
+ cursor.execute("""INSERT INTO commitfest_cfbottask (task_id, task_name, patch_id, branch_id,
+ position, status,
+ created, modified)
+ VALUES (%s, %s, %s, %s, %s, %s, %s, %s)
+ ON CONFLICT (task_id) DO UPDATE
+ SET status = EXCLUDED.status,
+ modified = EXCLUDED.modified
+ WHERE commitfest_cfbottask.modified < EXCLUDED.modified""",
+ (
+ task_status["task_id"],
+ task_status["task_name"],
+ patch_id,
+ branch_id,
+ task_status["position"],
+ task_status["status"],
+ task_status["created"],
+ task_status["modified"])
+ )
+
+ # Remove any old tasks that are not related to this branch. These should
+ # only be left over when we just updated the branch_id. Knowing if we just
+ # updated the branch_id was is not trivial though, because INSERT ON
+ # CONFLICT does not allow us to easily return the old value of the row.
+ # So instead we always delete all tasks that are not related to this
+ # branch. This is fine, because doing so is very cheap in the no-op case
+ # because we have an index on patch_id and there's only a handful of tasks
+ # per patch.
+ cursor.execute("DELETE FROM commitfest_cfbottask WHERE patch_id=%s AND branch_id != %s", (patch_id, branch_id))
+
+ # We change the needs_rebase field using a separate UPDATE because this way
+ # we can find out what the previous state of the field was (sadly INSERT ON
+ # CONFLICT does not allow us to return that). We need to know the previous
+ # state so we can skip sending notifications if the needs_rebase status did
+ # not change.
+ needs_rebase = branch_status['commit_id'] is None
+ if bool(branch_in_db.needs_rebase_since) is not needs_rebase:
+ if needs_rebase:
+ branch_in_db.needs_rebase_since = datetime.now()
+ else:
+ branch_in_db.needs_rebase_since = None
+ branch_in_db.save()
+
+ if needs_rebase:
+ PatchHistory(patch=patch, by=None, by_cfbot=True, what='Patch needs rebase').save_and_notify(authors_only=True)
+ else:
+ PatchHistory(patch=patch, by=None, by_cfbot=True, what='Patch does not need rebase anymore').save_and_notify(authors_only=True)
+
+
+@csrf_exempt
+def cfbot_notify(request):
+ if request.method != 'POST':
+ return HttpResponseForbidden("Invalid method")
+
+ j = json.loads(request.body)
+ if not hmac.compare_digest(j['shared_secret'], settings.CFBOT_SECRET):
+ return HttpResponseForbidden("Invalid API key")
+
+ cfbot_ingest(j)
+ return HttpResponse(status=200)
+
+
@csrf_exempt
def thread_notify(request):
if request.method != 'POST':
diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py
index 53fce1c1..c963a298 100644
--- a/pgcommitfest/urls.py
+++ b/pgcommitfest/urls.py
@@ -19,23 +19,24 @@
re_path(r'^(\d+)/$', views.commitfest),
re_path(r'^(open|inprogress|current)/(.*)$', views.redir),
re_path(r'^(?P\d+)/activity(?P\.rss)?/$', views.activity),
- re_path(r'^patch/(\d+)/$', views.patch_redirect),
- re_path(r'^(\d+)/(\d+)/$', views.patch),
- re_path(r'^(\d+)/(\d+)/edit/$', views.patchform),
+ re_path(r'^(\d+)/(\d+)/$', views.patch_legacy_redirect),
+ re_path(r'^patch/(\d+)/$', views.patch),
+ re_path(r'^patch/(\d+)/edit/$', views.patchform),
re_path(r'^(\d+)/new/$', views.newpatch),
- re_path(r'^(\d+)/(\d+)/status/(review|author|committer)/$', views.status),
- re_path(r'^(\d+)/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$', views.close),
- re_path(r'^(\d+)/(\d+)/reviewer/(become|remove)/$', views.reviewer),
- re_path(r'^(\d+)/(\d+)/committer/(become|remove)/$', views.committer),
- re_path(r'^(\d+)/(\d+)/(un)?subscribe/$', views.subscribe),
- re_path(r'^(\d+)/(\d+)/(comment|review)/', views.comment),
+ re_path(r'^patch/(\d+)/status/(review|author|committer)/$', views.status),
+ re_path(r'^patch/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$', views.close),
+ re_path(r'^patch/(\d+)/reviewer/(become|remove)/$', views.reviewer),
+ re_path(r'^patch/(\d+)/committer/(become|remove)/$', views.committer),
+ re_path(r'^patch/(\d+)/(un)?subscribe/$', views.subscribe),
+ re_path(r'^patch/(\d+)/(comment|review)/', views.comment),
re_path(r'^(\d+)/send_email/$', views.send_email),
- re_path(r'^(\d+)/\d+/send_email/$', views.send_email),
+ re_path(r'^patch/(\d+)/send_email/$', views.send_patch_email),
re_path(r'^(\d+)/reports/authorstats/$', reports.authorstats),
re_path(r'^search/$', views.global_search),
re_path(r'^ajax/(\w+)/$', ajax.main),
re_path(r'^lookups/user/$', lookups.userlookup),
re_path(r'^thread_notify/$', views.thread_notify),
+ re_path(r'^cfbot_notify/$', views.cfbot_notify),
# Auth system integration
re_path(r'^(?:account/)?login/?$', pgcommitfest.auth.login),