802600 - [Apps] Cancel App Download from Notification Center

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Ben Francis [:benfrancis]]

As a user, once I confirm an app's installation, if that app requires a download in order to complete installation, I want to be able to cancel that download once it is in progress, from the Notification Center progress indicator, so that I can control my phone's network activity and defer the full installation until a later point in time.

Comment 1

[Ben Francis [:benfrancis]]

This is requirement AppInstall-011 https://docs.google.com/a/tola.me.uk/spreadsheet/ccc?key=0AtuYwL8qXqZmdGQtNHJ6S2NxZXlmYVctRS1xbEh3V0E#gid=73

Comment 2

[Dietrich Ayala (:dietrich)]

Notification center functionality is nice, but not required.

Comment 3

[Dietrich Ayala (:dietrich)]

Josh and I spent some time discussing the various scenarios, and it's not clear that we can do without notification center part of the spec.

Also, remaining required feature work listed at http://j.mp/VWpueM is now blocking+ to ensure visibility and tracking, per drivers' decision.

Comment 4

[Alex Keybl [:akeybl]]

We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Ben,

do you know if this bug should use the same dialog than bug 802583?

Comment 6

[Ben Francis [:benfrancis]]

No this is the "cancel download" dialog as opposed to the "cancel install" dialog. See page 10 of https://www.dropbox.com/sh/b0kyykhzcfkpm8b/fdA0gFkf5U/Gaia_AppInstall_20121011.pdf

This confused me too at first!

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ben Francis [:benfrancis] from comment #6)
> No this is the "cancel download" dialog as opposed to the "cancel install"
> dialog. See page 10 of
> https://www.dropbox.com/sh/b0kyykhzcfkpm8b/fdA0gFkf5U/
> Gaia_AppInstall_20121011.pdf
> 
> This confused me too at first!

Thanks for the quick answer :)

Comment 8

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: patch

Comment 9

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: patch

also fixed some gjslint problems in various files.

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 683085 [details] [diff] [review]
patch

Review of attachment 683085 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/system/index.html
@@ +689,5 @@
> +          </menu>
> +        </section>
> +      </form>
> +
> +      <form id="updates-download-dialog" data-type="confirm" role="dialog" data-z-index-level="updates-download-dialog">

80 cols or not? :)

::: apps/system/js/app_install_manager.js
@@ +275,5 @@
> +    var currentNode = e.target;
> +    while (!currentNode.classList.contains('fake-notification') &&
> +        currentNode !== this.notifContainer) {
> +      currentNode = currentNode.parentNode;
> +    }

definitively need some |pointer-events:none| goodness!

https://developer.mozilla.org/en-US/docs/CSS/pointer-events

