Migration 'next' close action to transitions

Remove all usage of close/next in the UI (just administrative actions).

Remove all 'next' detection logic.  The UI will provide valid options
to the user to choose between and they can make an informed choice;
especially since for the typical user there will only be a single choice,
to the Future Commitfest or back to the Parked one.  Committers can
also choose to bring any patch into the In Progress or Open/Interlude.

The implicit constraint 'next' applied to the system was that,
practically speaking, as Patches moved they always move to a Commitfest
that was started in the future: thus current_patch_on_commitfest could
just use a descending start date sort to find the primary Commitfest.
The transition action uses the Workflow implementation which allows
for a Patch to re-enter the same Commitfest.  This changes the
entrydate on the POC which is now used to find the current Commitfest.

The Workflow module tries to improve upon the History messages being
generated.  Additional thoughts forthcoming in this area.  Should be
simple enough to retain the status quo on these if desired, especially
since I'm disliking the absence of a cfid column; which is why i added
the relevant POC Commitfest name to the messages.

The new code is a bit chatty in terms of feedback to the user.  Though,
on the flip side, it intentionally doesn't feel the need to verify
that the button just clicked was intended.  That needs to be tidied
up based upon the guidelines I need to review.

Author: polobo

pgcommitfest/commitfest/models.py

@@ -180,7 +180,7 @@ class Patch(models.Model, DiffableModel):
     }
 
     def current_commitfest(self):
-        return self.commitfests.order_by("-startdate").first()
+        return self.commitfests.order_by("-patchoncommitfest__enterdate").first()
 
     def current_patch_on_commitfest(self):
         cf = self.current_commitfest()

pgcommitfest/commitfest/templates/patch_administrative.inc

@@ -21,8 +21,37 @@
      <li role="presentation"><a href="close/reject/" onclick="return verify_reject()">Rejected</a></li>
      <li role="presentation"><a href="close/withdrawn/" onclick="return verify_withdrawn()">Withdrawn</a></li>
      <li role="presentation"><a href="close/feedback/" onclick="return verify_returned()">Returned with feedback</a></li>
-     <li role="presentation"><a href="close/next/?cfid={{cf.id}}" onclick="return verify_next()">Move to next CF</a></li>
      <li role="presentation"><a href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Committed</a></li>
+     <li role="presentation" class="divider"></li>
+     <li role="presentation" class="dropdown-header">Move To Options</li>
+     {%if not cf.isfuture and workflow.future %}
+      <li role="presentation">
+       <a
+        href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.future.id}}">
+       {{workflow.future.name}}</a>
+      </li>
+     {%endif%}
+     {%if not cf.isopen and workflow.open %}
+      <li role="presentation">
+       <a
+        href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.open.id}}">
+       {{workflow.open.name}}</a>
+      </li>
+     {%endif%}
+     {%if not cf.isinprogress and workflow.progress %}
+      <li role="presentation">
+       <a
+        href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.progress.id}}">
+       {{workflow.progress.name}}</a>
+      </li>
+     {%endif%}
+     {%if not cf.isparked and workflow.parked %}
+      <li role="presentation">
+       <a
+        href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.parked.id}}">
+       {{workflow.parked.name}}</a>
+      </li>
+     {%endif%}
     </ul>
    </div>
 

pgcommitfest/commitfest/templates/patch_workflow.inc

@@ -27,12 +27,42 @@
      <a class="btn btn-default" href="close/reject/" onclick="return verify_reject()">Rejected</a>
      <a class="btn btn-default" href="close/withdrawn/" onclick="return verify_withdrawn()">Withdrawn</a>
      <a class="btn btn-default" href="close/feedback/" onclick="return verify_returned()">Returned with feedback</a>
-     <a class="btn btn-default" href="close/next/?cfid={{cf.id}}" onclick="return verify_next()">Move to next CF</a>
      <a class="btn btn-default" href="close/committed/" onclick="return flagCommitted({%if patch.committer%}'{{patch.committer}}'{%elif is_committer%}'{{user.username}}'{%else%}null{%endif%})">Committed</a>
   </td>
   <!-- Move -->
   <td>
