[Gekidou] Add push notification check to ping #19610

larkox · 2022-02-22T09:56:04Z

Summary

Add push notification check to ping.

Changes in the API require doc changes.

Related PRs

Mobile (Gekidou): mattermost/mattermost-mobile#6002
Push Proxy: mattermost/mattermost-push-proxy#96

Ticket Link

None

Release Note

Ping endpoint now can receive a device ID, which will report whether the device is able to receive push notifications.

enahum · 2022-02-23T10:58:50Z

app/notification_push.go

@@ -390,6 +391,8 @@ func (a *App) sendToPushProxy(msg *model.PushNotification, session *model.Sessio
 		mlog.String("status", model.PushSendPrepare),
 	)

+	fmt.Printf("%s %s\n", msg.DeviceId, msg.Platform)


do we want to keep this for some reason? or was it there while you developed?

Forgot to remove. Was there to understand the(and test with a real) deviceID.

enahum · 2022-02-23T10:59:38Z

app/notification_push.go

@@ -568,6 +571,47 @@ func (a *App) BuildPushNotificationMessage(contentsConfig string, post *model.Po
 	return msg, nil
 }

+func (a *App) TestPushNotification(deviceID string) (canSend bool, err error) {


perhaps we should rename the function not to confuse it with unit tests? something like SendTestPushNotification?

enahum · 2022-02-23T11:10:20Z

api4/system.go

@@ -183,6 +183,17 @@ func getSystemPing(c *Context, w http.ResponseWriter, r *http.Request) {
 		w.Header().Set(filestoreStatusKey, s[filestoreStatusKey])
 	}

+	if deviceID := r.FormValue("device_id"); deviceID != "" {
+		canSend, err := c.App.TestPushNotification(deviceID)


Forgot to change it here?

Trusted too much in the "rename symbol".

agnivade

Lastly, let's also add a test please :)

agnivade · 2022-02-24T04:23:03Z

api4/system.go

+	if deviceID := r.FormValue("device_id"); deviceID != "" {
+		canSend, err := c.App.SendTestPushNotification(deviceID)
+		if err != nil {
+			s["CanReceiveNotifications"] = err.Error()


We don't do the same for other keys in the map. To remain consistent, I'd suggest to log a warning and return false instead.

The problem is that returning false will mean "You cannot receive push notifications from this server". And that is a value that will be stored in the client "forever". This error may be something temporal. That will translate into the 3 states possible in the client: Can receive, cannot receive or unknown. We could hide the error from the client, by sending directly unknown, but not sure if we want to give more information to the user.

About logging or not a warning, I have no problem in doing that.

We could hide the error from the client, by sending directly unknown,

I like it, let's do that 👍

agnivade · 2022-02-24T04:28:52Z

app/notification_push.go

@@ -568,6 +568,47 @@ func (a *App) BuildPushNotificationMessage(contentsConfig string, post *model.Po
 	return msg, nil
 }

+func (a *App) SendTestPushNotification(deviceID string) (canSend bool, err error) {
+	var msg *model.PushNotification = &model.PushNotification{


Nit: the more idiomatic way is msg := ...

app/notification_push.go

agnivade · 2022-02-24T04:37:37Z

Also, this needs changes to the mattermost-api-reference repo.

agnivade

This isn't quite what I was suggesting. There is still some repetition going on here. What I had in mind was to have a single method that can handle a nil session and avoid doing anything session related in that.

So far the session is only used in the Info log method and in the model.PushStatusRemove case. For the first one, we can have a slice of mlog.Fields and conditionally append the session field. And then for the second one, an if condition will suffice.

This makes a single method work with and without session. After this, we have 2 top-level methods - one having session in it's signature, the other doesn't, and passes a nil session to this underlying method.

Sorry for the double work here, but it's good to keep things clean in this notification flow.

enahum · 2022-03-01T16:05:24Z

@larkox no, this can be merged now.. no need to wait for the proxy

larkox · 2022-03-01T16:06:27Z

/update-branch

* Add push notification check to ping * Add test type and missing details * Address feedback * Fix function name * Address feedback Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

amyblais · 2022-04-21T18:06:24Z

/cherry-pick release-6.3

mattermod · 2022-04-21T18:06:25Z

Cherry pick is scheduled.

* Add push notification check to ping * Add test type and missing details * Address feedback * Fix function name * Address feedback Co-authored-by: Mattermod <mattermod@users.noreply.github.com> (cherry picked from commit 32e3c2f)

…erver-mattermost#19610-upstream-release-6.3

Automatic Merge

amyblais · 2022-05-13T12:51:45Z

/cherry-pick release-6.5

mattermod · 2022-05-13T12:51:46Z

Cherry pick is scheduled.

* Add push notification check to ping * Add test type and missing details * Address feedback * Fix function name * Address feedback Co-authored-by: Mattermod <mattermod@users.noreply.github.com> (cherry picked from commit 32e3c2f)

…erver-mattermost#19610-upstream-release-6.5

* Add push notification check to ping * Add test type and missing details * Address feedback * Fix function name * Address feedback Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

Add push notification check to ping

38a111c

larkox added the Docs/Needed Requires documentation label Feb 22, 2022

larkox requested a review from enahum February 22, 2022 09:56

mm-cloud-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 22, 2022

This was referenced Feb 23, 2022

Handle test notifications on iOS mattermost/mattermost-mobile#6001

Closed

Handle test notifications on iOS mattermost/mattermost-mobile#6002

Merged

Add test type and missing details

0632e38

larkox mentioned this pull request Feb 23, 2022

Handle test notifications mattermost/mattermost-push-proxy#96

Merged

enahum approved these changes Feb 23, 2022

View reviewed changes

Address feedback

b43ea2e

larkox marked this pull request as ready for review February 23, 2022 11:09

larkox requested review from agnivade and DSchalla February 23, 2022 11:09

larkox added 2: Dev Review Requires review by a developer 3: Security Review Review requested from Security Team labels Feb 23, 2022

enahum requested changes Feb 23, 2022

View reviewed changes

Fix function name

82d3369

larkox requested a review from enahum February 23, 2022 11:12

enahum approved these changes Feb 23, 2022

View reviewed changes

DSchalla requested review from jupenur and removed request for DSchalla February 23, 2022 15:08

jupenur approved these changes Feb 23, 2022

View reviewed changes

jupenur removed the 3: Security Review Review requested from Security Team label Feb 23, 2022

agnivade reviewed Feb 24, 2022

View reviewed changes

Address feedback

b1bf28f

larkox requested a review from agnivade February 24, 2022 11:37

larkox mentioned this pull request Feb 24, 2022

Update ping to add push notification test definition mattermost/mattermost-api-reference#678

Merged

agnivade reviewed Feb 24, 2022

View reviewed changes

larkox added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed Docs/Needed Requires documentation labels Mar 1, 2022

larkox removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Mar 1, 2022

Merge branch 'master' into AddDeviceIDToPing

06b4eb3

larkox added the EETests label Mar 2, 2022

mattermod removed the EETests label Mar 2, 2022

larkox merged commit 32e3c2f into mattermost:master Mar 2, 2022

amyblais added this to the v6.6.0 milestone Mar 2, 2022

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Mar 2, 2022

mattermost-build mentioned this pull request Apr 21, 2022

Automated cherry pick of #19610 #20046

Closed

mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Apr 21, 2022

mattermod added a commit to mattermost-build/mattermost that referenced this pull request Apr 21, 2022

Merge branch 'release-6.3' into automated-cherry-pick-of-mattermost-s…

3f46b4d

…erver-mattermost#19610-upstream-release-6.3

larkox mentioned this pull request Apr 22, 2022

Cherry pick of #19610 #20050

Merged

mattermod pushed a commit that referenced this pull request Apr 28, 2022

Cherry pick of #19610 (#20050)

79f614b

Automatic Merge

larkox mentioned this pull request May 10, 2022

Respect send push proxy notification configuration on test notifications #20063

Merged

mattermost-build mentioned this pull request May 13, 2022

Automated cherry pick of #19610 #20205

Closed

mattermod added a commit to mattermost-build/mattermost that referenced this pull request May 13, 2022

Merge branch 'release-6.5' into automated-cherry-pick-of-mattermost-s…

27c2710

…erver-mattermost#19610-upstream-release-6.5

mattermod added a commit to mattermost-build/mattermost that referenced this pull request May 18, 2022

Merge branch 'release-6.5' into automated-cherry-pick-of-mattermost-s…

8db30fa

…erver-mattermost#19610-upstream-release-6.5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Gekidou] Add push notification check to ping #19610

[Gekidou] Add push notification check to ping #19610

larkox commented Feb 22, 2022 •

edited

Loading

enahum Feb 23, 2022

larkox Feb 23, 2022

enahum Feb 23, 2022

enahum Feb 23, 2022

larkox Feb 23, 2022

agnivade left a comment

agnivade Feb 24, 2022

larkox Feb 24, 2022

agnivade Feb 24, 2022

agnivade Feb 24, 2022

agnivade commented Feb 24, 2022

agnivade left a comment

enahum commented Mar 1, 2022

larkox commented Mar 1, 2022

amyblais commented Apr 21, 2022

mattermod commented Apr 21, 2022

amyblais commented May 13, 2022

mattermod commented May 13, 2022

[Gekidou] Add push notification check to ping #19610

[Gekidou] Add push notification check to ping #19610

Conversation

larkox commented Feb 22, 2022 • edited Loading

Summary

Related PRs

Ticket Link

Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnivade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnivade commented Feb 24, 2022

agnivade left a comment

Choose a reason for hiding this comment

enahum commented Mar 1, 2022

larkox commented Mar 1, 2022

amyblais commented Apr 21, 2022

mattermod commented Apr 21, 2022

amyblais commented May 13, 2022

mattermod commented May 13, 2022

larkox commented Feb 22, 2022 •

edited

Loading