Fixes for receive contact (#2359)

Fixes for receive contact

_locales/en/messages.json

@@ -476,6 +476,18 @@
       "message": "mobile",
       "description": "Shown on contact detail screen as a label for aa phone or email"
     },
+    "email": {
+      "message": "email",
+      "description": "Generic label shown if contact email has custom type but no label"
+    },
+    "phone": {
+      "message": "phone",
+      "description": "Generic label shown if contact phone has custom type but no label"
+    },
+    "address": {
+      "message": "address",
+      "description": "Generic label shown if contact address has custom type but no label"
+    },
     "poBox": {
       "message": "PO Box",
       "description": "When rendering an address, used to provide context to a post office box"

js/models/messages.js

@@ -16,7 +16,7 @@
 
   window.Whisper = window.Whisper || {};
 
-  const { Message: TypedMessage } = Signal.Types;
+  const { Message: TypedMessage, Contact } = Signal.Types;
   const { deleteAttachmentData } = Signal.Migrations;
 
   window.Whisper.Message = Backbone.Model.extend({
@@ -162,6 +162,10 @@
         const conversation = this.getModelForKeyChange();
         return i18n('keychanged', conversation.getTitle());
       }
+      const contacts = this.get('contact');
+      if (contacts && contacts.length) {
+        return Contact.getName(contacts[0]);
+      }
 
       return '';
     },

js/modules/types/contact.js

@@ -2,6 +2,7 @@ const { omit, compact, map } = require('lodash');
 
 const { toLogFormat } = require('./errors');
 const { SignalService } = require('../../../ts/protobuf');
+const { parse: parsePhoneNumber } = require('../../../ts/types/PhoneNumber');
 
 const DEFAULT_PHONE_TYPE = SignalService.DataMessage.Contact.Phone.Type.HOME;
 const DEFAULT_EMAIL_TYPE = SignalService.DataMessage.Contact.Email.Type.HOME;
@@ -12,19 +13,23 @@ exports.parseAndWriteAvatar = upgradeAttachment => async (
   contact,
   context = {}
 ) => {
-  const { message } = context;
+  const { message, regionCode } = context;
   const { avatar } = contact;
+
+  // This is to ensure that an omit() call doesn't pull in prototype props/methods
+  const contactShallowCopy = Object.assign({}, contact);
+
   const contactWithUpdatedAvatar =
     avatar && avatar.avatar
-      ? Object.assign({}, contact, {
+      ? Object.assign({}, contactShallowCopy, {
           avatar: Object.assign({}, avatar, {
             avatar: await upgradeAttachment(avatar.avatar, context),
           }),
         })
-      : omit(contact, ['avatar']);
+      : omit(contactShallowCopy, ['avatar']);
 
   // eliminates empty numbers, emails, and addresses; adds type if not provided
-  const parsedContact = parseContact(contactWithUpdatedAvatar);
+  const parsedContact = parseContact(contactWithUpdatedAvatar, { regionCode });
 
   const error = exports._validate(parsedContact, {
     messageId: idForLogging(message),
@@ -39,12 +44,17 @@ exports.parseAndWriteAvatar = upgradeAttachment => async (
   return parsedContact;
 };
 
-function parseContact(contact) {
+function parseContact(contact, options = {}) {
+  const { regionCode } = options;
+
+  const boundParsePhone = phoneNumber =>
+    parsePhoneItem(phoneNumber, { regionCode });
+
   return Object.assign(
     {},
     omit(contact, ['avatar', 'number', 'email', 'address']),
     parseAvatar(contact.avatar),
-    createArrayKey('number', compact(map(contact.number, parsePhoneItem))),
+    createArrayKey('number', compact(map(contact.number, boundParsePhone))),
     createArrayKey('email', compact(map(contact.email, parseEmailItem))),
     createArrayKey('address', compact(map(contact.address, parseAddress)))
   );
@@ -77,13 +87,16 @@ exports._validate = (contact, options = {}) => {
   return null;
 };
 
-function parsePhoneItem(item) {
+function parsePhoneItem(item, options = {}) {
+  const { regionCode } = options;
+
   if (!item.value) {
     return null;
   }
 
   return Object.assign({}, item, {
     type: item.type || DEFAULT_PHONE_TYPE,
+    value: parsePhoneNumber(item.value, { regionCode }),
   });
 }
 

js/modules/types/message.js

@@ -255,10 +255,16 @@ const VERSIONS = [
 exports.CURRENT_SCHEMA_VERSION = VERSIONS.length - 1;
 
 // UpgradeStep
-exports.upgradeSchema = async (rawMessage, { writeNewAttachmentData } = {}) => {
+exports.upgradeSchema = async (
+  rawMessage,
+  { writeNewAttachmentData, getRegionCode } = {}
+) => {
   if (!isFunction(writeNewAttachmentData)) {
     throw new TypeError('`context.writeNewAttachmentData` is required');
   }
+  if (!isFunction(getRegionCode)) {
+    throw new TypeError('`context.getRegionCode` is required');
+  }
 
   let message = rawMessage;
   // eslint-disable-next-line no-restricted-syntax
@@ -266,7 +272,10 @@ exports.upgradeSchema = async (rawMessage, { writeNewAttachmentData } = {}) => {
     // We really do want this intra-loop await because this is a chained async action,
     //   each step dependent on the previous
     // eslint-disable-next-line no-await-in-loop
-    message = await currentVersion(message, { writeNewAttachmentData });
+    message = await currentVersion(message, {
+      writeNewAttachmentData,
+      regionCode: getRegionCode(),
+    });
   }
 
   return message;

js/signal.js

@@ -50,7 +50,7 @@ const { IdleDetector } = require('./modules/idle_detector');
 const MessageDataMigrator = require('./modules/messages_data_migrator');
 
 exports.setup = (options = {}) => {
-  const { Attachments, userDataPath } = options;
+  const { Attachments, userDataPath, getRegionCode } = options;
 
   const Components = {
     ContactDetail,
@@ -84,6 +84,7 @@ exports.setup = (options = {}) => {
     upgradeMessageSchema: message =>
       Message.upgradeSchema(message, {
         writeNewAttachmentData: Attachments.createWriterForNew(attachmentsPath),
+        getRegionCode,
       }),
     writeMessageAttachments: Message.createAttachmentDataWriter(
       Attachments.createWriterForExisting(attachmentsPath)

libtextsecure/message_receiver.js

@@ -1065,12 +1065,26 @@ MessageReceiver.prototype.extend({
       promises.push(this.handleAttachment(attachment));
     }
 
-    if (
-      decrypted.contact &&
-      decrypted.contact.avatar &&
-      decrypted.contact.avatar.avatar
-    ) {
-      promises.push(this.handleAttachment(decrypted.contact.avatar.avatar));
+    if (decrypted.contact && decrypted.contact.length) {
+      const contacts = decrypted.contact;
+
+      for (let i = 0, max = contacts.length; i < max; i += 1) {
+        const contact = contacts[i];
+        const { avatar } = contact;
+
+        if (avatar && avatar.avatar) {
+          // We don't want the failure of a thumbnail download to fail the handling of
+          //   this message entirely, like we do for full attachments.
+          promises.push(
+            this.handleAttachment(avatar.avatar).catch(error => {
+              console.log(
+                'Problem loading avatar for contact',
+                error && error.stack ? error.stack : error
+              );
+            })
+          );
+        }
+      }
     }
 
     if (decrypted.quote && decrypted.quote.id) {

preload.js

@@ -129,6 +129,7 @@ window.moment.locale(locale);
 window.Signal = Signal.setup({
   Attachments,
   userDataPath: app.getPath('userData'),
+  getRegionCode: () => window.storage.get('regionCode'),
 });
 
 // Pulling these in separately since they access filesystem, electron

test/modules/types/contact_test.js

@@ -36,6 +36,46 @@ describe('Contact', () => {
       assert.deepEqual(result, message.contact[0]);
     });
 
+    it('turns phone numbers to e164 format', async () => {
+      const upgradeAttachment = sinon
+        .stub()
+        .throws(new Error("Shouldn't be called"));
+      const upgradeVersion = Contact.parseAndWriteAvatar(upgradeAttachment);
+
+      const message = {
+        body: 'hey there!',
+        contact: [
+          {
+            name: {
+              displayName: 'Someone Somewhere',
+            },
+            number: [
+              {
+                type: 1,
+                value: '(202) 555-0099',
+              },
+            ],
+          },
+        ],
+      };
+      const expected = {
+        name: {
+          displayName: 'Someone Somewhere',
+        },
+        number: [
+          {
+            type: 1,
+            value: '+12025550099',
+          },
+        ],
+      };
+      const result = await upgradeVersion(message.contact[0], {
+        message,
+        regionCode: 'US',
+      });
+      assert.deepEqual(result, expected);
+    });
+
     it('removes contact avatar if it has no sub-avatar', async () => {
       const upgradeAttachment = sinon
         .stub()

test/modules/types/message_test.js

@@ -277,6 +277,7 @@ describe('Message', () => {
           assert.deepEqual(attachmentData, expectedAttachmentData);
           return 'abc/abcdefg';
         },
+        getRegionCode: () => 'US',
       };
       const actual = await Message.upgradeSchema(input, context);
       assert.deepEqual(actual, expected);

ts/components/conversation/ContactDetail.md

@@ -62,6 +62,45 @@ const contact = {
 />;
 ```
 
+### With missing custom labels
+
+```jsx
+const contact = {
+  avatar: {
+    avatar: {
+      path: util.gifObjectUrl,
+    },
+  },
+  name: {
+    displayName: 'Someone Somewhere',
+  },
+  number: [
+    {
+      value: '(202) 555-0000',
+      type: 4,
+    },
+  ],
+  email: [
+    {
+      value: 'someone2@somewhere.com',
+      type: 4,
+    },
+  ],
+  address: [
+    {
+      street: '10 Pike Place, Seattle WA',
+      type: 3,
+    },
+  ],
+};
+<ContactDetail
+  contact={contact}
+  hasSignalAccount={true}
+  i18n={util.i18n}
+  onSendMessage={() => console.log('onSendMessage')}
+/>;
+```
+
 ### With default avatar
 
 ```jsx

ts/components/conversation/ContactDetail.tsx

@@ -26,10 +26,10 @@ interface Props {
   onSendMessage: () => void;
 }
 
-function getLabelForContactMethod(method: Phone | Email, i18n: Localizer) {
+function getLabelForEmail(method: Email, i18n: Localizer): string {
   switch (method.type) {
     case ContactType.CUSTOM:
-      return method.label;
+      return method.label || i18n('email');
     case ContactType.HOME:
       return i18n('home');
     case ContactType.MOBILE:
@@ -37,36 +37,63 @@ function getLabelForContactMethod(method: Phone | Email, i18n: Localizer) {
     case ContactType.WORK:
       return i18n('work');
     default:
-      return missingCaseError(method.type);
+      throw missingCaseError(method.type);
   }
 }
 
-function getLabelForAddress(address: PostalAddress, i18n: Localizer) {
+function getLabelForPhone(method: Phone, i18n: Localizer): string {
+  switch (method.type) {
+    case ContactType.CUSTOM:
+      return method.label || i18n('phone');
+    case ContactType.HOME:
+      return i18n('home');
+    case ContactType.MOBILE:
+      return i18n('mobile');
+    case ContactType.WORK:
+      return i18n('work');
+    default:
+      throw missingCaseError(method.type);
+  }
+}
+
+function getLabelForAddress(address: PostalAddress, i18n: Localizer): string {
   switch (address.type) {
     case AddressType.CUSTOM:
-      return address.label;
+      return address.label || i18n('address');
     case AddressType.HOME:
       return i18n('home');
     case AddressType.WORK:
       return i18n('work');
     default:
-      return missingCaseError(address.type);
+      throw missingCaseError(address.type);
   }
 }
 
 export class ContactDetail extends React.Component<Props, {}> {
-  public renderAdditionalContact(
-    items: Array<Phone | Email> | undefined,
-    i18n: Localizer
-  ) {
+  public renderEmail(items: Array<Email> | undefined, i18n: Localizer) {
     if (!items || items.length === 0) {
       return;
     }
 
-    return items.map((item: Phone | Email) => {
+    return items.map((item: Email) => {
       return (
         <div key={item.value} className="additional-contact">
-          <div className="type">{getLabelForContactMethod(item, i18n)}</div>
+          <div className="type">{getLabelForEmail(item, i18n)}</div>
+          {item.value}
+        </div>
+      );
+    });
+  }
+
+  public renderPhone(items: Array<Phone> | undefined, i18n: Localizer) {
+    if (!items || items.length === 0) {
+      return;
+    }
+
+    return items.map((item: Phone) => {
+      return (
+        <div key={item.value} className="additional-contact">
+          <div className="type">{getLabelForPhone(item, i18n)}</div>
           {item.value}
         </div>
       );
@@ -136,8 +163,8 @@ export class ContactDetail extends React.Component<Props, {}> {
         {renderName(contact)}
         {renderContactShorthand(contact)}
         {renderSendMessage({ hasSignalAccount, i18n, onSendMessage })}
-        {this.renderAdditionalContact(contact.number, i18n)}
-        {this.renderAdditionalContact(contact.email, i18n)}
+        {this.renderPhone(contact.number, i18n)}
+        {this.renderEmail(contact.email, i18n)}
         {this.renderAddresses(contact.address, i18n)}
       </div>
     );

ts/components/conversation/EmbeddedContact.md

@@ -150,6 +150,44 @@ const View = Whisper.MessageView;
 </util.ConversationContext>;
 ```
 
+#### No displayName or organization
+
+```jsx
+const outgoing = new Whisper.Message({
+  type: 'outgoing',
+  sent_at: Date.now() - 18000000,
+  contact: [
+    {
+      name: {
+        givenName: 'Someone',
+      },
+      number: [
+        {
+          value: '+12025551000',
+          type: 1,
+        },
+      ],
+      avatar: {
+        avatar: {
+          path: util.gifObjectUrl,
+        },
+      },
+    },
+  ],
+});
+const incoming = new Whisper.Message(
+  Object.assign({}, outgoing.attributes, {
+    source: '+12025550011',
+    type: 'incoming',
+  })
+);
+const View = Whisper.MessageView;
+<util.ConversationContext theme={util.theme}>
+  <util.BackboneWrapper View={View} options={{ model: incoming }} />
+  <util.BackboneWrapper View={View} options={{ model: outgoing }} />
+</util.ConversationContext>;
+```
+
 #### Default avatar
 
 ```jsx

ts/styleguide/StyleGuideUtil.ts

@@ -137,6 +137,7 @@ const Attachments = {
 parent.Signal = Signal.setup({
   Attachments,
   userDataPath: '/',
+  getRegionCode: () => parent.storage.get('regionCode'),
 });
 parent.SignalService = SignalService;
 

ts/test/types/Contact_test.ts

@@ -0,0 +1,65 @@
+import 'mocha';
+import { assert } from 'chai';
+
+import { getName } from '../../types/Contact';
+
+describe('Contact', () => {
+  describe('getName', () => {
+    it('returns displayName if provided', () => {
+      const contact = {
+        name: {
+          displayName: 'displayName',
+          givenName: 'givenName',
+          familyName: 'familyName',
+        },
+        organization: 'Somewhere, Inc.',
+      };
+      const expected = 'displayName';
+      const actual = getName(contact);
+      assert.strictEqual(actual, expected);
+    });
+    it('returns organization if no displayName', () => {
+      const contact = {
+        name: {
+          givenName: 'givenName',
+          familyName: 'familyName',
+        },
+        organization: 'Somewhere, Inc.',
+      };
+      const expected = 'Somewhere, Inc.';
+      const actual = getName(contact);
+      assert.strictEqual(actual, expected);
+    });
+    it('returns givenName + familyName if no displayName or organization', () => {
+      const contact = {
+        name: {
+          givenName: 'givenName',
+          familyName: 'familyName',
+        },
+      };
+      const expected = 'givenName familyName';
+      const actual = getName(contact);
+      assert.strictEqual(actual, expected);
+    });
+    it('returns just givenName', () => {
+      const contact = {
+        name: {
+          givenName: 'givenName',
+        },
+      };
+      const expected = 'givenName';
+      const actual = getName(contact);
+      assert.strictEqual(actual, expected);
+    });
+    it('returns just familyName', () => {
+      const contact = {
+        name: {
+          familyName: 'familyName',
+        },
+      };
+      const expected = 'familyName';
+      const actual = getName(contact);
+      assert.strictEqual(actual, expected);
+    });
+  });
+});

ts/types/Contact.ts

@@ -1,9 +1,9 @@
 // @ts-ignore
 import Attachments from '../../app/attachments';
-import { formatPhoneNumber } from '../util/formatPhoneNumber';
+import { format as formatPhoneNumber } from '../types/PhoneNumber';
 
 export interface Contact {
-  name: Name;
+  name?: Name;
   number?: Array<Phone>;
   email?: Array<Email>;
   address?: Array<PostalAddress>;
@@ -17,7 +17,7 @@ interface Name {
   prefix?: string;
   suffix?: string;
   middleName?: string;
-  displayName: string;
+  displayName?: string;
 }
 
 export enum ContactType {
@@ -101,5 +101,11 @@ export function contactSelector(
 
 export function getName(contact: Contact): string | null {
   const { name, organization } = contact;
-  return (name && name.displayName) || organization || null;
+  const displayName = (name && name.displayName) || null;
+  const givenName = (name && name.givenName) || null;
+  const familyName = (name && name.familyName) || null;
+  const backupName =
+    (givenName && familyName && `${givenName} ${familyName}`) || null;
+
+  return displayName || organization || backupName || givenName || familyName;
 }

ts/types/PhoneNumber.ts

@@ -1,6 +1,6 @@
-import { instance, PhoneNumberFormat } from './libphonenumberInstance';
+import { instance, PhoneNumberFormat } from '../util/libphonenumberInstance';
 
-export function formatPhoneNumber(
+export function format(
   phoneNumber: string,
   options: {
     ourRegionCode: string;
@@ -20,3 +20,19 @@ export function formatPhoneNumber(
     return phoneNumber;
   }
 }
+
+export function parse(
+  phoneNumber: string,
+  options: {
+    regionCode: string;
+  }
+): string {
+  const { regionCode } = options;
+  const parsedNumber = instance.parse(phoneNumber, regionCode);
+
+  if (instance.isValidNumber(parsedNumber)) {
+    return instance.format(parsedNumber, PhoneNumberFormat.E164);
+  }
+
+  return phoneNumber;
+}