Page MenuHomePhabricator

Consider requesting more notifications if the popup only has bundles
Open, Needs TriagePublic

Description

When we ask the API for certain amount of notifications (in the popup's case - 25) the API returns a response that ignores the bundle number.

For example, if we ask for 25 notifications but we have 10 new-topic notifications and 15 user-talk-page notifications, the API will return all 25 but the UI will display only two bundles, one for the new topic and one for the user talk page. Worse yet, if the user also had, say, 5 more unread notifications that are unrelated (and unbundled) the user will not see them, because they are beyond the 25-limit API request.

This can look really awkward, and can hide valid unread notifications.

What we could do is have the model manager run a "do I have enough first-level notifications" test and request more notifications if it only has certain top-level widget items, under a certain cap.

So, this can be a good scenario:

  1. The badge is clicked, triggering a request for notifications from the controller (current behavior)
  2. The controller asks the API for 25 notifications (current behavior)
  3. The controller populates the model manager with the response. (current behavior)
  4. After the population is done, the controller asks the model manager how many "top level" items it has (how many items in the 'local' model + how many models)
  5. If the answer is under some threshhold, the controller will use the 'continue' value (it should store that in step #3 as we do when we populate Special:Notifications) to fetch more notifications.
  6. The controller adds to the model manager (we need to make sure the model manager allows for that; currently, it has an "update" event which assumes the model is filled once)
  7. The widget fills itself according to the model(s)

We will have to consider how to change the current model behavior so we can add more notifications to the model. Right now, the model assumes the population happens once (and emits 'update' which the widget then uses to empty itself and refill) but that can be a relatively minor implementation details. We can also avoid that by having the controller itself count the number of non-bundled notifications from the API response itself before it fills the model manager, ask for more items, and then fill the manager -- following the "proper" and current operation that we currently have with the manager being filled and refilled from scratch each time.

Event Timeline

[...] can look really awkward, and can hide valid unread notifications.

T144311: When a bundle has 25 notifications no other notifications are displayed illustrates the case when if there are bundled notifications (25 and less) and no other notifications get displayed.

This is particularly bad with the new "successful pings sent" notification, which I believe has an upper limit of 50, so it's common for people to get more than 25+ bundled notifications at once, triggering this bug. Maybe bumping the default limit to 50 would make this edge case harder to hit? I don't remember how 25 was chosen.

As @AlexisJazz mentions above, this is due to the notifications API call made having a limit set (notlimit=25, from mw.echo.api.EchoApi.js I think?), and bundling related notifications (notbundle=1) (API docs)

{P27831, highlight=19-44}
<snip>

We could have the API module query for notlimit+1 entries, if a 26th row is returned add a flag in the API response like more: true, then we insert a placeholder notification at the bottom of the flyout that says "Read more notifications at Special:Notifications", which has pagination.

We could have the API module query for notlimit+1 entries, if a 26th row is returned add a flag in the API response like more: true, then we insert a placeholder notification at the bottom of the flyout that says "Read more notifications at Special:Notifications", which has pagination.

Could we not just do if rawcount > notlimit, as that'd be one less API call? My understanding is that the (raw)count will return the actual number of notifications irrespective of the notlimit

D'oh, yep. Sounds good to me.