Download Item Removal Confirmation #7072

michalbe · 2017-02-04T19:41:22Z

Test Plan

Download a file
try to delete it from the downloadsBar
notice new confirmation bar
- click ok to remove the file
- move the mouse outside the downloadItem panel to hide the confirmation bar.

Automated tests

npm run test -- --grep="downloadItem"

Description

Fixes #2604

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

How it works:

luixxiul · 2017-02-04T19:57:37Z

I like it :-D

michalbe · 2017-02-04T23:49:36Z

@luixxiul glad to hear that, thanks :)

bsclifton · 2017-02-05T06:36:17Z

@michalbe well done! 😄 This will be a great feature

Reviewing the code... would you be able to leave this component as an ImmutableComponent and instead have our AppStore be updated? Ideally, our controls would have state bound to them as a property (with binding happening in components/main.js or a child component). To trigger this state changing we would want to use an action (see appAction.js)

michalbe · 2017-02-05T13:54:28Z

@bsclifton Thanks, I implemented changes you've requested (even if I don't fully understand why global appState should be aware of the delete confirmation bar visibility, could you explain me this so I'll be smarter for the next time?).
I have a problem with tests - I'm probably doing something wrong with mocking the appState in the test suite - event fires as expected, app action fires as well (so I assume the event is dispatched), but it never gets to the reducer in appStore. Could you please check my miserable attempt in downloadItemTest.js and guide me in the right direction?

siemiatj · 2017-02-06T01:10:26Z

@michalbe I haven't yet read through the browser code but from my experience for instance some other bar/popup/modal may potentially be hidden by this ?

bsclifton · 2017-02-14T08:23:18Z

test/unit/app/renderer/downloadItemTest.js

+          <DownloadItem
+            downloadId={this.downloadId}
+            download={this.download}
+            deleteConfirmationVisible={this.download.get('deleteConfirmationVisible')} />


NOTE: this was wrong before. Before, it was this.download.downloadDeleteConfirmationVisible. Accessing child objects from an Immutable object will return undefined.

I updated this to use the Immutable.js get method to fetch the variable

Thanks! I never worked with ImmutableJS before, so most of my code in there was just a blind guess.

@michalbe no worries- it's something that bites me all the time (because it fails silently). Frankly, I wish an error would be thrown when it's accessed that way. I can't count the # times I've debugged an issue relating to that 😄

michalbe · 2017-02-14T08:38:42Z

test/unit/app/renderer/downloadItemTest.js

