From e118dfbd6f1702b81822182a75ccfcb1b79ed991 Mon Sep 17 00:00:00 2001
From: Ben Chidgey <ben@feedmypixel.com>
Date: Fri, 6 Sep 2019 15:37:36 +0100
Subject: [PATCH 1/4] Build summary information for move sidebar

This work builds and presents the data required for the create move
journey sidebar
---
 app/move/controllers/create/base.js           | 16 ++++
 app/move/controllers/create/base.test.js      | 94 +++++++++++++++++++
 app/move/controllers/create/move-details.js   | 18 ++++
 .../controllers/create/move-details.test.js   | 91 ++++++++++++++++++
 4 files changed, 219 insertions(+)

diff --git a/app/move/controllers/create/base.js b/app/move/controllers/create/base.js
index be7b42306..336f27792 100644
--- a/app/move/controllers/create/base.js
+++ b/app/move/controllers/create/base.js
@@ -1,4 +1,5 @@
 const FormWizardController = require('../../../../common/controllers/form-wizard')
+const presenters = require('../../../../common/presenters')
 
 class CreateBaseController extends FormWizardController {
   middlewareChecks() {
@@ -9,6 +10,7 @@ class CreateBaseController extends FormWizardController {
   middlewareLocals() {
     super.middlewareLocals()
     this.use(this.setCancelUrl)
+    this.use(this.setMoveSummary)
   }
 
   setCancelUrl(req, res, next) {
@@ -24,6 +26,20 @@ class CreateBaseController extends FormWizardController {
 
     next()
   }
+
+  setMoveSummary(req, res, next) {
+    const currentLocation = req.session.currentLocation
+    const sessionModel = req.sessionModel.toJSON()
+    const moveSummary = presenters.moveToMetaListComponent({
+      ...sessionModel,
+      from_location: currentLocation,
+    })
+
+    res.locals.person = sessionModel.person
+    res.locals.moveSummary = sessionModel.move_type ? moveSummary : {}
+
+    next()
+  }
 }
 
 module.exports = CreateBaseController
diff --git a/app/move/controllers/create/base.test.js b/app/move/controllers/create/base.test.js
index 75f545167..7e5764297 100644
--- a/app/move/controllers/create/base.test.js
+++ b/app/move/controllers/create/base.test.js
@@ -123,5 +123,99 @@ describe('Move controllers', function() {
         })
       })
     })
+
+    describe('#setMoveSummary()', function() {
+      let req, res, nextSpy
+
+      const mockPerson = {
+        first_names: 'Mr',
+        fullname: 'Benn, Mr',
+        last_name: 'Benn',
+      }
+      const mockSessionModel = overrides => {
+        const sessionModel = {
+          date: '2019-06-09',
+          time_due: '2000-01-01T14:00:00Z',
+          move_type: 'court_appearance',
+          to_location: {
+            title: 'Mock to location',
+          },
+          additional_information: 'Additional information',
+          person: mockPerson,
+          ...overrides,
+        }
+
+        return {
+          ...sessionModel,
+          toJSON: () => sessionModel,
+          get: () => sessionModel.person,
+        }
+      }
+      const expectedMoveSummary = {
+        items: [
+          { key: { text: 'From' }, value: { text: 'Mock location' } },
+          {
+            key: { text: 'To' },
+            value: { text: 'Mock to location — Additional information' },
+          },
+          { key: { text: 'Date' }, value: { text: 'Sunday 9 Jun 2019' } },
+          { key: { text: 'Time due' }, value: { text: '2pm' } },
+        ],
+      }
+
+      beforeEach(function() {
+        nextSpy = sinon.spy()
+        req = {
+          session: {
+            currentLocation: {
+              title: 'Mock location',
+            },
+          },
+        }
+        res = {
+          locals: {},
+        }
+      })
+
+      context('when current location exists', function() {
+        beforeEach(async function() {
+          req.sessionModel = mockSessionModel()
+
+          await controller.setMoveSummary(req, res, nextSpy)
+        })
+
+        it('should set locals as expected', function() {
+          expect(res.locals).to.deep.equal({
+            person: mockPerson,
+            moveSummary: expectedMoveSummary,
+          })
+        })
+
+        it('should call next without error', function() {
+          expect(nextSpy).to.be.calledOnceWithExactly()
+        })
+      })
+
+      context('without move_type', function() {
+        beforeEach(async function() {
+          req.sessionModel = mockSessionModel({
+            move_type: '',
+          })
+
+          await controller.setMoveSummary(req, res, nextSpy)
+        })
+
+        it('should set locals as expected', function() {
+          expect(res.locals).to.deep.equal({
+            person: mockPerson,
+            moveSummary: {},
+          })
+        })
+
+        it('should call next without error', function() {
+          expect(nextSpy).to.be.calledOnceWithExactly()
+        })
+      })
+    })
   })
 })
