TP2000-1077 Add review tabs to workbasket detail view (#1071)

* Modify review view routes to use wb pk

* Add wb pk kwarg to review view calls

* Update tests

* Fix flaky test

* Remove comma from total_count to fix progress bar

* Make page title conditional

importer/jinja2/eu-importer/notify-success.jinja

@@ -13,7 +13,7 @@
       {"text": "Home", "href": url("home")},
       {"text": "Edit an existing workbasket", "href": url("workbaskets:workbasket-ui-list")},
       {"text": "Workbasket " ~ request.session.workbasket.id ~ " - Review goods", 
-      "href": url("workbaskets:workbasket-ui-review-goods")}, 
+      "href": url("workbaskets:workbasket-ui-review-goods", kwargs={"pk": request.session.workbasket.id})},
       {"text": page_title}
     ]
   }) }}
@@ -30,7 +30,7 @@
       }) }}
 
     <div class="govuk-button-group">
-      <a class="govuk-link" href="{{ url('workbaskets:workbasket-ui-review-goods') }}">
+      <a class="govuk-link" href="{{ url('workbaskets:workbasket-ui-review-goods', kwargs={"pk": request.session.workbasket.id}) }}">
         Back to Review goods list
       </a>
     </div>

measures/views.py

@@ -547,7 +547,12 @@ def done(self, form_list, **kwargs):
             )
         self.session_store.clear()
 
-        return redirect(reverse("workbaskets:workbasket-ui-review-measures"))
+        return redirect(
+            reverse(
+                "workbaskets:workbasket-ui-review-measures",
+                kwargs={"pk": workbasket.pk},
+            ),
+        )
 
 
 @method_decorator(require_current_workbasket, name="dispatch")
@@ -1229,17 +1234,23 @@ def post(self, request):
             # The user has cancelled out of the deletion process.
             return redirect("home")
 
+        workbasket = WorkBasket.current(request)
         object_list = self.get_queryset()
 
         for obj in object_list:
             # make a new version of the object with an update type of delete.
             obj.new_version(
-                workbasket=WorkBasket.current(request),
+                workbasket=workbasket,
                 update_type=UpdateType.DELETE,
             )
         self.session_store.clear()
 
-        return redirect(reverse("workbaskets:workbasket-ui-review-measures"))
+        return redirect(
+            reverse(
+                "workbaskets:workbasket-ui-review-measures",
+                kwargs={"pk": workbasket.pk},
+            ),
+        )
 
 
 class MeasureSelectionUpdate(MeasureSessionStoreMixin, View):

workbaskets/jinja2/includes/workbaskets/navigation.jinja

@@ -11,7 +11,7 @@
             <a href="{{ url('workbaskets:workbasket-ui-violations') }}" class="govuk-link">View violations ({{request.session.workbasket.error_count}})</a>
         </li>
         <li class="{% if active_link == 'review' %}active{% endif %}">
-            <a href="{{ url('workbaskets:workbasket-ui-review-measures') }}" class="govuk-link">Review ({{request.session.workbasket.measure_count}})</a>
+            <a href="{{ url('workbaskets:workbasket-ui-review-measures', args=[request.session.workbasket.id]) }}" class="govuk-link">Review ({{request.session.workbasket.measure_count}})</a>
         </li>
         <li class="{% if active_link == 'compare' %}active{% endif %}">
             <a href="{{ url('workbaskets:workbasket-ui-compare') }}" class="govuk-link">Worksheet check</a>
@@ -29,6 +29,9 @@
     <li class="{% if active_tab == "changes" %}active{% endif %}">
       <a href="{{ url("workbaskets:workbasket-ui-changes", args=[workbasket.pk]) }}" class="govuk-link">Changes</a>
     </li>
+    <li class="{% if active_tab == "review" %}active{% endif %}">
+      <a href="{{ url("workbaskets:workbasket-ui-review-measures", args=[workbasket.pk]) }}" class="govuk-link">Review</a>
+    </li>
   </ul>
 </nav>
 {% endmacro %}

workbaskets/jinja2/workbaskets/changes.jinja

