826946 - Gaia delays install prompt and then redownloads update after downloading update for preloaded packaged app

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

If an app calls mozApps.getSelf(), uses the result to checkForUpdate(), then calls download() because updateAvailable, Gaia should notify the user that an update is available as soon as the download is complete; prompt the user to install the update; and update the app as soon as the user confirms the request.

Instead, once the download is complete, Gaia doesn't prompt the user to install it.  But a few dozen seconds later, Gaia's UpdateManager notices that an update is available and notifies the user "1 update available. Tap to download."

Tapping that notification then shows a screen that prompts the user to download the update with "1 Update Available | {APPNAME} | [Later] [Download]".  And pressing [Download] redownloads the update before installing it.


I'm encountering this while developing stub apps for preloaded third-party apps that check for and download an update and then quit themselves to allow the system to install it.

And the delay between the downloading of the update and its installation is too long for a user who obviously wants to use the app, since they just started the stub; it also wastes the user's bandwidth to download the update twice.

All of which makes the stub experience pretty bad.  And we want to ship some of these stubs on Basecamp devices, so requesting blocking-basecamp.


jsmith: I'm not sure who the right set of cc:es is for this bug.  Any thoughts?

Comment 1

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #0)
> If an app calls mozApps.getSelf(), uses the result to checkForUpdate(), then
> calls download() because updateAvailable, Gaia should notify the user that
> an update is available as soon as the download is complete; prompt the user
> to install the update; and update the app as soon as the user confirms the
> request.

Sounds like what you are describing is that if the stub in itself is doing the checks for the update, the Gaia UI update flow should be made use of. Why is this necessary for the stubs? The update manager in Gaia should already be doing these checks on a preset schedule or by a manual sync, right?

> 
> Instead, once the download is complete, Gaia doesn't prompt the user to
> install it.  But a few dozen seconds later, Gaia's UpdateManager notices
> that an update is available and notifies the user "1 update available. Tap
> to download."

I'm guessing what might have happened here was that the preset schedule found the update possibly.

> 
> Tapping that notification then shows a screen that prompts the user to
> download the update with "1 Update Available | {APPNAME} | [Later]
> [Download]".  And pressing [Download] redownloads the update before
> installing it.

Right, and that's the typical behavior.

> 
> 
> I'm encountering this while developing stub apps for preloaded third-party
> apps that check for and download an update and then quit themselves to allow
> the system to install it.

Why not let the system update manager handle this?

> 
> And the delay between the downloading of the update and its installation is
> too long for a user who obviously wants to use the app, since they just
> started the stub; it also wastes the user's bandwidth to download the update
> twice.

Hmm...that could potentially be because the app itself is trying to do the update & the system update manager is trying to do it as well. Wasted bandwidth yes, but it's at the control of the app developer for the 1st case. Still want more info on why the system updater isn't sufficient here.

> 
> All of which makes the stub experience pretty bad.  And we want to ship some
> of these stubs on Basecamp devices, so requesting blocking-basecamp.

Note - I don't think the Gaia triagers actually know a lot about the stub story. You might want to have Rick or someone from group talk to those drivers. 

> 
> 
> jsmith: I'm not sure who the right set of cc:es is for this bug.  Any
> thoughts?

Etienne & Julien would probably good be candidates to look at this.

Josh for UX.

The blocking factor here I think really is dependent on understanding why the system updater isn't sufficient to solve this problem. Can you clarify?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jason Smith [:jsmith] from comment #1)
> The blocking factor here I think really is dependent on understanding why
> the system updater isn't sufficient to solve this problem. Can you clarify?

If a user starts their phone for the first time, sees an app they want to use, and taps its icon, then it's a bad user experience for the app to say, "Sorry, I don't work yet.  Eventually your phone will offer you an update for me that will make me work.  Wait for it."

It's much preferable for the app to say: "Sorry, you need an update to use me, and I'm going to download and install it for you right away!"  And then do so.  Perhaps prompting the user to confirm that it's ok.

Ideally, we would never ship a version of an app that isn't ready to be used without an update.  But we're preparing to do just that, building stubs for third-party apps whose real versions aren't going to be ready in time.  This bug is about making that experience as good as it can be, consistent with the functionality that is available for doing that.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> (In reply to Jason Smith [:jsmith] from comment #1)
> > The blocking factor here I think really is dependent on understanding why
> > the system updater isn't sufficient to solve this problem. Can you clarify?
> 
> If a user starts their phone for the first time, sees an app they want to
> use, and taps its icon, then it's a bad user experience for the app to say,
> "Sorry, I don't work yet.  Eventually your phone will offer you an update
> for me that will make me work.  Wait for it."

