Page MenuHomePhabricator
Differential D5210

[services] Tunnelbroker - Updating `Send` handler with using of the Rust notification library
ClosedPublic
Actions

Authored by • max on Sep 22 2022, 4:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 6:31 AM
Unknown Object (File)
Wed, Nov 27, 1:52 PM
Unknown Object (File)
Wed, Nov 27, 1:48 PM
Unknown Object (File)
Wed, Nov 27, 11:34 AM
Unknown Object (File)
Thu, Nov 21, 8:14 AM
Unknown Object (File)
Wed, Nov 20, 4:07 PM
Unknown Object (File)
Fri, Nov 8, 3:51 PM
Unknown Object (File)
Fri, Nov 8, 4:15 AM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
• jon
tomek
Commits
rCOMMa206a33acdc2: [services] Tunnelbroker - Updating `Send` handler with using of the Rust…
Summary

This diff introduces changes to the Send handler to use the Rust notification library to send the push notifications.
In case we have a token-related error returning from the APNs or FCM we will clear the device token in the database and request the client to provide a new one in D5206, D5207.

Related Linear task: ENG-1331

Test Plan
  • The service was successfully built.
  • To test APNs (iOS) messages delivery use the procedure described in ENG-1743
  • To test FCM (Android) messages delivery use the procedure described in ENG-1742

Diff Detail

Repository
rCOMM Comm
Branch
update-send-with-notifs
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Sep 22 2022, 4:59 AM
• max held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptSep 22 2022, 4:59 AM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript
• max added parent revisions: D5145: [services] Tunnelbroker - Update FCM library wrapper with return of token-related errors, D5149: [services] Tunnelbroker - Update A2 (APNs) library wrapper with return of token-related errors, D5179: [services] Tunnelbroker - Add `updateSessionItemDeviceToken` database method.Sep 22 2022, 5:00 AM
Harbormaster completed remote builds in B12368: Diff 16979.Sep 22 2022, 5:08 AM
• max published this revision for review.Sep 22 2022, 5:12 AM
• max edited the summary of this revision. (Show Details)
• max edited the test plan for this revision. (Show Details)
• max added a reviewer: tomek.
• max retitled this revision from [services] Tunnelbroker - Updating `Send` handler with using of the rust notification library to [services] Tunnelbroker - Updating `Send` handler with using of the Rust notification library.
• max mentioned this in D5149: [services] Tunnelbroker - Update A2 (APNs) library wrapper with return of token-related errors.Sep 22 2022, 5:21 AM
ashoat added inline comments.Sep 22 2022, 5:55 AM
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
191–194 ↗(On Diff #16979)

No point to these nested if statements, they can just be combined. The lower the indentation level, the more clear the code is (in my opinion)

• max marked an inline comment as done.Sep 22 2022, 6:02 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
191–194 ↗(On Diff #16979)

No point to these nested if statements, they can just be combined. The lower the indentation level, the more clear the code is (in my opinion)

I did it on purpose because we had the rule to split the long condition statement into a few to make it more readable. If it looks ok to merge it in case of reading the code I'll do it now ;)

ashoat added inline comments.Sep 22 2022, 6:08 AM
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
191–194 ↗(On Diff #16979)

Not familiar with the rule you're referencing

tomek requested changes to this revision.Sep 23 2022, 5:26 AM

Shouldn't we check if device is online?

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
166–200 ↗(On Diff #16979)

The code would be more readable if these were extracted into a function

181 ↗(On Diff #16979)

Maybe can't clear an invalid token, or something like that?

This revision now requires changes to proceed.Sep 23 2022, 5:26 AM
• max planned changes to this revision.Sep 23 2022, 5:54 AM
• max marked an inline comment as done.
• max updated this revision to Diff 17027.Sep 23 2022, 7:24 AM

Adding of checking for device is online.
Error message text was changed.

• max added a comment.Sep 23 2022, 7:24 AM
In D5210#153347, @tomek wrote:

Shouldn't we check if device is online?

Wooow, I've missed that! Thanks a lot @tomek !
Fixed.

• max marked 2 inline comments as done.Sep 23 2022, 7:25 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
166–200 ↗(On Diff #16979)

The code would be more readable if these were extracted into a function

Yes, I think so, I'll create a small follow-up for this.

181 ↗(On Diff #16979)

Maybe can't clear an invalid token, or something like that?

Makes sense, I've changed the text.

Harbormaster failed remote builds in B12408: Diff 17027!Sep 23 2022, 7:29 AM
Harbormaster completed remote builds in B12408: Diff 17027.Sep 23 2022, 7:34 AM
• max updated this revision to Diff 17030.Sep 23 2022, 8:11 AM
• max marked 2 inline comments as done.

Merging nested if's.

tomek accepted this revision.Sep 23 2022, 8:13 AM
tomek added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
164 ↗(On Diff #17027)

Early exit would decrease the indentation. Also we can check notifyToken.empty() here.

191–194 ↗(On Diff #16979)

Please merge the ifs as @ashoat suggested

This revision is now accepted and ready to land.Sep 23 2022, 8:13 AM
tomek added inline comments.Sep 23 2022, 8:14 AM
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
197 ↗(On Diff #17030)

This message should also be updated

• max marked an inline comment as done.Sep 23 2022, 8:15 AM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
191–194 ↗(On Diff #16979)

Please merge the ifs as @ashoat suggested

Already done, thanks @tomek !

Harbormaster failed remote builds in B12411: Diff 17030!Sep 23 2022, 8:16 AM
• max updated this revision to Diff 17056.Sep 23 2022, 12:25 PM
• max marked an inline comment as done.

Error message changed, rebased on the latest changes.

• max marked an inline comment as done.Sep 23 2022, 12:25 PM
• max added inline comments.
services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp
197 ↗(On Diff #17030)

This message should also be updated

Makes sense! Done.

Harbormaster failed remote builds in B12424: Diff 17056!Sep 23 2022, 12:26 PM
• max updated this revision to Diff 17060.Sep 23 2022, 12:39 PM
• max marked an inline comment as done.

Resolving merge conflict.

This revision was landed with ongoing or failed builds.Sep 23 2022, 12:46 PM
Closed by commit rCOMMa206a33acdc2: [services] Tunnelbroker - Updating `Send` handler with using of the Rust… (authored by • max). · Explain Why
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMMa206a33acdc2: [services] Tunnelbroker - Updating `Send` handler with using of the Rust….
Harbormaster completed remote builds in B12426: Diff 17060.Sep 23 2022, 12:49 PM

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
Service/
43 lines

Diff 17056

View Options

services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.cpp

Loading...