::: apps/system/style/app_install_manager/app_install_manager.css
@@ +1,3 @@
>  /* dialog */
> +/* FIXME we should be able to use .app-install-dialog however
> + * confirm.css is conflicting with our style.

isn't
|form[role="dialog"][data-type="confirm"].app-install-dialog| working?

::: apps/system/test/unit/app_install_manager_test.js
@@ +10,3 @@
>  
>  requireApp('system/js/app_install_manager.js');
>  

var mocksForAppInstallManager = [...]

@@ +55,4 @@
>        };
>      };
>  
> +    mocksHelper = new MocksHelper(

new MocksHelper(mocksForAppInstallManager)

@@ +377,5 @@
>        });
>      });
>    });
>  
> +  suite('duringInstall >', function() {

I'm not against the ">" but I'd like to stay consistent at least at the test file level.

@@ +587,5 @@
> +
> +      test('tapping the message should display the dialog', function() {
> +        fakeNotif.querySelector('.fake-notification .message').click();
> +        assert.isTrue(fakeDownloadCancelDialog.classList.contains('visible'));
> +      });

Maybe we should have a |pointer-event:none;| css rule on the message.

@@ +624,5 @@
> +        assert.isFalse(fakeDownloadCancelDialog.classList.contains('visible'));
> +        assert.ok(mockApp.mCancelCalled);
> +      });
> +
> +      test('accepting should hide the dialog but not call cancelDownload if app is uninstalled', function() {

this file needs linting :)

::: apps/system/test/unit/mock_applications.js
@@ +18,5 @@
> +  }
> +
> +  return {
> +    getByManifestURL: getByManifestURL,
> +    registerMockApp: registerMockApp,

how about prefixing the mock-specific methods with "m"?
that's the rationale behind "mTeardown()" btw.

@@ +19,5 @@
> +
> +  return {
> +    getByManifestURL: getByManifestURL,
> +    registerMockApp: registerMockApp,
> +    unregisterMockApp: unregisterMockApp,

same

::: apps/system/test/unit/mock_notification_screen.js
@@ +20,5 @@
>      this.wasMethodCalled = {};
> +    var self = this;
> +    this.mockMethods.forEach(function(methodName) {
> +      self[methodName].wasCalled = false;
> +    });

forEach(function(){}, this)

::: apps/system/test/unit/mocks_helper.js
@@ +4,5 @@
> +};
> +
> +MocksHelper.prototype = {
> +
> +  setup: function mh_setup() {

do we still need this?

@@ +8,5 @@
> +  setup: function mh_setup() {
> +  },
> +
> +  suiteSetup: function mh_suiteSetup() {
> +    var self = this;

not needed anymore

@@ +20,5 @@
> +      window[objName] = window[mockName];
> +    }, this);
> +  },
> +
> +  suiteTeardown: function mj_suiteTeardown() {

mj -> mh

::: apps/system/test/unit/notifications_test.js
@@ +47,4 @@
>      document.body.appendChild(fakeLockScreenContainer);
>      document.body.appendChild(fakeButton);
>  
> +    mocksHelper.setup();

so this does nothing right?

Comment 12

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Comment on attachment 683085 [details] [diff] [review]
patch

Review of attachment 683085 [details] [diff] [review]:
-----------------------------------------------------------------

a patch will follow soon

::: apps/system/js/app_install_manager.js
@@ +275,5 @@
> +    var currentNode = e.target;
> +    while (!currentNode.classList.contains('fake-notification') &&
> +        currentNode !== this.notifContainer) {
> +      currentNode = currentNode.parentNode;
> +    }

ah nice, seems better ;)

::: apps/system/style/app_install_manager/app_install_manager.css
@@ +1,3 @@
>  /* dialog */
> +/* FIXME we should be able to use .app-install-dialog however
> + * confirm.css is conflicting with our style.

is that really better ?

that's specificity war and we should be able to do better. Have to see what is really different here from what's in confirm.css.

::: apps/system/test/unit/app_install_manager_test.js
@@ +10,3 @@
>  
>  requireApp('system/js/app_install_manager.js');
>  

done (and also in notifications_test.js)

@@ +55,4 @@
>        };
>      };
>  
> +    mocksHelper = new MocksHelper(

done

@@ +377,5 @@
>        });
>      });
>    });
>  
> +  suite('duringInstall >', function() {

done

@@ +587,5 @@
> +
> +      test('tapping the message should display the dialog', function() {
> +        fakeNotif.querySelector('.fake-notification .message').click();
> +        assert.isTrue(fakeDownloadCancelDialog.classList.contains('visible'));
> +      });

I added the CSS rule and removed this test. Do you have an idea of how we could test that this works ? in integration tests ?

@@ +624,5 @@
> +        assert.isFalse(fakeDownloadCancelDialog.classList.contains('visible'));
> +        assert.ok(mockApp.mCancelCalled);
> +      });
> +
> +      test('accepting should hide the dialog but not call cancelDownload if app is uninstalled', function() {

done

::: apps/system/test/unit/mock_applications.js
@@ +18,5 @@
> +  }
> +
> +  return {
> +    getByManifestURL: getByManifestURL,
> +    registerMockApp: registerMockApp,

yes good idea.

::: apps/system/test/unit/mocks_helper.js
@@ +4,5 @@
> +};
> +
> +MocksHelper.prototype = {
> +
> +  setup: function mh_setup() {

I removed "init" but I think "setup" is nice to have so that tests can use it now and we may painlessly use it later if necessary.

@@ +8,5 @@
> +  setup: function mh_setup() {
> +  },
> +
> +  suiteSetup: function mh_suiteSetup() {
> +    var self = this;

right

@@ +20,5 @@
> +      window[objName] = window[mockName];
> +    }, this);
> +  },
> +
> +  suiteTeardown: function mj_suiteTeardown() {

done

::: apps/system/test/unit/notifications_test.js
@@ +47,4 @@
>      document.body.appendChild(fakeLockScreenContainer);
>      document.body.appendChild(fakeButton);
>  
> +    mocksHelper.setup();

yes.. but it sounds consistent to have a "handler" for each step of the test.

Comment 13

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: patch v2

see also the diff patch

Comment 14

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: diff to v1

Comment 15

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

PR is https://github.com/mozilla-b2g/gaia/pull/6506

Comment 16

[Etienne Segonzac (:etienne)]

Comment on attachment 683167 [details] [diff] [review]
patch v2

Review of attachment 683167 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the follow up!

Comment 17

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

Attachment: patch v3

slightly modified a comment from an oral discussion with etienne

keeping r=etienne from v2

Comment 18

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

merged https://github.com/mozilla-b2g/gaia/pull/6506

Comment 19

[Angela Hubenya]

When selecting "stop download", the application loading process stops. Verified as fixed. Using Unagi, build 20130103070201