diff --git a/app/move/controllers/create/move-details.js b/app/move/controllers/create/move-details.js
index 2fc408c8f..48ba544e3 100644
--- a/app/move/controllers/create/move-details.js
+++ b/app/move/controllers/create/move-details.js
@@ -68,6 +68,24 @@ class MoveDetailsController extends CreateBaseController {
 
     next()
   }
+
+  async successHandler(req, res, next) {
+    try {
+      const { to_location: toLocationId } = req.sessionModel.toJSON()
+
+      if (toLocationId) {
+        const locationDetail = await referenceDataService.getLocationById(
+          toLocationId
+        )
+
+        req.sessionModel.set('to_location', locationDetail)
+      }
+
+      super.successHandler(req, res, next)
+    } catch (error) {
+      next(error)
+    }
+  }
 }
 
 module.exports = MoveDetailsController
diff --git a/app/move/controllers/create/move-details.test.js b/app/move/controllers/create/move-details.test.js
index 720094482..1e433d7f1 100644
--- a/app/move/controllers/create/move-details.test.js
+++ b/app/move/controllers/create/move-details.test.js
@@ -292,5 +292,96 @@ describe('Move controllers', function() {
         })
       })
     })
+
+    describe('#successHandler()', function() {
+      const toLocationId = '123456-7'
+      const mockLocationDetail = {
+        title: 'mock to location',
+      }
+      let req, res, nextSpy
+
+      beforeEach(function() {
+        nextSpy = sinon.spy()
+        req = {
+          sessionModel: {
+            toJSON: sinon.stub(),
+            set: sinon.spy(),
+          },
+        }
+        res = {
+          locals: {},
+        }
+
+        sinon.spy(FormController.prototype, 'successHandler')
+      })
+
+      context('with location_id', function() {
+        beforeEach(async function() {
+          req.sessionModel.toJSON.returns({ to_location: toLocationId })
+          sinon
+            .stub(referenceDataService, 'getLocationById')
+            .resolves(mockLocationDetail)
+
+          await controller.successHandler(req, res, nextSpy)
+        })
+
+        it('should set to_location in session as expected', function() {
+          expect(req.sessionModel.set).to.be.calledOnceWithExactly(
+            'to_location',
+            mockLocationDetail
+          )
+        })
+
+        it('should call parent successHandler', function() {
+          expect(FormController.prototype.successHandler).to.be.calledOnceWith(
+            req,
+            res,
+            nextSpy
+          )
+        })
+      })
+
+      context('without location_id', function() {
+        beforeEach(async function() {
+          req.sessionModel.toJSON.returns({ to_location: '' })
+          sinon
+            .stub(referenceDataService, 'getLocationById')
+            .resolves(mockLocationDetail)
+
+          await controller.successHandler(req, res, nextSpy)
+        })
+
+        it('should not set to_location in session', function() {
+          expect(req.sessionModel.set).not.to.be.called
+        })
+
+        it('should call parent successHandler', function() {
+          expect(FormController.prototype.successHandler).to.be.calledOnceWith(
+            req,
+            res,
+            nextSpy
+          )
+        })
+      })
+
+      context('when getLocationById throws and error', function() {
+        const errorMock = new Error('Problem')
+
+        beforeEach(async function() {
+          req.sessionModel.toJSON.returns({ to_location: toLocationId })
+          sinon.stub(referenceDataService, 'getLocationById').throws(errorMock)
+
+          await controller.successHandler(req, res, nextSpy)
+        })
+
+        it('should call next with error', function() {
+          expect(nextSpy).to.be.calledOnceWithExactly(errorMock)
+        })
+
+        it('should not call parent successHandler', function() {
+          expect(FormController.prototype.successHandler).not.to.be.called
+        })
+      })
+    })
   })
 })

