From adbe989949a54a4e907cb74bf8119661020d7d8a Mon Sep 17 00:00:00 2001
From: Scott Nonnenberg <scott@signal.org>
Date: Tue, 8 May 2018 10:31:16 -0700
Subject: [PATCH] validateContact: Return error instead of logging

---
 js/modules/types/contact.js        | 23 ++++----
 test/modules/types/contact_test.js | 86 ++++++++++++++----------------
 2 files changed, 55 insertions(+), 54 deletions(-)

diff --git a/js/modules/types/contact.js b/js/modules/types/contact.js
index 7e6d6b667..4c0e5d9d0 100644
--- a/js/modules/types/contact.js
+++ b/js/modules/types/contact.js
@@ -1,5 +1,7 @@
 const { omit, compact, map } = require('lodash');
 
+const { toLogFormat } = require('./errors');
+
 exports.parseAndWriteContactAvatar = upgradeAttachment => async (
   contact,
   context = {}
@@ -18,10 +20,15 @@ exports.parseAndWriteContactAvatar = upgradeAttachment => async (
   // eliminates empty numbers, emails, and addresses; adds type if not provided
   const contactWithCleanedElements = parseContact(contactWithUpdatedAvatar);
 
-  // We'll log if the contact is invalid, leave everything as-is
-  validateContact(contactWithCleanedElements, {
+  const error = exports._validateContact(contactWithCleanedElements, {
     messageId: idForLogging(message),
   });
+  if (error) {
+    console.log(
+      'Contact.parseAndWriteContactAvatar: contact was malformed.',
+      toLogFormat(error)
+    );
+  }
 
   return contactWithCleanedElements;
 };
@@ -41,15 +48,14 @@ function idForLogging(message) {
   return `${message.source}.${message.sourceDevice} ${message.sent_at}`;
 }
 
-function validateContact(contact, options = {}) {
+exports._validateContact = (contact, options = {}) => {
   const { messageId } = options;
   const { name, number, email, address, organization } = contact;
 
   if ((!name || !name.displayName) && !organization) {
-    console.log(
+    return new Error(
       `Message ${messageId}: Contact had neither 'displayName' nor 'organization'`
     );
-    return false;
   }
 
   if (
@@ -57,14 +63,13 @@ function validateContact(contact, options = {}) {
     (!email || !email.length) &&
     (!address || !address.length)
   ) {
-    console.log(
+    return new Error(
       `Message ${messageId}: Contact had no included numbers, email or addresses`
     );
-    return false;
   }
 
-  return true;
-}
+  return null;
+};
 
 function cleanBasicItem(item) {
   if (!item.value) {
diff --git a/test/modules/types/contact_test.js b/test/modules/types/contact_test.js
index c195e62e4..7451abb8f 100644
--- a/test/modules/types/contact_test.js
+++ b/test/modules/types/contact_test.js
@@ -7,9 +7,9 @@ const {
 } = require('../../../js/modules/string_to_array_buffer');
 
 describe('Contact', () => {
-  describe('parseAndWriteContactAvatar', () => {
-    const NUMBER = '+12025550099';
+  const NUMBER = '+12025550099';
 
+  describe('parseAndWriteContactAvatar', () => {
     it('handles message with no avatar in contact', async () => {
       const upgradeAttachment = sinon
         .stub()
@@ -246,49 +246,7 @@ describe('Contact', () => {
       assert.deepEqual(result, expected);
     });
 
-    it('logs if contact has no name.displayName or organization', async () => {
-      const upgradeAttachment = sinon
-        .stub()
-        .throws(new Error("Shouldn't be called"));
-      const upgradeVersion = Contact.parseAndWriteContactAvatar(
-        upgradeAttachment
-      );
-
-      const message = {
-        body: 'hey there!',
-        source: NUMBER,
-        sourceDevice: '1',
-        sent_at: 1232132,
-        contact: [
-          {
-            name: {
-              name: 'Someone',
-            },
-            number: [
-              {
-                type: 1,
-                value: NUMBER,
-              },
-            ],
-          },
-        ],
-      };
-      const expected = {
-        name: {
-          name: 'Someone',
-        },
-        number: [
-          {
-            type: 1,
-            value: NUMBER,
-          },
-        ],
-      };
-      const result = await upgradeVersion(message.contact[0], { message });
-      assert.deepEqual(result, expected);
-    });
-
-    it('removes invalid elements then logs if no values remain in contact', async () => {
+    it('removes invalid elements if no values remain in contact', async () => {
       const upgradeAttachment = sinon
         .stub()
         .throws(new Error("Shouldn't be called"));
@@ -353,4 +311,42 @@ describe('Contact', () => {
       assert.deepEqual(result, message.contact[0]);
     });
   });
+
+  describe('_validateContact', () => {
+    it('returns error if contact has no name.displayName or organization', () => {
+      const messageId = 'the-message-id';
+      const contact = {
+        name: {
+          name: 'Someone',
+        },
+        number: [
+          {
+            type: 1,
+            value: NUMBER,
+          },
+        ],
+      };
+      const expected =
+        "Message the-message-id: Contact had neither 'displayName' nor 'organization'";
+
+      const result = Contact._validateContact(contact, { messageId });
+      assert.deepEqual(result.message, expected);
+    });
+
+    it('logs if no values remain in contact', async () => {
+      const messageId = 'the-message-id';
+      const contact = {
+        name: {
+          displayName: 'Someone Somewhere',
+        },
+        number: [],
+        email: [],
+      };
+      const expected =
+        'Message the-message-id: Contact had no included numbers, email or addresses';
+
+      const result = Contact._validateContact(contact, { messageId });
+      assert.deepEqual(result.message, expected);
+    });
+  });
 });