-   /transition Actions Will Go Here (Deprecate Next)
+   {%if not cf.isfuture and workflow.future %}
+    {%if True %}
+     <a class="btn btn-default"
+      href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.future.id}}">
+     {{workflow.future.name}}</a>
+    {%endif%}
+   {%endif%}
+
+   {%if not cf.isopen and workflow.open %}
+    {%if is_committer %}
+     <a class="btn btn-default"
+      href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.open.id}}">
+     {{workflow.open.name}}</a>
+    {%endif%}
+   {%endif%}
+
+   {%if not cf.isinprogress and workflow.progress %}
+    {%if is_committer %}
+     <a class="btn btn-default"
+      href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.progress.id}}">
+     {{workflow.progress.name}}</a>
+    {%endif%}
+   {%endif%}
+
+   {%if not cf.isparked and workflow.parked %}
+    {%if is_author %}
+     <a class="btn btn-default"
+      href="transition/?fromcfid={{poc.commitfest.id}}&tocfid={{workflow.parked.id}}">
+     {{workflow.parked.name}}</a>
+    {%endif%}
+   {%endif%}
+
   </td>
   </tr>
   <tr>

pgcommitfest/commitfest/views.py

@@ -725,6 +725,12 @@ def patch(request, patchid):
                 {"title": cf.title, "href": "/%s/" % cf.pk},
             ],
             "userprofile": getattr(request.user, "userprofile", UserProfile()),
+            "workflow": {
+                "open": Workflow.open_cf(),
+                "future": Workflow.future_cf(),
+                "progress": Workflow.inprogress_cf(),
+                "parked": Workflow.parked_cf(),
+            }
         },
     )
 
@@ -1016,29 +1022,7 @@ def status(request, patchid, status):
 @transaction.atomic
 def close(request, patchid, status):
     patch = get_object_or_404(Patch.objects.select_related(), pk=patchid)
-    cf = patch.current_commitfest()
-
-    try:
-        request_cfid = int(request.GET.get("cfid", ""))
-    except ValueError:
-        # int() failed, ignore
-        request_cfid = None
-
-    if request_cfid is not None and request_cfid != cf.id:
-        # The cfid parameter is only added to the /next/ link. That's the only
-        # close operation where two people pressing the button at the same time
-        # can have unintended effects.
-        messages.error(
-            request,
-            "The patch was moved to a new commitfest by someone else. Please double check if you still want to retry this operation.",
-        )
-        return HttpResponseRedirect("/%s/%s/" % (cf.id, patch.id))
-
-    poc = get_object_or_404(
-        PatchOnCommitFest.objects.select_related(),
-        commitfest__id=cf.id,
-        patch__id=patchid,
-    )
+    poc = patch.current_patch_on_commitfest()
 
     poc.leavedate = datetime.now()
 
@@ -1051,86 +1035,6 @@ def close(request, patchid, status):
         poc.status = PatchOnCommitFest.STATUS_WITHDRAWN
     elif status == "feedback":
         poc.status = PatchOnCommitFest.STATUS_RETURNED
