From c5026e58fff408a5bdf831d6c32fad39531260ef Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20Budzy=C5=84ski?= <michal@virtualdesign.pl>
Date: Sat, 4 Feb 2017 15:37:31 +0100
Subject: [PATCH] Download Item Removal Confirmation Fixes #2604

Updated test to handle confirm click
- rename downloadDeleteConfirmationVisible to deleteConfirmationVisible
- wrap spy code with try/finally to ensure proper cleanup
- confirmDeleteButton is checked independently from deleteButton

Auditors: @michalbe
---
 app/renderer/components/downloadItem.js    | 18 +++++++-
 app/renderer/components/downloadsBar.js    |  1 +
 docs/appActions.md                         | 12 ++++++
 js/actions/appActions.js                   | 18 ++++++++
 js/components/main.js                      |  1 +
 js/constants/appConstants.js               |  2 +
 js/stores/appStore.js                      |  6 +++
 less/downloadBar.less                      | 29 +++++++++++--
 test/unit/app/renderer/downloadItemTest.js | 50 +++++++++++++++++++---
 9 files changed, 126 insertions(+), 11 deletions(-)

diff --git a/app/renderer/components/downloadItem.js b/app/renderer/components/downloadItem.js
index 9d3e948bca9..0c6f179aebc 100644
--- a/app/renderer/components/downloadItem.js
+++ b/app/renderer/components/downloadItem.js
@@ -21,6 +21,8 @@ class DownloadItem extends ImmutableComponent {
     this.onResumeDownload = this.onDownloadActionPerformed.bind(this, RESUME)
     this.onCancelDownload = this.onDownloadActionPerformed.bind(this, CANCEL)
     this.onClearDownload = this.onClearDownload.bind(this)
+    this.onShowDeleteConfirmation = this.onShowDeleteConfirmation.bind(this)
+    this.onHideDeleteConfirmation = this.onHideDeleteConfirmation.bind(this)
     this.onDeleteDownload = this.onDeleteDownload.bind(this)
     this.onRedownload = this.onRedownload.bind(this)
     this.onCopyLinkToClipboard = this.onCopyLinkToClipboard.bind(this)
@@ -34,7 +36,14 @@ class DownloadItem extends ImmutableComponent {
   onClearDownload () {
     appActions.downloadCleared(this.props.downloadId)
   }
+  onShowDeleteConfirmation () {
+    appActions.showDownloadDeleteConfirmation()
+  }
+  onHideDeleteConfirmation () {
+    appActions.hideDownloadDeleteConfirmation()
+  }
   onDeleteDownload () {
+    appActions.hideDownloadDeleteConfirmation()
     appActions.downloadDeleted(this.props.downloadId)
   }
   onDownloadActionPerformed (downloadAction) {
@@ -74,10 +83,17 @@ class DownloadItem extends ImmutableComponent {
     return <span
       onContextMenu={contextMenus.onDownloadsToolbarContextMenu.bind(null, this.props.downloadId, this.props.download)}
       onDoubleClick={this.onOpenDownload}
+      onMouseLeave={this.onHideDeleteConfirmation}
       className={cx({
         downloadItem: true,
+        deleteConfirmationVisible: this.props.deleteConfirmationVisible,
         [this.props.download.get('state')]: true
       })}>
+      {
+        this.props.deleteConfirmationVisible
+        ? <div className='deleteConfirmation'>Delete?<Button l10nId='ok' className='primaryButton confirmDeleteButton' onClick={this.onDeleteDownload} /></div>
+        : null
+      }
       <div className='downloadActions'>
         {
           downloadUtil.shouldAllowPause(this.props.download)
@@ -111,7 +127,7 @@ class DownloadItem extends ImmutableComponent {
         }
         {
           downloadUtil.shouldAllowDelete(this.props.download)
-          ? <Button className='deleteButton' l10nId='downloadDelete' iconClass='fa-trash-o' onClick={this.onDeleteDownload} />
+          ? <Button className='deleteButton' l10nId='downloadDelete' iconClass='fa-trash-o' onClick={this.onShowDeleteConfirmation} />
           : null
         }
         {
diff --git a/app/renderer/components/downloadsBar.js b/app/renderer/components/downloadsBar.js
index dec90be7c28..5ed2adbcc14 100644
--- a/app/renderer/components/downloadsBar.js
+++ b/app/renderer/components/downloadsBar.js
@@ -37,6 +37,7 @@ class DownloadsBar extends ImmutableComponent {
             .map((download, downloadId) =>
               <DownloadItem download={download}
                 windowWidth={this.props.windowWidth}
+                deleteConfirmationVisible={this.props.deleteConfirmationVisible}
                 downloadId={downloadId}
                 downloadsSize={this.props.downloads.size} />)
         }
diff --git a/docs/appActions.md b/docs/appActions.md
index ab09972995b..cc1a6160772 100644
--- a/docs/appActions.md
+++ b/docs/appActions.md
@@ -628,6 +628,18 @@ Dispatches a message when a download is being redownloaded
 
 
 
+### showDownloadDeleteConfirmation() 
+
+Shows delete confirmation bar in download item panel
+
+
+
+### hideDownloadDeleteConfirmation() 
+
+Hides delete confirmation bar in download item panel
+
+
+
 ### clipboardTextCopied(text) 
 
 Dispatches a message when text is updated to the clipboard
diff --git a/js/actions/appActions.js b/js/actions/appActions.js
index 32e2867c153..49f8baf8fad 100644
--- a/js/actions/appActions.js
+++ b/js/actions/appActions.js
@@ -775,6 +775,24 @@ const appActions = {
     })
   },
 
+  /**
+   * Shows delete confirmation bar in download item panel
+   */
+  showDownloadDeleteConfirmation: function () {
+    AppDispatcher.dispatch({
+      actionType: appConstants.APP_SHOW_DOWNLOAD_DELETE_CONFIRMATION
+    })
+  },
+
+  /**
+   * Hides delete confirmation bar in download item panel
+   */
+  hideDownloadDeleteConfirmation: function () {
+    AppDispatcher.dispatch({
+      actionType: appConstants.APP_HIDE_DOWNLOAD_DELETE_CONFIRMATION
+    })
+  },
+
   /**
    * Dispatches a message when text is updated to the clipboard
    * @param {string} text - clipboard text which is copied
diff --git a/js/components/main.js b/js/components/main.js
index 70614112809..3706beaefc6 100644
--- a/js/components/main.js
+++ b/js/components/main.js
@@ -1298,6 +1298,7 @@ class Main extends ImmutableComponent {
         this.props.windowState.getIn(['ui', 'downloadsToolbar', 'isVisible']) && this.props.appState.get('downloads') && this.props.appState.get('downloads').size > 0
         ? <DownloadsBar
           windowWidth={window.innerWidth}
+          deleteConfirmationVisible={this.props.appState.get('deleteConfirmationVisible')}
           downloads={this.props.appState.get('downloads')} />
         : null
       }
diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js
index 406da26d2c8..8902ca5e1c7 100644
--- a/js/constants/appConstants.js
+++ b/js/constants/appConstants.js
@@ -78,6 +78,8 @@ const appConstants = {
   APP_DOWNLOAD_DELETED: _,
   APP_DOWNLOAD_CLEARED: _,
   APP_DOWNLOAD_REDOWNLOADED: _,
+  APP_SHOW_DOWNLOAD_DELETE_CONFIRMATION: _,
+  APP_HIDE_DOWNLOAD_DELETE_CONFIRMATION: _,
   APP_ALLOW_FLASH_ONCE: _,
   APP_ALLOW_FLASH_ALWAYS: _,
   APP_FLASH_PERMISSION_REQUESTED: _,
diff --git a/js/stores/appStore.js b/js/stores/appStore.js
index 5a02410cee2..786276eda76 100644
--- a/js/stores/appStore.js
+++ b/js/stores/appStore.js
@@ -885,6 +885,12 @@ const handleAppAction = (action) => {
         appState = appState.setIn(['sync', 'seedQr'], action.seedQr)
       }
       break
+    case appConstants.APP_SHOW_DOWNLOAD_DELETE_CONFIRMATION:
+      appState = appState.set('deleteConfirmationVisible', true)
+      break
+    case appConstants.APP_HIDE_DOWNLOAD_DELETE_CONFIRMATION:
+      appState = appState.set('deleteConfirmationVisible', false)
+      break
     default:
   }
 
diff --git a/less/downloadBar.less b/less/downloadBar.less
index 7c410b31fde..3cb70ac35b3 100644
--- a/less/downloadBar.less
+++ b/less/downloadBar.less
@@ -45,7 +45,7 @@
 
       &:hover {
         height: 62px;
-        top: -24px;
+        top: -23px;
 
         .downloadInfo .downloadArrow {
           display: none;
@@ -60,6 +60,10 @@
         .downloadActions {
           display: none;
         }
+
+        .deleteConfirmation {
+          display: none;
+        }
       }
 
       &.paused {
@@ -75,6 +79,11 @@
         }
       }
 
+      &.deleteConfirmationVisible:hover {
+        height: 97px;
+        top: -58px;
+      }
+
       .downloadProgress {
         background-color: @highlightBlue;
         left: 0;
@@ -125,6 +134,22 @@
           }
         }
       }
+
+      .deleteConfirmation {
+        line-height: 2;
+        border-bottom: 1px solid #CCC;
+        padding: 5px 0;
+        margin-bottom: auto 0 10px 0;
+        font-size: 12px;
+
+        .confirmDeleteButton {
+          font-weight: normal;
+          padding: 1px;
+          min-width: 50px;
+          float: right;
+          margin-right: -5px;
+        }
+      }
     }
   }
 
@@ -150,5 +175,3 @@
     }
   }
 }
-
-
diff --git a/test/unit/app/renderer/downloadItemTest.js b/test/unit/app/renderer/downloadItemTest.js
index d2eaaf69e7e..c98d031ffc8 100644
--- a/test/unit/app/renderer/downloadItemTest.js
+++ b/test/unit/app/renderer/downloadItemTest.js
@@ -1,7 +1,7 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
-/* global describe, before, after, it */
+/* global describe, before, beforeEach, after, it */
 
 const mockery = require('mockery')
 const {mount} = require('enzyme')
@@ -25,6 +25,7 @@ const newDownload = (state) => Immutable.fromJS({
   url: downloadUrl,
   totalBytes: 104729,
   receivedBytes: 96931,
+  deleteConfirmationVisible: false,
   state
 })
 
@@ -39,6 +40,7 @@ describe('downloadItem component', function () {
     DownloadItem = require('../../../../app/renderer/components/downloadItem')
     appActions = require('../../../../js/actions/appActions')
   })
+
   after(function () {
     mockery.disable()
   })
@@ -48,13 +50,19 @@ describe('downloadItem component', function () {
       before(function () {
         this.downloadId = uuid.v4()
         this.download = newDownload(state)
-        this.result = mount(<DownloadItem downloadId={this.downloadId} download={newDownload(state)} />)
+        this.result = mount(
+          <DownloadItem
+            downloadId={this.downloadId}
+            download={this.download}
+            deleteConfirmationVisible={this.download.get('deleteConfirmationVisible')} />
+        )
       })
 
-      const shouldProgressBarExist = [downloadStates.IN_PROGRESS, downloadStates.PAUSED].includes(state)
       it('filename exists and matches download filename', function () {
         assert.equal(this.result.find('.downloadFilename').text(), this.download.get('filename'))
       })
+
+      const shouldProgressBarExist = [downloadStates.IN_PROGRESS, downloadStates.PAUSED].includes(state)
       it(shouldProgressBarExist ? 'progress bar should exist' : 'progress bar should not exist', function () {
         assert.equal(this.result.find('.downloadProgress').length, shouldProgressBarExist ? 1 : 0)
       })
@@ -114,11 +122,39 @@ describe('downloadItem component', function () {
       })
 
       testButton('.deleteButton', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
-        const spy = sinon.spy(appActions, 'downloadDeleted')
-        button.simulate('click')
-        assert(spy.withArgs(this.downloadId).calledOnce)
-        appActions.downloadDeleted.restore()
+        const spy = sinon.spy(appActions, 'showDownloadDeleteConfirmation')
+        try {
+          // Confirmation should NOT be visible by default
+          assert.equal(this.result.find('.confirmDeleteButton').length, 0)
+
+          // Clicking delete should show confirmation
+          button.simulate('click')
+          assert(spy.called)
+        } finally {
+          appActions.showDownloadDeleteConfirmation.restore()
+        }
       })
+
+      if ([downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED].includes(state)) {
+        describe('when delete button has been clicked', function () {
+          beforeEach(function () {
+            this.result.setProps({
+              deleteConfirmationVisible: true
+            })
+          })
+
+          testButton('.confirmDeleteButton', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
+            const spy = sinon.spy(appActions, 'downloadDeleted')
+            try {
+              // Accepting confirmation should delete the item
+              button.simulate('click')
+              assert(spy.withArgs(this.downloadId).calledOnce)
+            } finally {
+              appActions.downloadDeleted.restore()
+            }
+          })
+        })
+      }
     })
   })
 })