From 66a57a11f170fa1fd7f108dbe13358778f55ebe9 Mon Sep 17 00:00:00 2001
From: Ben Chidgey <ben@feedmypixel.com>
Date: Fri, 6 Sep 2019 15:38:46 +0100
Subject: [PATCH 2/4] Create two column template layout

Create a two column template layout to be used with views that require a
sidebar/aside
---
 common/templates/layouts/two-column.njk | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 common/templates/layouts/two-column.njk

diff --git a/common/templates/layouts/two-column.njk b/common/templates/layouts/two-column.njk
new file mode 100644
index 000000000..6d6b08f2e
--- /dev/null
+++ b/common/templates/layouts/two-column.njk
@@ -0,0 +1,24 @@
+{% extends "./base.njk" %}
+
+{% set containerClass = ["govuk-grid-row"].concat(additionalContainerClass) | join(" ") %}
+
+{% block content %}
+
+  {% block contentHeader %}{% endblock %}
+
+  <div class="{{ containerClass }}">
+
+    <div class="govuk-grid-column-two-thirds">
+
+      {% block contentMain %}{% endblock %}
+
+    </div>
+
+    <div class="govuk-grid-column-one-third">
+
+      {% block contentSidebar %}{% endblock %}
+
+    </div>
+
+  </div>
+{% endblock %}

From 6991b77ee7b1cdd4dd36c04a139146992caa8a46 Mon Sep 17 00:00:00 2001
From: Ben Chidgey <ben@feedmypixel.com>
Date: Fri, 6 Sep 2019 15:39:52 +0100
Subject: [PATCH 3/4] Implement two column layout for views

Use the two column layout for:
- Move detail view
- Form wizard
---
 app/move/views/view.njk          | 54 +++++++++++++------------
 common/templates/form-wizard.njk | 68 +++++++++++++++++++-------------
 2 files changed, 69 insertions(+), 53 deletions(-)

diff --git a/app/move/views/view.njk b/app/move/views/view.njk
index 618e54c1f..1c58148ef 100644
--- a/app/move/views/view.njk
+++ b/app/move/views/view.njk
@@ -1,4 +1,6 @@
-{% extends "layouts/base.njk" %}
+{% extends "layouts/two-column.njk" %}
+
+{% set additionalContainerClass = "sticky-sidebar-container" %}
 
 {% block customGtagConfig %}
   gtag('set', {'page_title': 'Move details'});
@@ -39,7 +41,7 @@
   {% endif %}
 {% endblock %}
 
-{% block content %}
+{% block contentHeader %}
   <header class="govuk-!-margin-bottom-8">
     <h1 class="govuk-heading-xl govuk-!-margin-bottom-1">
       {{ move.person.fullname | upper }}
@@ -59,36 +61,36 @@
       <div>
     {% endif %}
   </header>
+{% endblock %}
 
-  <div class="govuk-grid-row sticky-sidebar-container">
-    <div class="govuk-grid-column-two-thirds">
-      <h2 class="govuk-heading-m">
-        {{ t("moves::steps.personal_details.heading") }}
-      </h2>
+{% block contentMain %}
+  <h2 class="govuk-heading-m">
+    {{ t("moves::steps.personal_details.heading") }}
+  </h2>
 
-      {{ govukSummaryList(personalDetailsSummary) }}
+  {{ govukSummaryList(personalDetailsSummary) }}
 
-      {% include "move/views/_includes/assessment.njk" %}
+  {% include "move/views/_includes/assessment.njk" %}
 
