Sort Notifications recipients (#1665)

https://eaflood.atlassian.net/browse/WATER-4775 This change introduces a sort on the recipients array in the presenter. It sorts by the contacts name. We need to work out the name before we can sort. So we first map the recipients and then sort. This will result in all the recipients being mapped and sorted, with only 25 (the current default pagination per page) of the results being used. We have deemed this acceptable for the sake of simplicity and have accounted for performance concerns.
DEFRA · Jan 29, 2025 · 2eb03c5 · 2eb03c5
1 parent fe52ea6
commit 2eb03c5
Show file tree

Hide file tree

Showing 4 changed files with 139 additions and 56 deletions.
diff --git a/app/presenters/notifications/setup/review.presenter.js b/app/presenters/notifications/setup/review.presenter.js
@@ -13,7 +13,7 @@ const { defaultPageSize } = require('../../../../config/database.config.js')
  *
  * @param {object[]} recipients - List of recipient objects, each containing recipient details like email or name.
  * @param {number|string} page - The currently selected page
- * @param {object} pagination -
+ * @param {object} pagination - The result from `PaginatorPresenter`
  *
  * @returns {object} - The data formatted for the view template
  */
@@ -29,13 +29,10 @@ function go(recipients, page, pagination) {
 /**
  * Contact can be an email or an address (letter)
  *
- * If it is an address then we convert the contact CSV string to an array
+ * If it is an address then we convert the contact CSV string to an array. If it is an email we return the email in
+ * array for the UI to have consistent formatting.
  *
- * If it is an email we return the email in array for the UI to have consistent formatting
- *
- * @param {object} recipient
- *
- * @returns {string[]}
+ * @private
  */
 function _contact(recipient) {
   if (recipient.email) {
@@ -44,17 +41,18 @@ function _contact(recipient) {
 
   const name = contactName(recipient.contact)
   const address = contactAddress(recipient.contact)
+
   return [name, ...address]
 }
 
-/**
- * Convert the licence CSV string to an array
- *
- * @param {string} licences
- * @returns {string[]}
- */
-function _licences(licences) {
-  return licences.split(',')
+function _formatRecipients(recipients) {
+  return recipients.map((recipient) => {
+    return {
+      contact: _contact(recipient),
+      licences: recipient.licence_refs.split(','),
+      method: `${recipient.message_type} - ${recipient.contact_type}`
+    }
+  })
 }
 
 function _pageTitle(page, pagination) {
@@ -66,24 +64,55 @@ function _pageTitle(page, pagination) {
 }
 
 /**
- * Due to the complexity of the query to get the recipients data, we handle pagination in the presenter.
+ * Due to the complexity of the query we had to use a raw query to get the recipients data. This means we need to handle
+ * pagination (which recipients to display based on selected page and page size) in here.
  *
  * @private
  */
-function _pagination(recipients, page) {
+function _paginateRecipients(recipients, page) {
   const pageNumber = Number(page) * defaultPageSize
+
   return recipients.slice(pageNumber - defaultPageSize, pageNumber)
 }
 
+/**
+ * Sorts, maps, and paginates the recipients list.
+ *
+ * This function first maps over the recipients to transform each recipient object into a new format, then sorts the
+ * resulting array of transformed recipients alphabetically by their contact's name. After sorting, it uses pagination
+ * to return only the relevant subset of recipients for the given page.
+ *
+ * The map and sort are performed before pagination, as it is necessary to have the recipients in a defined order before
+ * determining which recipients should appear on the page.
+ *
+ * @private
+ */
 function _recipients(recipients, page) {
-  const paginatedRecipients = _pagination(recipients, page)
+  const formattedRecipients = _formatRecipients(recipients)
+  const sortedRecipients = _sortRecipients(formattedRecipients)
 
-  return paginatedRecipients.map((recipient) => {
-    return {
-      contact: _contact(recipient),
-      licences: _licences(recipient.licence_refs),
-      method: `${recipient.message_type} - ${recipient.contact_type}`
+  return _paginateRecipients(sortedRecipients, page)
+}
+
+/**
+ * Sorts the recipients alphabetically by their 'contact name'.
+ *
+ * The contact name is the first element in the recipient's `contact` array. For letter-based recipients this will
+ * be either the person or organisation name, and for email recipients this will be the email address.
+ *
+ * @private
+ */
+function _sortRecipients(recipients) {
+  return recipients.sort((a, b) => {
+    if (a.contact[0] < b.contact[0]) {
+      return -1
+    }
+
+    if (a.contact[0] > b.contact[0]) {
+      return 1
     }
+
+    return 0
   })
 }
 

diff --git a/test/fixtures/recipients.fixtures.js b/test/fixtures/recipients.fixtures.js
@@ -25,9 +25,11 @@ function recipients() {
  */
 function duplicateRecipients() {
   const duplicateLicenceRef = generateLicenceRef()
+  const licenceDuplicateLicenceRef = generateLicenceRef()
+
   return {
-    duplicateLicenceHolder: _addDuplicateLicenceHolder(duplicateLicenceRef),
-    duplicateReturnsTo: _addDuplicateReturnsTo(duplicateLicenceRef),
+    duplicateLicenceHolder: _addDuplicateLicenceHolder(licenceDuplicateLicenceRef),
+    duplicateReturnsTo: _addDuplicateReturnsTo(licenceDuplicateLicenceRef),
     duplicatePrimaryUser: _addDuplicatePrimaryUser(duplicateLicenceRef),
     duplicateReturnsAgent: _addDuplicateReturnsAgent(duplicateLicenceRef)
   }
@@ -45,7 +47,7 @@ function _addDuplicateLicenceHolder(licenceRef) {
 function _addDuplicateReturnsTo(licenceRef) {
   return {
     licence_refs: licenceRef,
-    contact_type: 'Returns To',
+    contact_type: 'Returns to',
     contact: _contact('4', 'Duplicate Returns to', 'Returns to'),
     contact_hash_id: 'b1b355491c7d42778890c545e08797ea'
   }
@@ -96,7 +98,7 @@ function _addDuplicateReturnsAgent(licenceRef) {
     contact: null,
     contact_hash_id: '2e6918568dfbc1d78e2fbe279fftt990',
     contact_type: 'Returns agent',
-    recipient: '[email protected]'
+    email: '[email protected]'
   }
 }
 

diff --git a/test/presenters/notifications/setup/review.presenter.test.js b/test/presenters/notifications/setup/review.presenter.test.js
@@ -18,6 +18,7 @@ describe('Notifications Setup - Review presenter', () => {
   let pagination
   let testInput
   let testRecipients
+  let testDuplicateRecipients
 
   beforeEach(() => {
     page = 1
@@ -26,8 +27,12 @@ describe('Notifications Setup - Review presenter', () => {
     }
 
     testRecipients = RecipientsFixture.recipients()
+    // This data is used to ensure the recipients are grouped when they have the same licence ref / name.
+    // Ignore the fact that these would be considered duplicates elsewhere in the code
+    // (e.g. contact hash / address being identical)
+    testDuplicateRecipients = RecipientsFixture.duplicateRecipients()
 
-    testInput = Object.values(testRecipients).map((recipient) => {
+    testInput = [...Object.values(testRecipients), ...Object.values(testDuplicateRecipients)].map((recipient) => {
       return {
         ...recipient,
         // The determine recipients service will add the message_type relevant to the recipient
@@ -39,33 +44,28 @@ describe('Notifications Setup - Review presenter', () => {
   })
 
   describe('when provided with "recipients"', () => {
-    it('correctly presents the data', () => {
+    it('correctly presents the data (in alphabetical order)', () => {
       const result = ReviewPresenter.go(testInput, page, pagination)
 
       expect(result).to.equal({
         defaultPageSize: 25,
         pageTitle: 'Send returns invitations',
         recipients: [
           {
-            contact: ['[email protected]'],
-            licences: [testRecipients.primaryUser.licence_refs],
-            method: 'Letter or email - Primary user'
+            contact: ['Mr H J Duplicate Licence holder', '4', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
+            licences: [testDuplicateRecipients.duplicateLicenceHolder.licence_refs],
+            method: 'Letter or email - Licence holder'
           },
           {
-            contact: ['[email protected]'],
-            licences: [testRecipients.returnsAgent.licence_refs],
-            method: 'Letter or email - Returns agent'
+            contact: ['Mr H J Duplicate Returns to', '4', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
+            licences: [testDuplicateRecipients.duplicateReturnsTo.licence_refs],
+            method: 'Letter or email - Returns to'
           },
           {
             contact: ['Mr H J Licence holder', '1', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
             licences: [testRecipients.licenceHolder.licence_refs],
             method: 'Letter or email - Licence holder'
           },
-          {
-            contact: ['Mr H J Returns to', '2', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
-            licences: [testRecipients.returnsTo.licence_refs],
-            method: 'Letter or email - Returns to'
-          },
           {
             contact: [
               'Mr H J Licence holder with multiple licences',
@@ -77,9 +77,34 @@ describe('Notifications Setup - Review presenter', () => {
             ],
             licences: testRecipients.licenceHolderWithMultipleLicences.licence_refs.split(','),
             method: 'Letter or email - Licence holder'
+          },
+          {
+            contact: ['Mr H J Returns to', '2', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
+            licences: [testRecipients.returnsTo.licence_refs],
+            method: 'Letter or email - Returns to'
+          },
+          {
+            contact: ['[email protected]'],
+            licences: [testRecipients.primaryUser.licence_refs],
+            method: 'Letter or email - Primary user'
+          },
+          {
+            contact: ['[email protected]'],
+            licences: [testDuplicateRecipients.duplicatePrimaryUser.licence_refs],
+            method: 'Letter or email - Primary user'
+          },
+          {
+            contact: ['[email protected]'],
+            licences: [testRecipients.returnsAgent.licence_refs],
+            method: 'Letter or email - Returns agent'
+          },
+          {
+            contact: ['[email protected]'],
+            licences: [testDuplicateRecipients.duplicateReturnsAgent.licence_refs],
+            method: 'Letter or email - Returns agent'
           }
         ],
-        recipientsAmount: 5
+        recipientsAmount: 9
       })
     })
 
@@ -90,25 +115,27 @@ describe('Notifications Setup - Review presenter', () => {
 
           expect(result.recipients).to.equal([
             {
-              contact: ['[email protected]'],
-              licences: [testRecipients.primaryUser.licence_refs],
-              method: 'Letter or email - Primary user'
+              contact: [
+                'Mr H J Duplicate Licence holder',
+                '4',
+                'Privet Drive',
+                'Little Whinging',
+                'Surrey',
+                'WD25 7LR'
+              ],
+              licences: [testDuplicateRecipients.duplicateLicenceHolder.licence_refs],
+              method: 'Letter or email - Licence holder'
             },
             {
-              contact: ['[email protected]'],
-              licences: [testRecipients.returnsAgent.licence_refs],
-              method: 'Letter or email - Returns agent'
+              contact: ['Mr H J Duplicate Returns to', '4', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
+              licences: [testDuplicateRecipients.duplicateReturnsTo.licence_refs],
+              method: 'Letter or email - Returns to'
             },
             {
               contact: ['Mr H J Licence holder', '1', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
               licences: [testRecipients.licenceHolder.licence_refs],
               method: 'Letter or email - Licence holder'
             },
-            {
-              contact: ['Mr H J Returns to', '2', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
-              licences: [testRecipients.returnsTo.licence_refs],
-              method: 'Letter or email - Returns to'
-            },
             {
               contact: [
                 'Mr H J Licence holder with multiple licences',
@@ -120,6 +147,31 @@ describe('Notifications Setup - Review presenter', () => {
               ],
               licences: testRecipients.licenceHolderWithMultipleLicences.licence_refs.split(','),
               method: 'Letter or email - Licence holder'
+            },
+            {
+              contact: ['Mr H J Returns to', '2', 'Privet Drive', 'Little Whinging', 'Surrey', 'WD25 7LR'],
+              licences: [testRecipients.returnsTo.licence_refs],
+              method: 'Letter or email - Returns to'
+            },
+            {
+              contact: ['[email protected]'],
+              licences: [testRecipients.primaryUser.licence_refs],
+              method: 'Letter or email - Primary user'
+            },
+            {
+              contact: ['[email protected]'],
+              licences: [testDuplicateRecipients.duplicatePrimaryUser.licence_refs],
+              method: 'Letter or email - Primary user'
+            },
+            {
+              contact: ['[email protected]'],
+              licences: [testRecipients.returnsAgent.licence_refs],
+              method: 'Letter or email - Returns agent'
+            },
+            {
+              contact: ['[email protected]'],
+              licences: [testDuplicateRecipients.duplicateReturnsAgent.licence_refs],
+              method: 'Letter or email - Returns agent'
             }
           ])
         })
@@ -199,7 +251,7 @@ describe('Notifications Setup - Review presenter', () => {
 
       describe('and there are >= 25 recipients', () => {
         beforeEach(() => {
-          testInput = [...testInput, ...testInput, ...testInput, ...testInput, ...testInput, ...testInput]
+          testInput = [...testInput, ...testInput, ...testInput]
 
           pagination = {
             numberOfPages: 2
@@ -228,7 +280,7 @@ describe('Notifications Setup - Review presenter', () => {
           it('returns the remaining recipients', () => {
             const result = ReviewPresenter.go(testInput, page, pagination)
 
-            expect(result.recipients.length).to.equal(5)
+            expect(result.recipients.length).to.equal(2)
           })
 
           it('returns the updated "pageTitle"', () => {

diff --git a/test/services/notifications/setup/determine-recipients.service.test.js b/test/services/notifications/setup/determine-recipients.service.test.js
@@ -132,7 +132,7 @@ describe('Notifications Setup - Determine Recipients service', () => {
             type: 'Person'
           },
           contact_hash_id: 'b1b355491c7d42778890c545e08797ea',
-          contact_type: 'Licence holder',
+          contact_type: 'both',
           email: null,
           licence_refs: testDuplicateRecipients.duplicateLicenceHolder.licence_refs,
           message_type: 'Letter'
@@ -194,7 +194,7 @@ describe('Notifications Setup - Determine Recipients service', () => {
               type: 'Person'
             },
             contact_hash_id: 'b1b355491c7d42778890c545e08797ea',
-            contact_type: 'Licence holder',
+            contact_type: 'both',
             email: null,
             licence_refs: testDuplicateRecipients.duplicateLicenceHolder.licence_refs,
             message_type: 'Letter'