1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00

Fix the most significant "phantom notification" badness

Summary:
Ref T13124. Ref T13131. Fixes T8953. See PHI512.

When you receieve a notification about an object and then someone hides that object from you (or deletes it), you get a phantom notification which is very difficult to clear.

For now, test that notifications are visible when you open the menu and clear any that are not.

This could be a little more elegant than it is, but the current behavior is very clearly broken. This unbreaks it, at least.

Test Plan:
  - As Alice, configured task stuff to notify me (instead of sending email).
  - As Bailey, added Alice as a subscriber to a task, then commented on it.
  - As Alice, loaded home and saw a notification count. Didn't click it yet.
  - As Bailey, set the task to private.
  - As Alice, clicked the notification bell menu icon.
    - Before change: no unread notifications, bell menu is semi-stuck in a phantom state which you can't clear.
    - After change: bad notifications automatically cleared.

{F5530005}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13131, T13124, T8953

Differential Revision: https://secure.phabricator.com/D19384
This commit is contained in:
epriestley 2018-04-19 10:49:30 -07:00
parent e81b2173ad
commit 70d67a3908
5 changed files with 105 additions and 13 deletions

View file

@ -9,7 +9,7 @@ return array(
'names' => array(
'conpherence.pkg.css' => 'e68cf1fa',
'conpherence.pkg.js' => '15191c65',
'core.pkg.css' => '39061f68',
'core.pkg.css' => 'c30f6eaa',
'core.pkg.js' => 'e1f0f7bd',
'differential.pkg.css' => '06dc617c',
'differential.pkg.js' => 'c2ca903a',
@ -38,7 +38,7 @@ return array(
'rsrc/css/application/almanac/almanac.css' => 'dbb9b3af',
'rsrc/css/application/auth/auth.css' => '0877ed6e',
'rsrc/css/application/base/main-menu-view.css' => '1802a242',
'rsrc/css/application/base/notification-menu.css' => '10685bd4',
'rsrc/css/application/base/notification-menu.css' => 'ef480927',
'rsrc/css/application/base/phui-theme.css' => '9f261c6b',
'rsrc/css/application/base/standard-page-view.css' => '34ee718b',
'rsrc/css/application/chatlog/chatlog.css' => 'd295b020',
@ -768,7 +768,7 @@ return array(
'phabricator-nav-view-css' => '694d7723',
'phabricator-notification' => '4f774dac',
'phabricator-notification-css' => '457861ec',
'phabricator-notification-menu-css' => '10685bd4',
'phabricator-notification-menu-css' => 'ef480927',
'phabricator-object-selector-css' => '85ee8ce6',
'phabricator-phtize' => 'd254d646',
'phabricator-prefab' => '77b0ae28',

View file

@ -6,6 +6,10 @@ final class PhabricatorNotificationPanelController
public function handleRequest(AphrontRequest $request) {
$viewer = $request->getViewer();
$unread_count = $viewer->getUnreadNotificationCount();
$warning = $this->prunePhantomNotifications($unread_count);
$query = id(new PhabricatorNotificationQuery())
->setViewer($viewer)
->withUserPHIDs(array($viewer->getPHID()))
@ -66,13 +70,12 @@ final class PhabricatorNotificationPanelController
));
$content = hsprintf(
'%s%s%s',
'%s%s%s%s',
$header,
$warning,
$content,
$connection_ui);
$unread_count = $viewer->getUnreadNotificationCount();
$json = array(
'content' => $content,
'number' => (int)$unread_count,
@ -80,4 +83,81 @@ final class PhabricatorNotificationPanelController
return id(new AphrontAjaxResponse())->setContent($json);
}
private function prunePhantomNotifications($unread_count) {
// See T8953. If you have an unread notification about an object you
// do not have permission to view, it isn't possible to clear it by
// visiting the object. Identify these notifications and mark them as
// read.
$viewer = $this->getViewer();
if (!$unread_count) {
return null;
}
$table = new PhabricatorFeedStoryNotification();
$conn = $table->establishConnection('r');
$rows = queryfx_all(
$conn,
'SELECT chronologicalKey, primaryObjectPHID FROM %T
WHERE userPHID = %s AND hasViewed = 0',
$table->getTableName(),
$viewer->getPHID());
if (!$rows) {
return null;
}
$map = array();
foreach ($rows as $row) {
$map[$row['primaryObjectPHID']][] = $row['chronologicalKey'];
}
$handles = $viewer->loadHandles(array_keys($map));
$purge_keys = array();
foreach ($handles as $handle) {
$phid = $handle->getPHID();
if ($handle->isComplete()) {
continue;
}
foreach ($map[$phid] as $chronological_key) {
$purge_keys[] = $chronological_key;
}
}
if (!$purge_keys) {
return null;
}
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
$conn = $table->establishConnection('w');
queryfx(
$conn,
'UPDATE %T SET hasViewed = 1
WHERE userPHID = %s AND chronologicalKey IN (%Ls)',
$table->getTableName(),
$viewer->getPHID(),
$purge_keys);
PhabricatorUserCache::clearCache(
PhabricatorUserNotificationCountCacheType::KEY_COUNT,
$viewer->getPHID());
unset($unguarded);
return phutil_tag(
'div',
array(
'class' => 'phabricator-notification phabricator-notification-warning',
),
pht(
'%s notification(s) about objects which no longer exist or which '.
'you can no longer see were discarded.',
phutil_count($purge_keys)));
}
}

View file

@ -76,31 +76,31 @@ final class PhabricatorNotificationQuery
return $stories;
}
protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
$where = array();
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
$where = parent::buildWhereClauseParts($conn);
if ($this->userPHIDs !== null) {
$where[] = qsprintf(
$conn_r,
$conn,
'notif.userPHID IN (%Ls)',
$this->userPHIDs);
}
if ($this->unread !== null) {
$where[] = qsprintf(
$conn_r,
$conn,
'notif.hasViewed = %d',
(int)!$this->unread);
}
if ($this->keys) {
$where[] = qsprintf(
$conn_r,
$conn,
'notif.chronologicalKey IN (%Ls)',
$this->keys);
}
return $this->formatWhereClause($where);
return $where;
}
protected function getResultCursor($item) {

View file

@ -1651,6 +1651,14 @@ final class PhabricatorUSEnglishTranslation
'Destroyed %s credentials of type "%s".',
),
'%s notification(s) about objects which no longer exist or which '.
'you can no longer see were discarded.' => array(
'One notification about an object which no longer exists or which '.
'you can no longer see was discarded.',
'%s notifications about objects which no longer exist or which '.
'you can no longer see were discarded.',
),
);
}

View file

@ -68,6 +68,10 @@
color: {$lightgreytext};
}
.phabricator-notification-warning {
background: {$sh-yellowbackground};
}
.phabricator-notification-list .phabricator-notification-unread,
.phabricator-notification-menu .phabricator-notification-unread {
background: {$hoverblue};
@ -95,7 +99,7 @@
.phabricator-notification-unread .phabricator-notification-foot
.phabricator-notification-status {
font-size: 7px;
color: {$lightgreytext};
color: {$lightbluetext};
position: absolute;
display: inline-block;
top: 6px;