FEATURE: Delay push notifications as we do with emails

[xfalcox]

@tgxworld What is your opinion here?

My problem is that while actively navigating Meta I get push notifications for replies I already read and replied.

In my opinion, mirroring our email delay would solve this problem cheaply.

I could also check if the target post of a notifications was already read, but that depends on the notification type and it's expensive to query post_timings.

[tgxworld]

Does this mean if a notification is created in Discourse, we might delay or not send out the push notification?

[xfalcox]

Does this mean if a notification is created in Discourse, we might delay or not send out the push notification?

The idea is that we delay it, and after the delay we check if the user wasn't on the site during the delay, which means he already saw the discourse notification and don't need a push one to go for the mobile device.

[tgxworld]

Ah icic. This is a good change to have but I kind of wished we had some way to centralise the logic of sending out "external" notifications like push and emails.

[xfalcox]

Ah icic. This is a good change to have but I kind of wished we had some way to centralise the logic of sending out "external" notifications like push and emails.

Yes, that would be cool, but also a larger change.

[tgxworld]

But I don’t think we should be using a site settings meant for email on push notifications. If the intention is to make push notifications behave like emails, we should centralised the logic and rename the site setting so that it applies to both.

…

On Mon, 28 Dec 2020 at 23:43, Rafael dos Santos Silva < ***@***.***> wrote: Ah icic. This is a good change to have but I kind of wished we had some way to centralise the logic of sending out "external" notifications like push and emails. Yes, that would be cool, but also a larger change. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11574 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBCQ7TK62NDUEYBSLQXCATSXCRR7ANCNFSM4VHO3ALA> .

[eviltrout]

I agree with @tgxworld - it is counter intuitive that the site setting refers to email when it's not.

[SamSaffron]

so sorry I am closing this, I think we got to do the bigger change here if we are going down this path.

Also we don't delay this one...

discourse/app/services/post_alerter.rb

Lines 487 to 508 in 626b846

	def push_notification(user, payload)
	return if user.do_not_disturb?
	if user.push_subscriptions.exists?
	Jobs.enqueue(:send_push_notification, user_id: user.id, payload: payload)
	end
	if SiteSetting.allow_user_api_key_scopes.split("|").include?("push") && SiteSetting.allowed_user_api_push_urls.present?
	clients = user.user_api_keys
	.joins(:scopes)
	.where("user_api_key_scopes.name IN ('push', 'notifications')")
	.where("push_url IS NOT NULL AND push_url <> ''")
	.where("position(push_url IN ?) > 0", SiteSetting.allowed_user_api_push_urls)
	.where("revoked_at IS NULL")
	.order(client_id: :asc)
	.pluck(:client_id, :push_url)
	if clients.length > 0
	Jobs.enqueue(:push_notification, clients: clients, payload: payload, user_id: user.id)
	end
	end
	end

We should be consistent here if we are making a change.

Maybe a push_notification_delay site setting. It would be a bit complex cause when it eventually pushed you have to double confirm the user did not already read it in the intervening time.

Probably worth a dev TODO so we can scope it out.