-      {% if canAccess("move:cancel") and move.status == "requested" %}
-        <p class="govuk-!-margin-top-9 govuk-!-margin-bottom-0">
-          <a href="{{ move.id }}/cancel" class="app-link--destructive">
-            {{ t("actions::cancel_move") }}
-          </a>
-        </p>
-      {% endif %}
-    </div>
+  {% if canAccess("move:cancel") and move.status == "requested" %}
+    <p class="govuk-!-margin-top-9 govuk-!-margin-bottom-0">
+      <a href="{{ move.id }}/cancel" class="app-link--destructive">
+        {{ t("actions::cancel_move") }}
+      </a>
+    </p>
+  {% endif %}
+{% endblock %}
 
-    <div class="govuk-grid-column-one-third">
-      <div class="sticky-sidebar">
-        <div class="sticky-sidebar__inner">
-          <h3 class="govuk-heading-m app-border-top-2 app-border--blue govuk-!-padding-top-4">
-            {{ move.person.fullname | upper }}
-          </h3>
+{% block contentSidebar %}
+  {% if moveSummary %}
+    <div class="sticky-sidebar">
+      <div class="sticky-sidebar__inner">
+        <h3 class="govuk-heading-m app-border-top-2 app-border--blue govuk-!-padding-top-4">
+          {{ move.person.fullname | upper }}
+        </h3>
 
-          {{ appMetaList(moveSummary) }}
-        </div>
+        {{ appMetaList(moveSummary) }}
       </div>
     </div>
-  </div>
+  {% endif %}
 {% endblock %}
diff --git a/common/templates/form-wizard.njk b/common/templates/form-wizard.njk
index 3bda4a6ee..0feb8d67b 100644
--- a/common/templates/form-wizard.njk
+++ b/common/templates/form-wizard.njk
@@ -1,4 +1,6 @@
-{% extends "layouts/base.njk" %}
+{% extends "layouts/two-column.njk" %}
+
+{% set additionalContainerClass = "sticky-sidebar-container" %}
 
 {% block pageTitle %}
   {{ t(options.pageTitle) }} - {{ t(options.journeyPageTitle) }}
@@ -15,7 +17,7 @@
   {% endif %}
 {% endblock %}
 
