Page MenuHomePhabricator
Differential D13576

[lib] fix `PushHandler` to support DMs
ClosedPublic
Actions

Authored by tomek on Wed, Oct 2, 8:58 AM.
Tags
None
Referenced Files
F2870547: D13576.id44829.diff
Wed, Oct 2, 7:26 PM
F2870316: D13576.id.diff
Wed, Oct 2, 6:54 PM
F2869788: D13576.diff
Wed, Oct 2, 5:48 PM
F2868533: D13576.diff
Wed, Oct 2, 3:21 PM
F2868273: D13576.id44837.diff
Wed, Oct 2, 3:02 PM
F2867657: D13576.id44829.diff
Wed, Oct 2, 2:13 PM
F2867409: D13576.id44836.diff
Wed, Oct 2, 1:44 PM
F2867021: D13576.id.diff
Wed, Oct 2, 12:58 PM
View All 11 Files
Subscribers
None

Details

Reviewers
ashoat
kamil
Commits
rCOMMd304b0bc35f4: [lib] fix `PushHandler` to support DMs
Summary

Solution described in ENG-9465.

Test Plan

Apply this diff:

  1. Comment this line to make sure we're testing actual notifs code & add some logs to make sure code in reducer was executed.
  2. Open physical device
  3. In DM thread send a test message
  4. Notif delivered
  5. Message immediately visible in UI
  6. Thread marked as unread
  7. Logs from reducer are there

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D13576
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Wed, Oct 2, 8:58 AM
kamil held this revision as a draft.
kamil published this revision for review.Wed, Oct 2, 8:59 AM
tomek accepted this revision.Wed, Oct 2, 9:06 AM
tomek added inline comments.
lib/reducers/thread-reducer.js
338 ↗(On Diff #44829)

When threadInfo.currentUser.unread we should bump the timestamp.

341–342 ↗(On Diff #44829)

Might be a good idea to reference in getThreadUpdatesForNewMessages this place.

This revision is now accepted and ready to land.Wed, Oct 2, 9:06 AM
Harbormaster completed remote builds in B32019: Diff 44829.Wed, Oct 2, 9:16 AM
ashoat added inline comments.Wed, Oct 2, 9:19 AM
lib/reducers/thread-reducer.js
341–342 ↗(On Diff #44829)

I'm confused why we'd want to set the thread to unread even if the timestamp is newer than the message timestamp

What's the benefit of this? Doesn't it introduce a risk of setting something to unread incorrectly if a notif for message X is received after a Tunnelbroker message is received for message X?

ashoat accepted this revision.Wed, Oct 2, 10:54 AM

Preemptively accepting so that after @tomek commandeers, it's not in an In Review state

tomek commandeered this revision.Wed, Oct 2, 11:31 AM
tomek edited reviewers, added: kamil; removed: tomek.
tomek updated this revision to Diff 44836.Wed, Oct 2, 11:32 AM

Check the timestamp before updating the status

ashoat accepted this revision.Wed, Oct 2, 11:39 AM
Harbormaster completed remote builds in B32024: Diff 44836.Wed, Oct 2, 11:52 AM
tomek added a comment.Wed, Oct 2, 11:55 AM

Performed Kamil's test again (except the logs stuff) and the solution works correctly.

Closed by commit rCOMMd304b0bc35f4: [lib] fix `PushHandler` to support DMs (authored by kamil, committed by tomek). · Explain WhyWed, Oct 2, 11:57 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMMd304b0bc35f4: [lib] fix `PushHandler` to support DMs.

Revision Contents
Changeset List

PathSize
lib/
reducers/
32 lines
types/
2 lines
native/
push/
3 lines

Diff 44836

View Options

lib/reducers/thread-reducer.js

Loading...
View Options

lib/types/message-types.js

Loading...
View Options

native/push/push-handler.react.js

Loading...