PSP-11710 : PIMS Notifications Panel not Displaying Reminders (BVT O…#5347
Conversation
…servations) - HOTFIX
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/5347 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| result.Status = ExternalResponseStatus.Error; | ||
| result.Message = $"Unexpected error sending CHES email: {ex.Message}"; | ||
| _logger.LogError(ex, "Unexpected error sending CHES email."); | ||
|
|
||
| return result; | ||
| return result; | ||
| } |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/5347 |
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/5347 |
1 similar comment
|
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/5347 |
asanchezr
left a comment
asanchezr
left a comment
There was a problem hiding this comment.
Approved but see comments
| } | ||
|
|
||
| public PimsNotificationUserOutput Update(PimsNotificationUserOutput userNotification) | ||
| public async Task<PimsNotificationUserOutput> Update(PimsNotificationUserOutput userNotification) |
There was a problem hiding this comment.
nit: Naming conventions suggest to name async methods XYZAsync to let consumers know it is async. Can be done next sprint if no time now
There was a problem hiding this comment.
Should do on a another version.
| public async Task<PimsNotificationUserOutput> Update(PimsNotificationUserOutput userNotification) | ||
| { | ||
| try | ||
| var @lock = _synchronizationProvider.CreateLock("NotificationUserOutputLock"); |
There was a problem hiding this comment.
A comment here goes a long way in explaining future developers why we put a lock in place here:
| var @lock = _synchronizationProvider.CreateLock("NotificationUserOutputLock"); | |
| // Use a distributed lock to ensure that only one process can update the notification output table at a time (prevents deadlocks from history triggers). | |
| var @lock = _synchronizationProvider.CreateLock("NotificationUserOutputLock"); |
There was a problem hiding this comment.
Not always if code if self explanatory.
| .FirstOrDefault(x => x.NotificationUserOutputId == userNotification.NotificationUserOutputId); | ||
|
|
||
| Context.Entry(existingUserNotification).CurrentValues.SetValues(userNotification); | ||
| Context.Update(existingUserNotification); |
There was a problem hiding this comment.
I'm not sure if need the extra Update here since the line above updates the values via Context.Entry().CurrentValues
Also I see in the document queue they force the save in scope of the lock like so:
Context.SaveChanges(); // Force changes to be saved here, in the scope of the lock.I would assume we need it here too?
There was a problem hiding this comment.
tried it, didnt work. not needed.
| if(filter.NotificationTriggerDate is not null) | ||
| { | ||
| predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate == filter.NotificationTriggerDate); | ||
| predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate <= filter.NotificationTriggerDate); |
There was a problem hiding this comment.
thank you for fixing this, but I think this query need an extra condition to exclude PRIOR notifications that have already been sent (sent_date !== null)
| predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate <= filter.NotificationTriggerDate); | |
| predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate <= filter.NotificationTriggerDate).And(x => x.NotificationSentDt == null); |
There was a problem hiding this comment.
Otherwise we risk the query returning ALL outputs from this day on (including all previously sent)
There was a problem hiding this comment.
NotificationSentDate == null is already in the predicate builder.
|
Tick the box to add this pull request to the merge queue (same as
|
|
3 participants
…bservations) - HOTFIX