872543 - Long-form data reporting notification sticks around after visiting settings

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Android, defect, P2)

Description

[Richard Newman [:rnewman]]

HTC One, 4.1.something, HTC Sense.

Two-finger swiping on the notification expands it, which is awesome.

You now see a gear icon and "Choose what I share".

Tapping that action takes you to settings. It does not dismiss the notification.

Tapping the body of the notification takes you to settings and dismisses the notification.

I thing the former should also dismiss the notification. I was quite surprised to see it still there.

Comment 2

[Chenxia Liu [:liuche]]

Seems like setAutoCancel() doesn't work as expected on the big notification action. This probably needs an explicit clearing of the notification when click "action" is called.

Comment 3

[Richard Newman [:rnewman]]

Seems like this'll be a quick fix...

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Patch: Explicitly cancel notification on preferences launch.

This patch explicitly cancels the notification when the notification Action has been selected.

This also uses the correct (white) icon in the BigNotification.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 752369 [details] [diff] [review]
Patch: Explicitly cancel notification on preferences launch.

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

Looks good.

Another option to consider: always attempt to cancel the notification when the user accesses data reporting preferences. Your call.

Please land, bake, then flag for uplift.

Comment 6

[Chenxia Liu [:liuche]]

I considered that, but adding a single extra key into the bundle is pretty trivial and a one-time-call, and I don't like the idea of sending a notification cancel every single time a settings pane is rendered.

Waiting for inbound to re-open...

Comment 7

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b890e8b931d

Will bake on Nightly and if no regressions, will submit an aurora patch.

Comment 8

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/9c7c0b3f8c64 for "java-exception | java.lang.NullPointerExceptionat org.mozilla.gecko.GeckoPreferences.onCreate(GeckoPreferences.java:100)" in robocop like https://tbpl.mozilla.org/php/getParsedLog.php?id=23283474&tree=Mozilla-Inbound

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Patch: Explicitly cancel notification on preferences launch v2

Left out a null check for intent extras! Added that in, waiting on Try tests.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 753009 [details] [diff] [review]
Patch: Explicitly cancel notification on preferences launch v2

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

::: mobile/android/base/DataReportingNotification.java
@@ +56,5 @@
>                  Bundle fragmentArgs = new Bundle();
>                  fragmentArgs.putString("resource", "preferences_datareporting");
>                  prefIntent.putExtra(PreferenceActivity.EXTRA_SHOW_FRAGMENT_ARGUMENTS, fragmentArgs);
>              }
> +            prefIntent.putExtra(ALERT_NAME_DATAREPORTING_NOTIFICATION, ALERT_NAME_DATAREPORTING_NOTIFICATION.hashCode());

This is weird: you include the notificationId in the extras, but when you actually delete the notification, you ignore it.  Either be consistent and cancel the passed in ID, or change the meaning of the extra to be "if this key is present, cancel the one unique notification ID".  I think I prefer the latter, since it avoids having to parse the extras bundle.

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Patch: Explicitly cancel notification on preferences launch v3

Changed the value from an id to a boolean, but kept the keyname reuse. I kind of want to create another field like IS_DATAREPORTING_NOTIFICATION, because it'd technically be more correct and descriptive, but it would add yet another datareporting-specific single use field.

Comment 12

[Chenxia Liu [:liuche]]

All green on try with android unit-tests with the null check added.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2178f76d9a

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ae2178f76d9a

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: Clear notification explicitly on tap

This has been baking on Nightly for a couple of weeks without problems. Testing on try server against current aurora, then will nom for aurora uplift.

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 758120 [details] [diff] [review]
Aurora patch: Clear notification explicitly on tap

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Follow-up to bug 833625
User impact if declined: Users of Android 4.1+ may see Android system notification for Firefox data collection persist even after attempting to make selections.
Testing completed (on m-c, etc.): This has baked in Nightly since 5/23; passes all tests on try: https://tbpl.mozilla.org/?tree=Try&rev=85898e84bc8b and runs correctly
Risk to taking this patch (and alternatives if risky): low - added a single additional conditional check for a new flag added in this patch.
String or IDL/UUID changes made by this patch: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4cb244c7ac7