[cnathe]

Rationale

The notification panel loaded a user's entire notification history into memory just to show the unread ones, triggering large-result warnings on labkey.org (71k rows for one user). This PR loads only unread notifications, streamed rather than collected, and checks for read notifications with a lightweight exists query. Issue: GH Issue 1632

Related Pull Requests

• None

Changes

• The notification panel now streams only unread notifications, and checks for read ones with an exists query
• The notification list API streams results, keeping only the requested page in memory while counting totals
• The list-returning notification lookups are replaced by a single streaming lookup that optionally filters by type label
• Removed unused notification service methods (getNotificationsByType and the 3-arg getNotificationsByUser/getNotificationsByTypeLabels overloads); getNotificationsByUser/getNotificationsByTypeLabels themselves are replaced by the streaming lookup
• Mark all as read still marks notifications one at a time; it now collects unread rows itself, bypassing the large-result warning, and a TODO notes the follow-up to make it a single bulk update

[labkey-adam]

@cnathe this is great for that case. Should we convert the other List<Notification> entry points to use forEach...() or similar? Or remove them (looks like at least one is unused right now). Looks like we'll get the warning in other, perhaps less common, scenarios. Like mark all for example.

[cnathe]

@labkey-adam I agree that there is more to be done here. I'll look at those other cases and see about changing to forEach for them as well.

I didn't want to go too far with this PR since the larger scope change would probably be to make some larger improvements to this feature for the "many notifications" case to work better in general. For example, capping the panel to show something like "last 1,000 notifications" or to have some sort of background process that just starts cleaning up notifications after a certain number or certain period of time (since those users are clearly not looking at them if they get into the thousands of notifications range).

[labkey-adam]

Yep, seems good for now

[labkey-adam]

Manual testing looks good. I had ~300 notifications, so not enough to trigger the warning but certainly enough to exercise the code paths. Panel, viewing & deleting individual notifications, marking all as read, deleting all, etc. I did notice some old notification URLs not resolving, but I'll open a separate issue for that.