Responding to pull request review feedback

- messages.getQuoteObjectUrl: early return
- backup.js: explaining variables for long if statement
- types/messages.js: Log if thumbnail has neither data nor path
- sendmessage.js:
  - remove extraneous logging
  - fix indentation
  - upload attachments and thumbnails in parallel
- preload: don't load fs for tests, just fse
- _conversation.scss: split two selectors into two lines, 0px -> 0
- backup_test.js: use fse.existsSync and comment twoSlashes regex
- network_tests_view_test.js: Comment duplicate assignment to window.getSocketStatus

js/models/messages.js

@@ -214,11 +214,11 @@
     },
     getQuoteObjectUrl() {
       const thumbnail = this.quoteThumbnail;
-      if (thumbnail && thumbnail.objectUrl) {
-        return thumbnail.objectUrl;
+      if (!thumbnail || !thumbnail.objectUrl) {
+        return null;
       }
 
-      return null;
+      return thumbnail.objectUrl;
     },
     getQuoteContact() {
       const quote = this.get('quote');

js/modules/backup.js

@@ -1030,8 +1030,11 @@ async function importConversation(db, dir, options) {
       return false;
     }
 
-    if ((message.attachments && message.attachments.length) ||
-      (message.quote && message.quote.attachments && message.quote.attachments.length)) {
+    const hasAttachments = message.attachments && message.attachments.length;
+    const hasQuotedAttachments = message.quote && message.quote.attachments &&
+      message.quote.attachments.length > 0;
+
+    if (hasAttachments || hasQuotedAttachments) {
       const importMessage = async () => {
         const getName = attachmentsDir
           ? _getAnonymousAttachmentFileName

js/modules/types/message.js

@@ -274,6 +274,13 @@ exports.createAttachmentDataWriter = (writeExistingAttachmentData) => {
 
       // we want to be bulletproof to thumbnails without data
       if (!data || !path) {
+        console.log(
+          'Thumbnail had neither data nor path.',
+          'id:',
+          message.id,
+          'source:',
+          message.source
+        );
         return thumbnail;
       }
 

libtextsecure/sendmessage.js

@@ -97,11 +97,10 @@ Message.prototype = {
         if (this.quote) {
             var QuotedAttachment = textsecure.protobuf.DataMessage.Quote.QuotedAttachment;
             var Quote = textsecure.protobuf.DataMessage.Quote;
-            console.log(this.quote);
 
-            proto.quote      = new Quote();
+            proto.quote = new Quote();
+            var quote = proto.quote;
 
-            var quote         = proto.quote;
             quote.id          = this.quote.id;
             quote.author      = this.quote.author;
             quote.text        = this.quote.text;
@@ -289,9 +288,10 @@ MessageSender.prototype = {
 
     sendMessage: function(attrs) {
         var message = new Message(attrs);
-        return this.uploadAttachments(message).then(function() {
-            return this.uploadThumbnails(message);
-        }.bind(this)).then(function() {
+        return Promise.all([
+            this.uploadAttachments(message),
+            this.uploadThumbnails(message),
+        ]).then(function() {
             return new Promise(function(resolve, reject) {
                 this.sendMessageProto(
                     message.timestamp,

preload.js

@@ -209,7 +209,6 @@ require('./js/spell_check');
 if (window.config.environment === 'test') {
   /* eslint-disable global-require, import/no-extraneous-dependencies */
   window.test = {
-    fs: require('fs'),
     glob: require('glob'),
     fse: require('fs-extra'),
     tmp: require('tmp'),

stylesheets/_conversation.scss

@@ -869,12 +869,13 @@ span.status {
 
 // We only add margin if there's no 'sender' element beforehand, which is only possible
 //   on incoming messages, and only in groups (when we're not in a .private conversation).
-.outgoing .quoted-message, .private .incoming .quoted-message {
+.outgoing .quoted-message,
+.private .incoming .quoted-message {
   margin-top: $android-bubble-quote-padding - $android-bubble-padding-vertical;
 }
 
 .bottom-bar .quoted-message {
-  margin: 0px;
+  margin: 0;
 }
 
 // We need to use the wrapper because the conversation view calculates the height of all

test/backup_test.js

@@ -231,7 +231,6 @@ describe('Backup', () => {
     it('exports then imports to produce the same data we started with', async () => {
       const {
         attachmentsPath,
-        fs,
         fse,
         glob,
         path,
@@ -266,14 +265,16 @@ describe('Backup', () => {
         return _.omit(model, ['id']);
       }
 
+      // We want to know which paths have two slashes, since that tells us which files
+      //   in the attachment fan-out are files vs. directories.
+      const TWO_SLASHES = /[^/]*\/[^/]*\/[^/]*/;
       // On windows, attachmentsPath has a normal windows path format (\ separators), but
       //   glob returns only /. We normalize to / separators for our manipulations.
-      const twoSlashes = /[^/]*\/[^/]*\/[^/]*/;
       const normalizedBase = attachmentsPath.replace(/\\/g, '/');
       function removeDirs(dirs) {
         return _.filter(dirs, (fullDir) => {
           const dir = fullDir.replace(normalizedBase, '');
-          return twoSlashes.test(dir);
+          return TWO_SLASHES.test(dir);
         });
       }
 
@@ -433,7 +434,7 @@ describe('Backup', () => {
 
         console.log('Backup test: Ensure that messages.zip exists');
         const zipPath = path.join(backupDir, 'messages.zip');
-        const messageZipExists = fs.existsSync(zipPath);
+        const messageZipExists = fse.existsSync(zipPath);
         assert.strictEqual(true, messageZipExists);
 
         console.log('Backup test: Ensure that all attachments made it to backup dir');

test/views/network_status_view_test.js

@@ -14,6 +14,11 @@ describe('NetworkStatusView', function() {
 
         after(function() {
             window.getSocketStatus = oldGetSocketStatus;
+
+            // It turns out that continued calls to window.getSocketStatus happen
+            //   because we host NetworkStatusView in three mock interfaces, and the view
+            //   checks every N seconds. That results in infinite errors unless there is
+            //   something to call.
             window.getSocketStatus = function() { return WebSocket.OPEN; };
         });
         /* END stubbing globals */