Couldn't you get around that issue by having stubs be "hidden" apps initially? We have apps that are "hidden" from the user that exist in Gaia that can be updated (although these are core Gaia apps - FTE is a good example). After the update, you could see the app move from being "hidden" to be "revealed" for general usage.

> 
> It's much preferable for the app to say: "Sorry, you need an update to use
> me, and I'm going to download and install it for you right away!"  And then
> do so.  Perhaps prompting the user to confirm that it's ok.

The hidden factor might be something to consider.

> 
> Ideally, we would never ship a version of an app that isn't ready to be used
> without an update.  But we're preparing to do just that, building stubs for
> third-party apps whose real versions aren't going to be ready in time.  This
> bug is about making that experience as good as it can be, consistent with
> the functionality that is available for doing that.

I understand, but - given our strict criteria for blocking these days, I'm not sure this is showstopper. The UX certainly could be better, but a work-around exists in this flow, though less optimal - rely purely on the system updater. Given that there's a work-around, I don't think we can block.

Comment 4

[Etienne Segonzac (:etienne)]

Some additional informations:

* The delay in question (30 seconds) is here because we want to present the user with only 1 notification (with the total updates count). And the system always checks for updates on all apps (which leads to multiple ondownloadavailable callbacks in a short time frame).

* Currently we're waiting for the app to be put in the background before applying the update (if an update was downloaded for the app currently displayed). Again the rationale was to bug the user as little as possible and not display confirmation prompts to apply each app update.

* The gaia update manager only registers to the app.onprogress/ondownloadsuccess for downloads it started itself (no addEventListeners on the Webapps API and we want to avoid conflicts with the app install flow).


So the biggest issue here is probably that if an app .checkForUpdate() and .download() itself, we're not applying the update. If we did the UpdateManager would remove the update notification (and if the download was < 30sec the user wouldn't even see the notification because of the delay).

Julien what do you think? I can think of at least 2 ugly solutions, do you happen to have a nice one? :)

Comment 5

[Julien Wajsberg [:julienw]]

Some random thoughts :

1 - About the delay, we already have a bug about this (Bug 814055). We know that it's sometimes not appropriate.

2 - couldn't the application access the management object, to apply its own update ? (I don't know very well what the applications can do and what they can't do)

3 - can't we use hosted apps with appcache instead of packaged apps for the stubs ? (ugly solution for v1)

I think of the following things we could do :

a - Gaia should not show a notification if the download has already begun (because another app asks for the download).

b - Gaia could register app.onprogress/ondownloadsuccess as soon as we get the downloadavailable event. If we take care to the app.installState, this should not conflict with the app install flow.


I think this bug could be fixed with either [2], [3] or [b]