-    elif status == "next":
-        # Only some patch statuses can actually be moved.
-        if poc.status in (
-            PatchOnCommitFest.STATUS_COMMITTED,
-            PatchOnCommitFest.STATUS_NEXT,
-            PatchOnCommitFest.STATUS_RETURNED,
-            PatchOnCommitFest.STATUS_REJECTED,
-        ):
-            # Can't be moved!
-            messages.error(
-                request,
-                "A patch in status {0} cannot be moved to next commitfest.".format(
-                    poc.statusstring
-                ),
-            )
-            return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id))
-        elif poc.status in (
-            PatchOnCommitFest.STATUS_REVIEW,
-            PatchOnCommitFest.STATUS_AUTHOR,
-            PatchOnCommitFest.STATUS_COMMITTER,
-        ):
-            # This one can be moved
-            pass
-        else:
-            messages.error(request, "Invalid existing patch status")
-
-        oldstatus = poc.status
-
-        poc.status = PatchOnCommitFest.STATUS_NEXT
-        # Figure out the commitfest to actually put it on
-        newcf = CommitFest.objects.filter(status=CommitFest.STATUS_OPEN)
-        if len(newcf) == 0:
-            # Ok, there is no open CF at all. Let's see if there is a
-            # future one.
-            newcf = CommitFest.objects.filter(status=CommitFest.STATUS_FUTURE)
-            if len(newcf) == 0:
-                messages.error(request, "No open and no future commitfest exists!")
-                return HttpResponseRedirect(
-                    "/%s/%s/" % (poc.commitfest.id, poc.patch.id)
-                )
-            elif len(newcf) != 1:
-                messages.error(
-                    request, "No open and multiple future commitfests exist!"
-                )
-                return HttpResponseRedirect(
-                    "/%s/%s/" % (poc.commitfest.id, poc.patch.id)
-                )
-        elif len(newcf) != 1:
-            messages.error(request, "Multiple open commitfests exists!")
-            return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id))
-        elif newcf[0] == poc.commitfest:
-            # The current open CF is the same one that we are already on.
-            # In this case, try to see if there is a future CF we can
-            # move it to.
-            newcf = CommitFest.objects.filter(status=CommitFest.STATUS_FUTURE)
-            if len(newcf) == 0:
-                messages.error(
-                    request,
-                    "Cannot move patch to the same commitfest, and no future commitfests exist!",
-                )
-                return HttpResponseRedirect(
-                    "/%s/%s/" % (poc.commitfest.id, poc.patch.id)
-                )
-            elif len(newcf) != 1:
-                messages.error(
-                    request,
-                    "Cannot move patch to the same commitfest, and multiple future commitfests exist!",
-                )
-                return HttpResponseRedirect(
-                    "/%s/%s/" % (poc.commitfest.id, poc.patch.id)
-                )
-        # Create a mapping to the new commitfest that we are bouncing
-        # this patch to.
-        newpoc = PatchOnCommitFest(
-            patch=poc.patch,
-            commitfest=newcf[0],
-            status=oldstatus,
-            enterdate=datetime.now(),
-        )
-        newpoc.save()
     elif status == "committed":
         committer = get_object_or_404(Committer, user__username=request.GET["c"])
         if committer != poc.patch.committer:
@@ -1160,6 +1064,37 @@ def close(request, patchid, status):
     return HttpResponseRedirect("/%s/%s/" % (poc.commitfest.id, poc.patch.id))
 
 
+@login_required
+@transaction.atomic
+def transition(request, patchid):
+    from_cf = Workflow.getCommitfest(request.GET.get("fromcfid", None))
+
+    if from_cf is None:
+        messages.error(request, "Unknown from commitfest id {}".format(request.GET.get("fromcfid", None)))
+        return HttpResponseRedirect("/patch/%s/" % (patchid))
+
+    cur_poc = get_object_or_404(Patch.objects.select_related(), pk=patchid).current_patch_on_commitfest()
+
+    if from_cf != cur_poc.commitfest:
+        messages.error(request, "Transition aborted, Redirect performed.  Commitfest for patch already changed.")
+        return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id))
+
+    target_cf = Workflow.getCommitfest(request.GET.get("tocfid", None))
+
+    if target_cf is None:
+        messages.error(request, "Unknown destination commitfest id {}".format(request.GET.get("tocfid", None)))
+        return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id))
+
+    try:
+        new_poc = Workflow.transitionPatch(cur_poc, target_cf, request.user)
+        messages.info(request, "Transitioned patch to commitfest %s" % new_poc.commitfest.name)
+    except Exception as e:
+        messages.error(request, "Failed to transition patch: {}".format(e))
+        return HttpResponseRedirect("/patch/%s/" % (cur_poc.patch.id))
+
+    return HttpResponseRedirect("/patch/%s/" % (new_poc.patch.id))
+
+
 @login_required
 @transaction.atomic
 def reviewer(request, patchid, status):

pgcommitfest/urls.py

@@ -28,8 +28,9 @@
     re_path(r"^(\d+)/new/$", views.newpatch),
     re_path(r"^patch/(\d+)/status/(review|author|committer)/$", views.status),
     re_path(
-        r"^patch/(\d+)/close/(reject|withdrawn|feedback|committed|next)/$", views.close
+        r"^patch/(\d+)/close/(reject|withdrawn|feedback|committed)/$", views.close
     ),
+    re_path(r"^patch/(\d+)/transition/$", views.transition),
     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),