From 49e1abc374c6a9a7e77e141b9c96f453f835039d Mon Sep 17 00:00:00 2001
From: Elliot Winkler <elliot.winkler@gmail.com>
Date: Fri, 3 Sep 2021 13:31:12 -0600
Subject: [PATCH] Sort contacts alphabetically (#11982)

* Sort contacts alphabetically

Contacts are grouped together by letter, and the groups are listed
alphabetically, but the contacts in each group are not sorted
alphabetically themselves.

Fixes #10318.

* Improve tests to be less brittle

* Remove this matcher

* Revert this file

* Also don't need this change anymore

* Don't need this data attribute either
---
 .../contact-list/contact-list.component.js    | 55 +++++++++---------
 .../app/contact-list/contact-list.test.js     | 58 +++++++++++++++++++
 .../recipient-group.component.js              | 10 +++-
 3 files changed, 95 insertions(+), 28 deletions(-)
 create mode 100644 ui/components/app/contact-list/contact-list.test.js

diff --git a/ui/components/app/contact-list/contact-list.component.js b/ui/components/app/contact-list/contact-list.component.js
index 04e3a3abd..713edfcfb 100644
--- a/ui/components/app/contact-list/contact-list.component.js
+++ b/ui/components/app/contact-list/contact-list.component.js
@@ -1,5 +1,6 @@
 import React, { PureComponent } from 'react';
 import PropTypes from 'prop-types';
+import { sortBy } from 'lodash';
 import Button from '../../ui/button';
 import RecipientGroup from './recipient-group/recipient-group.component';
 
@@ -50,34 +51,36 @@ export default class ContactList extends PureComponent {
   }
 
   renderAddressBook() {
-    const contacts = this.props.searchForContacts();
+    const unsortedContactsByLetter = this.props
+      .searchForContacts()
+      .reduce((obj, contact) => {
+        const firstLetter = contact.name[0].toUpperCase();
+        return {
+          ...obj,
+          [firstLetter]: [...(obj[firstLetter] || []), contact],
+        };
+      }, {});
 
-    const contactGroups = contacts.reduce((acc, contact) => {
-      const firstLetter = contact.name.slice(0, 1).toUpperCase();
-      acc[firstLetter] = acc[firstLetter] || [];
-      const bucket = acc[firstLetter];
-      bucket.push(contact);
-      return acc;
-    }, {});
+    const letters = Object.keys(unsortedContactsByLetter).sort();
 
-    return Object.entries(contactGroups)
-      .sort(([letter1], [letter2]) => {
-        if (letter1 > letter2) {
-          return 1;
-        } else if (letter1 === letter2) {
-          return 0;
-        }
-        return -1;
-      })
-      .map(([letter, groupItems]) => (
-        <RecipientGroup
-          key={`${letter}-contract-group`}
-          label={letter}
-          items={groupItems}
-          onSelect={this.props.selectRecipient}
-          selectedAddress={this.props.selectedAddress}
-        />
-      ));
+    const sortedContactGroups = letters.map((letter) => {
+      return [
+        letter,
+        sortBy(unsortedContactsByLetter[letter], (contact) => {
+          return contact.name.toLowerCase();
+        }),
+      ];
+    });
+
+    return sortedContactGroups.map(([letter, groupItems]) => (
+      <RecipientGroup
+        key={`${letter}-contact-group`}
+        label={letter}
+        items={groupItems}
+        onSelect={this.props.selectRecipient}
+        selectedAddress={this.props.selectedAddress}
+      />
+    ));
   }
 
   renderMyAccounts() {
diff --git a/ui/components/app/contact-list/contact-list.test.js b/ui/components/app/contact-list/contact-list.test.js
new file mode 100644
index 000000000..71b638dee
--- /dev/null
+++ b/ui/components/app/contact-list/contact-list.test.js
@@ -0,0 +1,58 @@
+import React from 'react';
+import { within } from '@testing-library/react';
+import configureMockStore from 'redux-mock-store';
+import { renderWithProvider } from '../../../../test/jest/rendering';
+import ContactList from '.';
+
+describe('Contact List', () => {
+  const store = configureMockStore([])({ metamask: {} });
+
+  describe('given searchForContacts', () => {
+    const selectRecipient = () => null;
+    const selectedAddress = null;
+
+    it('sorts contacts by name within each letter group', () => {
+      const { getAllByTestId } = renderWithProvider(
+        <ContactList
+          searchForContacts={() => {
+            return [
+              {
+                name: 'Al',
+                address: '0x0000000000000000000000000000000000000000',
+              },
+              {
+                name: 'aa',
+                address: '0x0000000000000000000000000000000000000001',
+              },
+              {
+                name: 'Az',
+                address: '0x0000000000000000000000000000000000000002',
+              },
+              {
+                name: 'bbb',
+                address: '0x0000000000000000000000000000000000000003',
+              },
+            ];
+          }}
+          selectRecipient={selectRecipient}
+          selectedAddress={selectedAddress}
+        />,
+        store,
+      );
+
+      const recipientGroups = getAllByTestId('recipient-group');
+      expect(within(recipientGroups[0]).getByText('A')).toBeInTheDocument();
+      const recipientsInA = within(recipientGroups[0]).getAllByTestId(
+        'recipient',
+      );
+      expect(recipientsInA[0]).toHaveTextContent('aa0x0000...0001');
+      expect(recipientsInA[1]).toHaveTextContent('Al0x0000...0000');
+      expect(recipientsInA[2]).toHaveTextContent('Az0x0000...0002');
+      expect(within(recipientGroups[1]).getByText('B')).toBeInTheDocument();
+      const recipientsInB = within(recipientGroups[1]).getAllByTestId(
+        'recipient',
+      );
+      expect(recipientsInB[0]).toHaveTextContent('bbb0x0000...0003');
+    });
+  });
+});
diff --git a/ui/components/app/contact-list/recipient-group/recipient-group.component.js b/ui/components/app/contact-list/recipient-group/recipient-group.component.js
index d57854917..a8fd589db 100644
--- a/ui/components/app/contact-list/recipient-group/recipient-group.component.js
+++ b/ui/components/app/contact-list/recipient-group/recipient-group.component.js
@@ -19,7 +19,10 @@ export default function RecipientGroup({
   }
 
   return (
-    <div className="send__select-recipient-wrapper__group">
+    <div
+      className="send__select-recipient-wrapper__group"
+      data-testid="recipient-group"
+    >
       {label && (
         <div className="send__select-recipient-wrapper__group-label">
           {label}
@@ -41,7 +44,10 @@ export default function RecipientGroup({
           })}
         >
           <Identicon address={address} diameter={28} />
-          <div className="send__select-recipient-wrapper__group-item__content">
+          <div
+            className="send__select-recipient-wrapper__group-item__content"
+            data-testid="recipient"
+          >
             <div className="send__select-recipient-wrapper__group-item__title">
               {name || ellipsify(address)}
             </div>