1016503 - [email] move sound preference into app, allowing email to be a privileged app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[James Burke [:jrburke]]

Right now the email app needs to be a certified app because it uses the "settings" permission to read the "play sound when email sent" setting.

By placing this preference inside the other account settings in the email app, this would allow the email app to be a privileged app.

This is one way to comply with the new CSP policy as discussed in bug 1012652.

It would also set the stage for allowing a version of the email app to be delivered in the marketplace. In addition to giving us the option to upgrade the email app for older FxOS versions, it removes a barrier for someone who might want to use the email codebase as a starting point for another marketplace-delivered email client.

For this to work with gaia deployments without triggering contacts permissions prompts, bug 1014410 should be addressed and landed first.

Comment 1

[James Burke [:jrburke]]

Attachment: email-sound.png

The attached image shows the first pass at moving the sound preference into the email app. The left side is the account prefs screen once an account has been set up. The right side is what is seen on initial account setup.

Here is a zip of the app if you want to try it:

http://jrburke.com/work/gaia/email-sound-1.zip

which can be installed on a device via these instructions:

https://github.com/jrburke/gaia-dev-zip#for-the-ux-person

I have a bug at the moment where the initial account setup version defaults to Off, but that is just a bug, it should be On by default, will be fixed before shipping.

Current work in this branch (diff view):

https://github.com/jrburke/gaia/compare/bug1016503-email-privileged

The branch also **removes** the sound pref from the Settings app.

Since we are reworking sending in 2.0, with the async outbox, this also seemed to be a good time to introduce this change as it deals with sending, and we need to pick a default for accounts that migrate from a previous version to this version.

Since we are removing the Settings pref, we cannot migrate their previous setting. Right now, the code is set up to default to the sound being On for accounts that migrate to a release with this change. For new accounts created with this version of the app, the default will also be On.

Asking for UX and Product feedback on the placement, name of the pref and for confirmation that the default behavior of On is the desired approach, even for migrated accounts. Ideally those questions could be resolved this week to give enough time for cleanup and landing and QA prep.

Once we have those details worked out, I will email dev-gaia to mention this change, the move to a privileged app status, and the change in the Settings area. I also likely need some copy editing done for the pref name as shown in the UI.

Comment 2

[James Burke [:jrburke]]

I forgot to mention: if installing the email app zip, that has no effect on the Settings app, so you will still see the sound pref if you look in the Settings app. However, once this changeset is merged with master gaia, it would go away.

Comment 3

[Juwei Huang[:juwei]]

Comment on attachment 8430565 [details]
email-sound.png

Looks fine with me!

Comment 4

[James Burke [:jrburke]]

Attachment: GELAM pull request

The GELAM changes to store and change the sound pref on the account. Just a straight copy of the pathways used for notifyOnNew, but for this new account pref, playSoundOnSend.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8432026 [details] [review]
GELAM pull request

r=asuth by inspection/highlight all.

Comment 6

[James Burke [:jrburke]]

Attachment: Gaia pull request

Gaia pull request. Includes the GELAM pull request changes in it, and surfacing the sound pref next to the one about notifying about new messages in the email app. Asking :asuth to review email changes.

Removes the pref from the Settings app. Asking :arthurcc to review the settings changes. Hopefully straightforward as it is a removal.

An email app zip of just the email changes is here:

http://jrburke.com/work/gaia/email-sound-2.zip

Note that this will not land until the l10n event change in bug 1013929 lands, but that change should be transparent to these changes.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8432030 [details] [review]
Gaia pull request

r=asuth by inspection with l10n quasi-nit addressed.

I think I might suggest that we break the actual flip of the manifest into a separate bug with a patch that does just that.  Specifically, I think it'd be nice that if it turns out something horrible happens that a backout of the change doesn't move the setting back into the settings UI.  Especially since that localization change may get backed out a few times on its own and we might want to wait to make sure it sticks and not delay this patch.

Thanks for aggressively pursuing this set of changes!

Comment 8

[James Burke [:jrburke]]

Addressed the feedback in the email parts of the pull request:

* whitespace
* localization comment
* changed the manifest back to "certified"

then I rebased and force pushed. 

I filed bug 1018534 to handle the specific manifest type change to "privileged" so that we can land these changes independently have them stick if any issues are found with just the privileged change.

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8432030 [details] [review]
Gaia pull request

Looks good to me, thanks!

Comment 10

[James Burke [:jrburke]]

Comment on attachment 8430565 [details]
email-sound.png

Got the OK from Wilfred in another channel, so clearing that flag. Will land this shortly, once tree is reopened.

Comment 11

[James Burke [:jrburke]]

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/04beefa2eb66b2977d9e5fbd778f5ad2a6b8c003

from pull request:
https://github.com/mozilla-b2g/gaia/pull/19846

Comment 12

[James Burke [:jrburke]]

And the GELAM change merged in master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/d3213027b9db96348fee3e2c5deb11ebf80fea94

from pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/308