@@ -35,12 +35,12 @@
 {% endset %}
 
 {% set current_count = page_obj|length + ((page_obj.number - 1) * view.paginate_by) %}
-{% set total_count = "{0:,}".format(page_obj.paginator.count) %}
+{% set total_count = page_obj.paginator.count %}
 
 {% set load_more_pagination %}
   <nav id="workbasket-items" class="pagination" role="navigation" aria-label="Pagination navigation">
     <p class="govuk-body govuk-!-margin-0">
-      You've viewed {{current_count}} out of {{total_count}} items
+      You've viewed {{current_count}} out of {{"{0:,}".format(total_count)}} item{{ total_count|pluralize }}
     </p>
 
     <progress value="{{current_count}}" max="{{total_count}}" role="progressbar"></progress>

workbaskets/jinja2/workbaskets/review.jinja

@@ -1,60 +1,85 @@
 {% extends "layouts/layout.jinja" %}
 {% from "components/table/macro.njk" import govukTable %}
+{% from "includes/workbaskets/navigation.jinja" import create_workbasket_detail_navigation with context %}
 {% from "includes/workbaskets/navigation.jinja" import navigation %}
 {% from "macros/fake_tabs.jinja" import fake_tabs %}
 
-{% set page_title %}Workbasket {{ workbasket.id if workbasket else request.session.workbasket.id }} - Review{% endset %}
+{% if workbasket == session_workbasket %}
+  {% set page_title %}Workbasket {{ workbasket.id }} - Review{% endset %}
+{% else %}
+  {% set page_title %} Workbasket {{ workbasket.id }} - {{ workbasket.status }} {% endset %}
+{% endif %}
 
 {% set links = [
   {
     "text": "Additional codes",
-    "href": url("workbaskets:workbasket-ui-review-additional-codes"),
+    "href": url("workbaskets:workbasket-ui-review-additional-codes", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "additional-codes"
   },
   {
     "text": "Certificates",
-    "href": url("workbaskets:workbasket-ui-review-certificates"),
+    "href": url("workbaskets:workbasket-ui-review-certificates", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "certificates"
   },
   {
     "text": "Commodities",
-    "href": url("workbaskets:workbasket-ui-review-goods"),
+    "href": url("workbaskets:workbasket-ui-review-goods", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "commodities"
   },
   {
     "text": "Footnotes",
-    "href": url("workbaskets:workbasket-ui-review-footnotes"),
+    "href": url("workbaskets:workbasket-ui-review-footnotes", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "footnotes"
   },
   {
     "text": "Geographical areas",
-    "href": url("workbaskets:workbasket-ui-review-geo-areas"),
+    "href": url("workbaskets:workbasket-ui-review-geo-areas", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "geographical-areas"
   },
   {
     "text": "Measures",
-    "href": url("workbaskets:workbasket-ui-review-measures"),
+    "href": url("workbaskets:workbasket-ui-review-measures", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "measures"
   },
   {
     "text": "Quotas",
-    "href": url("workbaskets:workbasket-ui-review-quotas"),
+    "href": url("workbaskets:workbasket-ui-review-quotas", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "quotas"
   },
   {
     "text": "Regulations",
-    "href": url("workbaskets:workbasket-ui-review-regulations"),
+    "href": url("workbaskets:workbasket-ui-review-regulations", kwargs={"pk": workbasket.pk}),
     "selected": selected_tab == "regulations"
   },
   ]
 %}
 
+{% block breadcrumb %}
+  {% if workbasket != session_workbasket %}
+    {{ breadcrumbs(request, [
+        {"text": "Find and view workbaskets", "href": url("workbaskets:workbasket-ui-list-all")},
+        {
+          "text": "Workbasket " ~ workbasket.id ~ "-" ~ workbasket.status,
+          "href": url("workbaskets:workbasket-ui-detail", kwargs={"pk": workbasket.pk}),
+        },
+        {"text": page_title}
+      ])
+    }}
+  {% else %}
+    {{ super() }}
+  {% endif %}
+{% endblock %}
+
 {% block content %}
   <h1 class="govuk-heading-xl govuk-!-margin-bottom-3">
     {{ page_title }}
   </h1>
 
-  {{ navigation(request, "review") }}
+  {% if workbasket == session_workbasket %}
+    {{ navigation(request, "review") }}
+  {% else %}
+    {{ create_workbasket_detail_navigation(active_tab="review") }}
+  {% endif %}
 
   <div id="workbasket-review-tabs">
     {{ fake_tabs(links) }}

workbaskets/jinja2/workbaskets/summary-workbasket.jinja

@@ -50,7 +50,7 @@
     <span class="govuk-!-font-weight-bold">
       Number of changes: {{workbasket.tracked_models.count()}} &nbsp
     </span>
-    <a href={{ url('workbaskets:workbasket-ui-review-measures') }}>Review changes</a>
+    <a href={{ url('workbaskets:workbasket-ui-review-measures', args=[workbasket.pk]) }}>Review changes</a>
     <span class="govuk-!-font-weight-bold" style="float:right">
       Last Run: ({{ "{:%d %b %Y %H:%M}".format(localtime(workbasket.tracked_model_checks.order_by("created_at").last().created_at)) }})
       {{ run_business_rules_form_icon }}

workbaskets/tests/test_views.py

@@ -289,20 +289,21 @@ def test_select_workbasket_with_errored_status(valid_user_client):
 
 
 @pytest.mark.parametrize(
-    "workbasket_tab, expected_url",
+    "workbasket_tab, expected_url, url_kwargs_required",
     [
-        ("view-summary", "workbaskets:current-workbasket"),
-        ("add-edit-items", "workbaskets:edit-workbasket"),
-        ("view-violations", "workbaskets:workbasket-ui-violations"),
-        ("review-measures", "workbaskets:workbasket-ui-review-measures"),
-        ("review-goods", "workbaskets:workbasket-ui-review-goods"),
-        ("", "workbaskets:current-workbasket"),
+        ("view-summary", "workbaskets:current-workbasket", False),
+        ("add-edit-items", "workbaskets:edit-workbasket", False),
+        ("view-violations", "workbaskets:workbasket-ui-violations", False),
+        ("review-measures", "workbaskets:workbasket-ui-review-measures", True),
+        ("review-goods", "workbaskets:workbasket-ui-review-goods", True),
+        ("", "workbaskets:current-workbasket", False),
     ],
 )
 def test_select_workbasket_redirects_to_tab(
     valid_user_client,
     workbasket_tab,
     expected_url,
+    url_kwargs_required,
 ):
     """Test that SelectWorkbasketView redirects to a specific tab on the
     selected workbasket if a tab has been provided."""
@@ -315,7 +316,10 @@ def test_select_workbasket_redirects_to_tab(
         },
     )
     assert response.status_code == 302
-    assert response.url == reverse(expected_url)
+    if url_kwargs_required:
+        assert response.url == reverse(expected_url, kwargs={"pk": workbasket.pk})
+    else:
+        assert response.url == reverse(expected_url)
 
 
 @pytest.mark.parametrize(
@@ -453,9 +457,10 @@ def test_workbasket_list_all_view_search_filters(
 def test_workbasket_review_tabs_without_permission(url, client):
     """Tests that workbasket review tabs return 403 to users without
     view_workbasket permission."""
+    workbasket = factories.WorkBasketFactory.create()
     user = factories.UserFactory.create()
     client.force_login(user)
-    url = reverse(url)
+    url = reverse(url, kwargs={"pk": workbasket.pk})
     response = client.get(url)
     assert response.status_code == 403
 
@@ -511,7 +516,7 @@ def test_workbasket_review_tabs(
     table."""
     with session_workbasket.new_transaction():
         object_factory()
-    url = reverse(url)
+    url = reverse(url, kwargs={"pk": session_workbasket.pk})
     response = valid_user_client.get(url)
     assert response.status_code == 200
 
@@ -533,7 +538,10 @@ def test_workbasket_review_measures(valid_user_client):
     with workbasket.new_transaction() as tx:
         factories.MeasureFactory.create_batch(30, transaction=tx)
 
-    url = reverse("workbaskets:workbasket-ui-review-measures")
+    url = reverse(
+        "workbaskets:workbasket-ui-review-measures",
+        kwargs={"pk": workbasket.pk},
+    )
     response = valid_user_client.get(url)
 
     assert response.status_code == 200
@@ -596,7 +604,10 @@ def test_workbasket_review_measures_filters_update_type(
         workbasket=session_workbasket,
     )
 
-    url = reverse("workbaskets:workbasket-ui-review-measures")
+    url = reverse(
+        "workbaskets:workbasket-ui-review-measures",
+        kwargs={"pk": session_workbasket.pk},
+    )
     search_filter = f"?update_type={update_type}"
     response = valid_user_client.get(url + search_filter)
     assert response.status_code == 200
@@ -619,7 +630,10 @@ def test_workbasket_review_measures_pagination(
         )
         factories.MeasureFactory.create_batch(40, transaction=unapproved_transaction)
 
-    url = reverse("workbaskets:workbasket-ui-review-measures")
+    url = reverse(
+        "workbaskets:workbasket-ui-review-measures",
+        kwargs={"pk": workbasket.pk},
+    )
     response = valid_user_client.get(url)
 
     assert response.status_code == 200
@@ -649,7 +663,10 @@ def test_workbasket_review_measures_conditions(valid_user_client):
         required_certificate=certificate,
         action__code="27",
     )
-    url = reverse("workbaskets:workbasket-ui-review-measures")
+    url = reverse(
+        "workbaskets:workbasket-ui-review-measures",
+        kwargs={"pk": workbasket.pk},
+    )
     response = valid_user_client.get(url)
     soup = BeautifulSoup(str(response.content), "html.parser")
     # 11th column is conditions. We're interested in the first (and only) row.
@@ -1406,7 +1423,7 @@ def test_workbasket_changes_view_sort_by_queryset(ordering_param, expected_order
     get_request = request.get(url + ordering_param)
     view = ui.WorkBasketChangesView(request=get_request, kwargs={"pk": workbasket.pk})
     assert list(view.get_queryset()) == list(
-        workbasket.tracked_models.order_by(expected_ordering),
+        workbasket.tracked_models.order_by(expected_ordering, "transaction"),
     )
 
 

workbaskets/urls.py

@@ -47,42 +47,42 @@
         name="edit-workbasket",
     ),
     path(
-        f"current/review-additional-codes/",
+        f"<pk>/review-additional-codes/",
         ui_views.WorkBasketReviewAdditionalCodesView.as_view(),
         name="workbasket-ui-review-additional-codes",
     ),
     path(
-        f"current/review-certificates/",
+        f"<pk>/review-certificates/",
         ui_views.WorkBasketReviewCertificatesView.as_view(),
         name="workbasket-ui-review-certificates",
     ),
     path(
-        f"current/review-goods/",
+        f"<pk>/review-goods/",
         ui_views.WorkbasketReviewGoodsView.as_view(),
         name="workbasket-ui-review-goods",
     ),
     path(
-        f"current/review-footnotes/",
+        f"<pk>/review-footnotes/",
         ui_views.WorkBasketReviewFootnotesView.as_view(),
         name="workbasket-ui-review-footnotes",
     ),
     path(
-        f"current/review-geographical-areas/",
+        f"<pk>/review-geographical-areas/",
         ui_views.WorkBasketReviewGeoAreasView.as_view(),
         name="workbasket-ui-review-geo-areas",
     ),
     path(
-        f"current/review-measures/",
+        f"<pk>/review-measures/",
         ui_views.WorkBasketReviewMeasuresView.as_view(),
         name="workbasket-ui-review-measures",
     ),
     path(
-        f"current/review-quotas/",
+        f"<pk>/review-quotas/",
         ui_views.WorkBasketReviewQuotasView.as_view(),
         name="workbasket-ui-review-quotas",
     ),
     path(
-        f"current/review-regulations/",
+        f"<pk>/review-regulations/",
         ui_views.WorkBasketReviewRegulationsView.as_view(),
         name="workbasket-ui-review-regulations",
     ),