Use model package types instead of local duplicates#160
Conversation
| } | ||
|
|
||
| func (me *AndroidNotificationServer) SendNotification(msg *PushNotification) PushResponse { | ||
| func (me *AndroidNotificationServer) SendNotification(_ int, msg *model.PushNotification) PushResponse { |
There was a problem hiding this comment.
appVersion not on model.PushNotification -- passed explicitly instead.
|
|
||
| data := payload.NewPayload() | ||
| if msg.Badge == 0 && msg.Type == PushTypeClear && msg.AppVersion > 1 { | ||
| if msg.Badge == 0 && msg.Type == model.PushTypeClear && appVersion > 1 { |
There was a problem hiding this comment.
On the appVersion parameter, @enahum I noticed this was introduced in April 2022. Is there an opportunity to drop support for old appVersion, or do we want to maintain this indefinitely? I can follow up with a metrics change in a separate PR to give us visibility here if the answer isn't obvious.
There was a problem hiding this comment.
I think we can drop it, at this point after 4 years no one should be using v1 off the app. I'd say definitely drop it
| OverrideIconURL string `json:"override_icon_url"` | ||
| FromWebhook string `json:"from_webhook"` | ||
| Version string `json:"version"` | ||
| AppVersion int `json:"app_version,omitempty"` |
There was a problem hiding this comment.
This was the single field "missing", but also never part of the wire payload -- so just handling it discretely.
| appVersion := 1 | ||
| if index := strings.Index(msg.Platform, "-v"); index > -1 { | ||
| platform := msg.Platform | ||
| msg.Platform = platform[:index] | ||
| appVersionString := platform[index+2:] | ||
| version, e := strconv.Atoi(appVersionString) | ||
| if e == nil { | ||
| msg.AppVersion = version | ||
| appVersion = version |
There was a problem hiding this comment.
Same logic, but not setting it on the struct anymore.
lieut-data
force-pushed
the
use-model-types
branch
from
9ea19ee to
68a6f22
Compare
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. 📝 WalkthroughWalkthroughThis PR migrates push notification handling from locally defined types to the ChangesPush Notification Model Type Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/android_notification_server.go (1)
251-256:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancel retry context per iteration instead of deferring in the loop.
defer cancelRetryContext()(server/android_notification_server.go, lines 251-252) is executed only when the enclosing function returns, so timeout resources can stay alive across retries. CallcancelRetryContext()inside each retry iteration instead.Suggested fix
retryContext, cancelRetryContext := context.WithTimeout(generalContext, me.retryTimeout) - defer cancelRetryContext() _, err = me.client.Send(retryContext, fcmMsg) + cancelRetryContext() if me.metrics != nil { me.metrics.observerNotificationResponse(model.PushNotifyAndroid, time.Since(start).Seconds()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/android_notification_server.go` around lines 251 - 256, The defer cancelRetryContext() inside the retry loop keeps each retry's timeout alive until the enclosing function returns; replace the deferred call with an explicit cancelRetryContext() invocation at the end of each iteration (after me.client.Send and metrics.observerNotificationResponse) so the context created by context.WithTimeout(generalContext, me.retryTimeout) is cancelled per iteration; ensure retryContext and cancelRetryContext are still created each loop and that cancelRetryContext() is always called before continuing or breaking the loop.server/apple_notification_server.go (1)
381-386:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid deferring
cancelRetryContext()inside the retry loop
cancelRetryContext()is deferred on every retry iteration, so cancellations (and the underlying timer/resources forcontext.WithTimeout) won’t run until the enclosing function returns. Cancel the attempt immediately afterPushWithContextreturns.Suggested fix
retryContext, cancelRetryContext := context.WithTimeout(generalContext, me.retryTimeout) - defer cancelRetryContext() res, err = me.AppleClient.PushWithContext(retryContext, notification) + cancelRetryContext() if me.metrics != nil { me.metrics.observerNotificationResponse(model.PushNotifyApple, time.Since(start).Seconds()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/apple_notification_server.go` around lines 381 - 386, The retry loop is deferring cancelRetryContext() each iteration which delays cancelling the per-attempt timeout until the enclosing function returns; instead, remove the defer and call cancelRetryContext() immediately after the call to me.AppleClient.PushWithContext (and after recording metrics via me.metrics.observerNotificationResponse) so the attempt's context resources are released promptly; locate the retry loop that creates retryContext/cancelRetryContext and replace the defer with a direct cancelRetryContext() call after res, err = me.AppleClient.PushWithContext(...) and metrics handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/android_notification_server.go`:
- Around line 251-256: The defer cancelRetryContext() inside the retry loop
keeps each retry's timeout alive until the enclosing function returns; replace
the deferred call with an explicit cancelRetryContext() invocation at the end of
each iteration (after me.client.Send and metrics.observerNotificationResponse)
so the context created by context.WithTimeout(generalContext, me.retryTimeout)
is cancelled per iteration; ensure retryContext and cancelRetryContext are still
created each loop and that cancelRetryContext() is always called before
continuing or breaking the loop.
In `@server/apple_notification_server.go`:
- Around line 381-386: The retry loop is deferring cancelRetryContext() each
iteration which delays cancelling the per-attempt timeout until the enclosing
function returns; instead, remove the defer and call cancelRetryContext()
immediately after the call to me.AppleClient.PushWithContext (and after
recording metrics via me.metrics.observerNotificationResponse) so the attempt's
context resources are released promptly; locate the retry loop that creates
retryContext/cancelRetryContext and replace the defer with a direct
cancelRetryContext() call after res, err = me.AppleClient.PushWithContext(...)
and metrics handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3c0fb74-f6dc-4c47-bd36-afc8a13ded88
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
go.modserver/android_notification_server.goserver/apple_notification_server.goserver/apple_notification_test.goserver/metrics.goserver/metrics_test.goserver/push_notification.goserver/server.goserver/server_test.go
💤 Files with no reviewable changes (1)
- server/push_notification.go
enahum
left a comment
enahum
left a comment
There was a problem hiding this comment.
Going to approve, but as I said in the comment, perhaps a good opportunity to drop appVersion.
Yes, the server and mobile has it in the prefix, but that should not affect anything here other than parsing the Platform which will remain
|
|
||
| data := payload.NewPayload() | ||
| if msg.Badge == 0 && msg.Type == PushTypeClear && msg.AppVersion > 1 { | ||
| if msg.Badge == 0 && msg.Type == model.PushTypeClear && appVersion > 1 { |
There was a problem hiding this comment.
I think we can drop it, at this point after 4 years no one should be using v1 off the app. I'd say definitely drop it
2 participants
Summary
A followup to #158 that consolidates under a common
model.*structs and constants. Note that while some field names have changed, the wire names are identical between both.Ticket Link
Relates-to: https://mattermost.atlassian.net/browse/MM-68727