+      if ([downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED].includes(state)) {
+        describe('when delete button has been clicked', function () {
+          beforeEach(function () {
+            this.result.setProps({


THIS is the part I missed! Thanks!

michalbe · 2017-02-14T08:39:57Z

test/unit/app/renderer/downloadItemTest.js

+
+          testButton('.confirmDeleteButton', [downloadStates.CANCELLED, downloadStates.INTERRUPTED, downloadStates.COMPLETED], function (button) {
+            const spy = sinon.spy(appActions, 'downloadDeleted')
+            try {


Why there's a try block in here?

In case something fails. For example, with your original commit, it failed because 0 items were found. This raised an exception which then did not properly restore the sinon spy. When the next test case ran, it would try to put a spy on the same method but fail (because spy has already been done). The try is required only because I wanted to use a finally to restore the method which had been spied

bsclifton

@michalbe I believe this is all done and working great 😄 I had one last change, if you're able to do so:

When you click delete, you can visibly see the icons jump up about 2 or 3 pixels (your GIF shows this really well). Would you be able to tweak the CSS for this? If so, I think we're good to go here 😄

michalbe · 2017-02-14T21:12:07Z

@bsclifton fixed

bsclifton

@michalbe CSS changes look (and work) great!

While testing, I found a bug where moving the mouse immediately to another download item (after deleting) will show the confirmation on another item. You can then click OK and that one will be deleted too.

After OK is clicked (but before the download is deleted), I think we need to clear the appState

@michalbe

Fixes brave#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

michalbe · 2017-02-15T12:13:44Z

@bsclifton fixed and conflict resolved

bsclifton

Works great! Awesome job, @michalbe 😄

michalbe force-pushed the issue-2604-download-confirmation branch 2 times, most recently from 36f7cd8 to 429d766 Compare February 5, 2017 00:10

luixxiul added the feature/download label Feb 5, 2017

bsclifton added this to the 0.13.3 milestone Feb 5, 2017

michalbe force-pushed the issue-2604-download-confirmation branch 2 times, most recently from 4e71f1d to 2b60d47 Compare February 5, 2017 13:50

bsclifton assigned michalbe Feb 13, 2017

bsclifton force-pushed the issue-2604-download-confirmation branch from 2b60d47 to 685e61d Compare February 14, 2017 08:20

bsclifton reviewed Feb 14, 2017

View reviewed changes

michalbe commented Feb 14, 2017

View reviewed changes

bsclifton suggested changes Feb 14, 2017

View reviewed changes

bsclifton added the PR/needs-QA-attention ☕ label Feb 14, 2017

michalbe force-pushed the issue-2604-download-confirmation branch from 685e61d to eec85c8 Compare February 14, 2017 21:11

bsclifton force-pushed the issue-2604-download-confirmation branch from eec85c8 to 6111569 Compare February 15, 2017 06:43

bsclifton suggested changes Feb 15, 2017

View reviewed changes

michalbe force-pushed the issue-2604-download-confirmation branch from 6111569 to 97f96bd Compare February 15, 2017 12:10

michalbe force-pushed the issue-2604-download-confirmation branch from 97f96bd to c5026e5 Compare February 15, 2017 12:13

bsclifton approved these changes Feb 16, 2017

View reviewed changes

bsclifton added QA/test-plan-specified release-notes/include and removed PR/needs-QA-attention ☕ labels Feb 16, 2017

bsclifton added the parity Features which should be supported in Brave since they're supported in other major browsers. label Feb 16, 2017

bsclifton merged commit 813d695 into brave:master Feb 16, 2017

alexwykoff mentioned this pull request Feb 26, 2017

Manual test template for 0.13.5 #7389

Closed

srirambv added the QA/checked-Win64 label Feb 27, 2017

cndouglas mentioned this pull request Mar 24, 2017

Tweak download usability problems #1202

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download Item Removal Confirmation #7072

Download Item Removal Confirmation #7072

michalbe commented Feb 4, 2017 •

edited by bsclifton

Loading

luixxiul commented Feb 4, 2017

michalbe commented Feb 4, 2017

bsclifton commented Feb 5, 2017 •

edited

Loading

michalbe commented Feb 5, 2017 •

edited

Loading

siemiatj commented Feb 6, 2017

bsclifton Feb 14, 2017

michalbe Feb 14, 2017

bsclifton Feb 14, 2017

michalbe Feb 14, 2017

michalbe Feb 14, 2017

bsclifton Feb 14, 2017 •

edited

Loading

bsclifton left a comment

michalbe commented Feb 14, 2017

bsclifton left a comment •

edited

Loading

michalbe commented Feb 15, 2017

bsclifton left a comment

Download Item Removal Confirmation #7072

Download Item Removal Confirmation #7072

Conversation

michalbe commented Feb 4, 2017 • edited by bsclifton Loading

Test Plan

Automated tests

Description

luixxiul commented Feb 4, 2017

michalbe commented Feb 4, 2017

bsclifton commented Feb 5, 2017 • edited Loading

michalbe commented Feb 5, 2017 • edited Loading

siemiatj commented Feb 6, 2017

bsclifton Feb 14, 2017

Choose a reason for hiding this comment

michalbe Feb 14, 2017

Choose a reason for hiding this comment

bsclifton Feb 14, 2017

Choose a reason for hiding this comment

michalbe Feb 14, 2017

Choose a reason for hiding this comment

michalbe Feb 14, 2017

Choose a reason for hiding this comment

bsclifton Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

michalbe commented Feb 14, 2017

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

michalbe commented Feb 15, 2017

bsclifton left a comment

Choose a reason for hiding this comment

michalbe commented Feb 4, 2017 •

edited by bsclifton

Loading

bsclifton commented Feb 5, 2017 •

edited

Loading

michalbe commented Feb 5, 2017 •

edited

Loading

bsclifton Feb 14, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading