Page MenuHomePhabricator

Notifications - fix race condition around "Mark All Read".
ClosedPublic

Authored by btrahan on Aug 1 2014, 8:16 PM.
Tags
None
Referenced Files
F14062700: D10113.diff
Mon, Nov 18, 1:44 PM
F14046342: D10113.diff
Wed, Nov 13, 8:24 PM
F14033304: D10113.diff
Sat, Nov 9, 5:24 PM
F14018991: D10113.diff
Tue, Nov 5, 8:50 PM
F14001818: D10113.id24321.diff
Fri, Oct 25, 12:17 PM
F13995774: D10113.id24322.diff
Wed, Oct 23, 3:24 PM
F13990582: D10113.diff
Tue, Oct 22, 4:44 AM
F13975147: D10113.id24321.diff
Oct 18 2024, 8:58 AM
Subscribers

Details

Summary

pre-patch "Mark All Read" marks *all* unread notifications as read. This is a race condition in that the user is looking at some set of notiifcations and that set may update such that the newest notifications aren't shown. An example might be if sitting on the notifications page or having the menu open while a new notification comes in... Note re-opening the menu would show the latest notifications.

This patch makes it so "Mark All Read" links only marks the notifications currently loaded (and older.) Fixes T5764.

Additionally, if there is nothing to "mark read" the button / link "Mark All Read" will have a disabled style and yield a dialog saying "nothing to mark as read".

Test Plan

carefully tracked ?chronoKey populating correctly in various links. Verified query constructed properly too.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

btrahan retitled this revision from to Notifications - fix race condition around "Mark All Read"..
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.
epriestley added inline comments.
src/applications/notification/controller/PhabricatorNotificationClearController.php
16–17

Should this be <=, so the most recent one gets marked too?

src/applications/notification/controller/PhabricatorNotificationListController.php
64

I think we should still show the button, just disable it (for consistency with other UIs where possible-but-unavailable actions are greyed out).

src/applications/notification/controller/PhabricatorNotificationPanelController.php
31–32

And likewise herte.

This revision is now accepted and ready to land.Aug 1 2014, 8:29 PM

I think removing phabricator/src/.phutil_module_cache will fix that lint now, if you've update libphutil/.

ah, nice catch...! will update after lunch.

btrahan edited edge metadata.
  • <=
  • make buttons be disabled as opposed to not rendered
    • make a little dialog for the disabled case a la subscribers, etc
btrahan updated this revision to Diff 24322.

Closed by commit rPe50b26941643 (authored by @btrahan).