-{% block content %}
+{% block contentHeader %}
   {% if errors.errorList.length %}
     {{ govukErrorSummary({
       titleText: t("validation::summary.heading"),
@@ -30,35 +32,47 @@
       </h1>
     </div>
   </header>
+{% endblock %}
 
-  <div class="govuk-grid-row">
-    <div class="govuk-grid-column-two-thirds">
-      <form method="post">
-        <input type="hidden" name="x-csrf-token" value="{{ getLocal('csrf-token') }}">
+{% block contentMain %}
+  <form method="post">
+    <input type="hidden" name="x-csrf-token" value="{{ getLocal('csrf-token') }}">
 
-        {% block beforeFields %}{% endblock %}
+    {% block beforeFields %}{% endblock %}
 
-        {% block fields %}
-          {% for key, fieldsOptions in options.fields %}
-            {% if fieldsOptions.component and not fieldsOptions.skip %}
-              {{ callAsMacro(fieldsOptions.component)(fieldsOptions) }}
-            {% endif %}
-          {% endfor %}
-        {% endblock %}
+    {% block fields %}
+      {% for key, fieldsOptions in options.fields %}
+        {% if fieldsOptions.component and not fieldsOptions.skip %}
+          {{ callAsMacro(fieldsOptions.component)(fieldsOptions) }}
+        {% endif %}
+      {% endfor %}
+    {% endblock %}
 
-        {% block formActions %}
-          {{ govukButton({
-            text: t(options.buttonText or "actions::continue"),
-            classes: options.buttonClasses
-          }) }}
+    {% block formActions %}
+      {{ govukButton({
+        text: t(options.buttonText or "actions::continue"),
+        classes: options.buttonClasses
+      }) }}
 
-          {% if cancelUrl %}
-            <a href="{{ cancelUrl }}" class="govuk-button govuk-button--text">
-              {{ t("actions::cancel") }}
-            </a>
-          {% endif %}
-        {% endblock %}
-      </form>
+      {% if cancelUrl %}
+        <a href="{{ cancelUrl }}" class="govuk-button govuk-button--text">
+          {{ t("actions::cancel") }}
+        </a>
+      {% endif %}
+    {% endblock %}
+  </form>
+{% endblock %}
+
+{% block contentSidebar %}
+  {% if person %}
+    <div class="sticky-sidebar">
+      <div class="sticky-sidebar__inner">
+        <h3 class="govuk-heading-m app-border-top-2 app-border--blue govuk-!-padding-top-4">
+          {{ person.fullname | upper }}
+        </h3>
+
+        {{ appMetaList(moveSummary) }}
+      </div>
     </div>
-  </div>
+  {% endif %}
 {% endblock %}

From 9332fc36a62292860eeece36e9605bc91a3b9b42 Mon Sep 17 00:00:00 2001
From: Ben Chidgey <ben@feedmypixel.com>
Date: Fri, 6 Sep 2019 15:41:02 +0100
Subject: [PATCH 4/4] Refactor of the move meta list presenter

This work involves:

- remove the eslint camelcase warning
- update snakecase destructuring to camelcase
- remove underscore for psuedo private methods
- move module export to bottom
---
 .../presenters/move-to-meta-list-component.js | 31 +++++++++----------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/common/presenters/move-to-meta-list-component.js b/common/presenters/move-to-meta-list-component.js
index 174af8cf0..e41f62250 100644
--- a/common/presenters/move-to-meta-list-component.js
+++ b/common/presenters/move-to-meta-list-component.js
@@ -1,11 +1,10 @@
-/* eslint-disable camelcase */
 const { isToday, isTomorrow, isYesterday } = require('date-fns')
 const { get } = require('lodash')
 
 const i18n = require('../../config/i18n')
 const filters = require('../../config/nunjucks/filters')
 
-function _isRelativeDate(date) {
+function isRelativeDate(date) {
   if (isToday(date) || isTomorrow(date) || isYesterday(date)) {
     return true
   }
@@ -13,30 +12,28 @@ function _isRelativeDate(date) {
   return false
 }
 
-module.exports = function moveToMetaListComponent({
+function moveToMetaListComponent({
   date,
-  time_due,
-  move_type,
-  from_location,
-  to_location,
-  additional_information,
+  time_due: timeDue,
+  move_type: moveType,
+  from_location: fromLocation,
+  to_location: toLocation,
+  additional_information: additionalInfo,
 } = {}) {
   const dateWithDay = filters.formatDateWithDay(date)
-  const destination = get(to_location, 'title', 'Unknown')
+  const destination = get(toLocation, 'title', 'Unknown')
   const destinationLabel =
-    move_type === 'prison_recall'
+    moveType === 'prison_recall'
       ? i18n.t('fields::move_type.items.prison_recall.label')
       : destination
-  const additionalInformation = additional_information
-    ? ` — ${additional_information}`
-    : ''
+  const additionalInformation = additionalInfo ? ` — ${additionalInfo}` : ''
   const items = [
     {
       key: {
         text: i18n.t('fields::from_location.short_label'),
       },
       value: {
-        text: get(from_location, 'title'),
+        text: get(fromLocation, 'title'),
       },
     },
     {
@@ -52,7 +49,7 @@ module.exports = function moveToMetaListComponent({
         text: i18n.t('fields::date_type.label'),
       },
       value: {
-        text: _isRelativeDate(date)
+        text: isRelativeDate(date)
           ? `${dateWithDay} (${filters.formatDateAsRelativeDay(date)})`
           : dateWithDay,
       },
@@ -62,7 +59,7 @@ module.exports = function moveToMetaListComponent({
         text: i18n.t('fields::time_due.label'),
       },
       value: {
-        text: filters.formatTime(time_due),
+        text: filters.formatTime(timeDue),
       },
     },
   ]
@@ -71,3 +68,5 @@ module.exports = function moveToMetaListComponent({
     items,
   }
 }
+
+module.exports = moveToMetaListComponent