[a] is nice to have.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Some random thoughts :
> 
> 1 - About the delay, we already have a bug about this (Bug 814055). We know
> that it's sometimes not appropriate.
> 
> 2 - couldn't the application access the management object, to apply its own
> update ? (I don't know very well what the applications can do and what they
> can't do)

The mgmt object can be accessed by certified apps only for v1. So that wouldn't be possible.

> 
> 3 - can't we use hosted apps with appcache instead of packaged apps for the
> stubs ? (ugly solution for v1)

I don't think so? There are preloaded apps that will be packaged, so we need to support both preload types - hosted vs. packaged.

> 
> I think of the following things we could do :
> 
> a - Gaia should not show a notification if the download has already begun
> (because another app asks for the download).
> 
> b - Gaia could register app.onprogress/ondownloadsuccess as soon as we get
> the downloadavailable event. If we take care to the app.installState, this
> should not conflict with the app install flow.
> 
> 
> I think this bug could be fixed with either [2], [3] or [b]
> 
> [a] is nice to have.

Sounds like [b] might be the right direction to go.

Comment 7

[Jason Smith [:jsmith]]

Fixing [b] is the blocker. Fixing [a] is not.

Comment 8

[Jason Smith [:jsmith]]

Opps...didn't mean to renom. Disregard.

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Setting back to blocking plus.  Jason accidentally renomed it.

Comment 10

[Julien Wajsberg [:julienw]]

I steal it, Etienne's is ok :)

Comment 11

[Julien Wajsberg [:julienw]]

Myk, do you have your test app available somewhere ?

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> Myk, do you have your test app available somewhere ?

Yes, my test stub app is in my Gaia fork > the packaged-app-stub branch > the external-apps/{1a48a62b-89dc-b843-83de-a0388c559cd2}/ subdirectory.  Create a branch and merge my branch into it via something like the following commands:

git checkout -b mykmelez-packaged-app-stub master
git pull git://github.com/mykmelez/gaia.git packaged-app-stub

Then make Gaia, and you should have an app named "Packaged App" with a blank white icon on your homescreen (probably on the rightmost one).  When you start it and press its Update button, it tries to update itself to its newest version (currently 1.5) from marketplace-dev:

https://marketplace-dev.allizom.org/app/packaged-app/
https://marketplace-dev.allizom.org/app/c9d54cbe-d77c-4485-8ba8-49a846e8519e/manifest.webapp
https://marketplace-dev.allizom.org/downloads/file/182111/packaged-app-1.5.zip

You're more likely to succeed with a version of B2G that has the patches from bug 826935, bug 826948, and bug 826940.


It might also be possible to test by installing a previous version from Marketplace, although I haven't been able to figure out how to do that.  Here's a link to 1.4:

https://marketplace-dev.allizom.org/downloads/file/182109/packaged-app-1.4.zip

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: WIP

I'm still testing it but I get caught in the middle of other bugs.

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: debug patch that makes testing this easier

Comment 15

[Julien Wajsberg [:julienw]]

Also I created an app which should be able to autoupdate : http://everlong.org/mozilla/selfupdatingpackaged/

I didn't check with your app yet Myk, I thought that with 2 independant apps we might be able to better check if this is working.

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Important changes:
* we attach the various events at downloadavailable instead of when the user
  asks to download. And so we don't remove them at downloaderror.
* We display the notification at the first progress (like in install manager)
* We don't display the toaster if we're downloading already
* Now we reset downloadedBytes when the last download finishes
* we remove the available update as soon as the download finishes (before it was
  when it was applied)
* Now the AppUpdatable instance registers itself in update manager at startup.
  And if a download is available, it calls its own callback, which makes it more
  consistent.

Other fixes:
* the app install manager does not add its handlers on an installed app anymore

Comment 17

[Julien Wajsberg [:julienw]]

https://github.com/julienw/self-updating-packaged-app makes it easy to test this.

Comment 18

[Etienne Segonzac (:etienne)]

Comment on attachment 700307 [details] [diff] [review]
patch v1

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

Looks like I just nit picked, r+ with those addressed!

::: apps/system/js/update_manager.js
@@ +338,5 @@
>      this.render();
>    },
>  
> +  displayNotificationAndToaster: function um_displayNotificationAndToaster() {
> +    if (this.updatesQueue.length && ! this._downloading) {

oooohhh, hello mister "!", didn't see you there, I'd like you to meet "this._downloading", I think you two should get closer...

::: apps/system/test/unit/app_install_manager_test.js
@@ +708,5 @@
>              assert.isTrue(onprogressCalled);
>            });
>  
> +          test('on downloadsuccess > should display a confirmation',
> +            function() {

I like it better with the function() aligned with test (that's what you do elsewhere BTW)

@@ +846,5 @@
>              assert.ok(true);
>            });
>  
> +          test('on downloadsuccess > should display a confirmation',
> +            function() {

same :)

::: apps/system/test/unit/mock_updatable.js
@@ +5,5 @@
>  
>    this.mDownloadCalled = false;
>    this.mCancelCalled = false;
>    this.mUninitCalled = false;
> +  MockAppUpdatable.cnt++;

it's a mock stuff so we should prefix with "m" and while we're at it, mCount looks better :)

::: apps/system/test/unit/updatable_test.js
@@ +269,5 @@
> +      downloadAvailableSuite('ondownloadavailable', function() {
> +        mockApp.mTriggerDownloadAvailable();
> +      });
> +
> +      downloadAvailableSuite('app has a download available at init', function() {

I'd move this test to the main "init" suite (clearer and no coverage lost)

::: apps/system/test/unit/update_manager_test.js
@@ +288,5 @@
>          assert.equal(initialLength - 1, UpdateManager.updatesQueue.length);
>        });
>  
> +      test('should remove from the update queue even if no downloadavailable',
> +          function() {

wow getting out of hand, 4 spaces ?!

I know you just wanted to see if I was paying attention :)

Comment 19

[Julien Wajsberg [:julienw]]

https://github.com/mozilla-b2g/gaia/commit/de643c2c2be5045dc5df0a6232ef997173a3e9fb

Comment 20

[Julien Wajsberg [:julienw]]

Attachment: patch v2

landed patch v2 r=etienne