PSP-11251 Notifications popover

[asanchezr]

Empty state

< 10 notifications

Notification menu

Load more when >= 10 notifications active

Scrollbar state

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[asanchezr]

To facilitate testing:

Fist, open a few leases from the Lease List view and add a reminder for the Lease Renewal expiry date. If the lease has no renewal, you will have to edit the lease and add a renewal with an expiry date first.

Once you have lease renewals in place, you can add reminders for it. Make sure to make the reminders in the past (ie, yesterday or older) - I'll explain in a moment why

Then, after setting reminders for a few leases (e.g. 5 or more), run the following SQL to "trigger" the notifications for those reminders. The SQL will look at the reminders with trigger date less than today and will set the "SENT_DATE" on the notification output table for the corresponding entry. This will allow the popover to show the notifications (basically this is what the scheduler does daily so the SQL is a shortcut for testing locally)

WITH cte AS (
SELECT TOP 100 OUTP.NOTIFICATION_SENT_DT, OUTP.CONCURRENCY_CONTROL_NUMBER FROM PIMS_NOTIFICATION_USER_OUTPUT OUTP JOIN 
	PIMS_NOTIFICATION_USER NUSR ON OUTP.NOTIFICATION_USER_ID = NUSR.NOTIFICATION_USER_ID JOIN
	PIMS_NOTIFICATION NOTI ON NUSR.NOTIFICATION_ID = NOTI.NOTIFICATION_ID
WHERE OUTP.NOTIFICATION_OUTPUT_TYPE_CODE = 'PIMS' 
	AND NOTI.NOTIFICATION_TRIGGER_DATE <= CONVERT (DATE, GETUTCDATE())
ORDER BY NOTI.NOTIFICATION_TRIGGER_DATE ASC
)

UPDATE cte 
SET CONCURRENCY_CONTROL_NUMBER = CONCURRENCY_CONTROL_NUMBER + 1,
NOTIFICATION_SENT_DT = CONVERT (DATE, GETUTCDATE())

[eddherrera]

Would like to see if there's any other conflict from the Hangfire PR. Please include and rebase your PR.

[asanchezr]

Would like to see if there's any other conflict from the Hangfire PR. Please include and rebase your PR.

PR rebased

[eddherrera]

NIT: I found this naming ambiguous between [PIMS_NOTIFICATION_USER_OUTPUT and [PIMS_NOTIFICATION_OUTPUT_TYPE]

[asanchezr]

I'd like to keep it as is for now. The type is not a model but an enum. The word user is actually confusing there. The notification "output" === "delivery" (at least in my mind). If after a while we are still confused we can rename those later

[asanchezr]

We can have a longer discussion on entity naming for notifications and create a tech debt ticket to address all renaming there

[eddherrera]

do we need this second ordering? I notice that in your snapshots is not ordered by date. could this be the cause of it?

[eddherrera]

We could have just always return the "deep" wich is ok since none of these goes above the granchild level.

[asanchezr]

The deep comes at a cost. This is 13 tables joined together. If you look at all the code paths, we don't need deep for the case of updating the "isRead/isUnread" (which lives in the main entity, not any of the associated joins).

For the case of either getting a list of "inbox items" or a single item, then yes the deep joins are needed

[eddherrera]

nit: At repository level I would have rather name it closer to the Entity. 'NotificationUserOutputRepo..'

[asanchezr]

Sure, but you already created one that was named like that NotificationUserOutputRepo which is why in your PR I commented that we need to differentiate the ADMIN surface area API from the USER based API. We can align on naming and rename all the repos/services/controllers next sprint

[sonarqubecloud]

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud