Pending drafts local notifications

develric commented 3 years ago

Not sure there is a real issue, but since I will not be able to look into it soon-ish, creating this so it's not lost 😄

Don't think it's urgent so adding it to GK project for now.

Expected behavior

The following is what I understood the feature should look like (so my interpretation of it 😄 )

In Notifications settings, the Notify me on pending drafts flag should deactivate/activate local AlarmManager notifications reminding the user about drafts they created on the app but they didn't promote to Published.

Actual behavior

With that flag on or off I did not get local notifications

Steps to reproduce the behavior

The relevant code is in PendingDraftsNotificationsUtils and NotificationsPendingDraftsReceiver. I reduced on purpose the NotificationsPendingDraftsReceiver.ONE_DAY variable to 60 * 1000 so to accelerate the tests. For completeness I'm not even sure we are using that Notify me on pending drafts anymore (but better to confirm it).

Create a draft post and save it
You can check the alarms are scheduled using this adb shell dumpsys alarm | grep -i "org.wordpress.android"
Wait for the first alarm trigger (60 seconds in my accelerated test)
Notification was not triggered for me

Tested on [OnePlus 7T], Android [10], WPAndroid [16.9 develop]

aerych commented 3 years ago

See also: p1615417475001800-slack-android pcaMk7-rI-p2

designsimply commented 3 years ago

Interesting! I don't feel strongly about the idea of getting notifications in the app about my own pending drafts (as seen in https://github.com/wordpress-mobile/WordPress-Android/pull/4835) and I don't know how many users would like to have that, however, I very much like the suggestion to make it so site admins and editors get notifications for pending drafts submitted contributors or other users.

From what I can see, broken notifications about your own pending drafts didn't get any user response and, in contrast, there have been requests from users who would like to see notifications from contributors pending review. See https://github.com/Automattic/wp-calypso/issues/8556.

designsimply commented 3 years ago

To clarify a bit, my vote would be to remove the current "Notify me on pending drafts" setting in the app in favor of waiting for https://github.com/Automattic/wp-calypso/issues/8556 to be updated and then deciding whether a separate toggle would even be needed in the app for that. It could just be a notification that you get automatically and I'm not sure whether a separate toggle would be needed to turn that specific type of notification on or off. Great discussion point though!

ashiagr commented 2 years ago

Noticed this log today related to this discussion. Seems like the pending draft 's post id is passed with intent as Long whereas the notification receiver class tries to get it as an Int, it fails to find it and uses default value 0. Notification is only displayed for a non-zero post id.

2022-03-09 17:44:42.704 I/WordPress-NOTIFS: entering Pending Drafts Receiver from alarm
2022-03-09 17:44:42.727 W/Bundle: Key postId expected Integer but value was a java.lang.Long.  The default value 0 was returned.
2022-03-09 17:44:42.731 W/Bundle: Attempt to cast generated internal exception:
    java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.Integer
        at android.os.BaseBundle.getInt(BaseBundle.java:1077)
        at android.content.Intent.getIntExtra(Intent.java:8455)
        at org.wordpress.android.ui.notifications.receivers.NotificationsPendingDraftsReceiver.onReceive(NotificationsPendingDraftsReceiver.java:75)
        at com.android.tools.profiler.support.energy.PendingIntentWrapper.wrapBroadcastReceive(PendingIntentWrapper.java:192)
        at android.app.ActivityThread.handleReceiver(ActivityThread.java:4308)
        at android.app.ActivityThread.access$1600(ActivityThread.java:247)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2064)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loopOnce(Looper.java:201)
        at android.os.Looper.loop(Looper.java:288)
        at android.app.ActivityThread.main(ActivityThread.java:7839)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
2022-03-09 17:44:42.744 I/WordPress-NOTIFS: notifications update job service > destroyed

ashiagr commented 2 years ago

👋 @osullivanchris

Pinging you to check if we should be showing these notifications considering that we now display Drafts Post Card on My Site Dashboard and https://github.com/wordpress-mobile/WordPress-Android/issues/14240#issuecomment-797088296.

Copying from https://github.com/wordpress-mobile/WordPress-Android/pull/5033:

As soon as you exit a post draft by pressing back without publishing it, +1 day, +1 week, +1 month reminders are created, with different messages randomly chosen as follows:

1 day:

You drafted “post title” yesterday. Don’t forget to publish it!
Did you know that “post title” is still a draft? Publish away!

1 week:

Your draft, “post title” awaits you — be sure to publish it!
“Post title” remains a draft. Remember to publish it!

1 month

Don’t leave it hanging! “Post title” is waiting to be published.

osullivanchris commented 2 years ago

@ashiagr hey thanks for the ping. Actually from my understanding, it sounds like the notification and the card work fine together. The notification appears as a regular notification in notifications tab, is that correct?

If so, they're getting a ping. And they're seeing their drafts. It seems to pair up nicely.

Let me know if you disagree or if I'm misunderstanding/missing something.

ashiagr commented 2 years ago

Your understanding is correct, @osullivanchris 🙂

I was just concerned about showing both:

prominent drafts card on my site dashboard
notifications to publish drafts

which might be too much as reminders for a user based on @designsimply's comment:

I don't feel strongly about the idea of getting notifications in the app about my own pending drafts (as seen in https://github.com/wordpress-mobile/WordPress-Android/pull/4835) and I don't know how many users would like to have that

But I also see that there's an option to opt-out from these notifications in the notification settings. So I think it should be fine to fix this issue and display these notifications.

osullivanchris commented 2 years ago

@ashiagr that comment is interesting. I don't have a way to know the value of this notification to the user. Presumably it was added to encourage more posts. Maybe taking it away would lead to less posting.

It does seem a little heavy handed to me to do this for every single post. The card is a more gentle nudge.

But I can't speak with confidence to taking it away. Its good that there is a user control at least.

antonis commented 4 months ago

I verified that the issue is still valid with an accelerated test following the steps in the description.

wordpress-mobile / WordPress-Android