Skip to content

PSP-11710 : PIMS Notifications Panel not Displaying Reminders (BVT O… #5347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
asanchezr merged 3 commits into from
Jun 30, 2026
Merged

PSP-11710 : PIMS Notifications Panel not Displaying Reminders (BVT O… #5347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de041e2
PSP-11710 : PIMS Notifications Panel not Displaying Reminders (BVT O…
eddherrera Jun 29, 2026
2921f26
- UPDATE
eddherrera Jun 29, 2026
6fd9a5c
- bump
eddherrera Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions source/backend/api/Pims.Api.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
<Project Sdk="Microsoft.NET.Sdk.Web">
<PropertyGroup>
<UserSecretsId>0ef6255f-9ea0-49ec-8c65-c172304b4926</UserSecretsId>
<Version>6.3.0-123.13</Version>
<AssemblyVersion>6.3.0.123</AssemblyVersion>
<Version>6.3.1-123.13</Version>
<AssemblyVersion>6.3.1.123</AssemblyVersion>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<ProjectGuid>{16BC0468-78F6-4C91-87DA-7403C919E646}</ProjectGuid>
<TargetFramework>net8.0</TargetFramework>
Expand Down
22 changes: 19 additions & 3 deletions source/backend/api/Repositories/Ches/ChesRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
using System.Net.Http;
using System.Net.Http.Headers;
using System.Net.Mime;
using System.Security.Authentication;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading.Tasks;
Expand All @@ -12,6 +11,7 @@
using Pims.Api.Models.CodeTypes;
using Pims.Api.Models.Config;
using Pims.Api.Models.Requests.Http;
using Pims.Core.Api.Exceptions;
using Polly.Registry;

namespace Pims.Api.Repositories.Ches
Expand Down Expand Up @@ -98,33 +98,49 @@
_logger.LogError("CHES email send failed: {Status} {Reason} {Body}", response.StatusCode, response.ReasonPhrase, errorBody);
result.Message = $"CHES email send failed: {response.StatusCode} {response.ReasonPhrase}. Response body: {errorBody}";
}

return result;
}
catch (HttpRequestException ex)
{
result.Status = ExternalResponseStatus.Error;
result.Message = $"HTTP error sending CHES email: {ex.Message}";
_logger.LogError(ex, "HTTP error sending CHES email.");

return result;
}
catch (TaskCanceledException ex)
{
result.Status = ExternalResponseStatus.Error;
result.Message = $"Timeout sending CHES email: {ex.Message}";
_logger.LogError(ex, "Timeout sending CHES email.");

return result;
}
catch (JsonException ex)
{
result.Status = ExternalResponseStatus.Error;
result.Message = $"Serialization error: {ex.Message}";
_logger.LogError(ex, "Serialization error sending CHES email.");

return result;
}
catch (AuthenticationException ex)
{
result.Status = ExternalResponseStatus.Error;
result.Status = ExternalResponseStatus.NotExecuted;
result.Message = $"Authentication error: {ex.Message}";
_logger.LogError(ex, "Authentication error sending CHES email.");

return result;
}
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;
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
Comment on lines +136 to +143
}

public async Task<HttpResponseMessage> TryGetHealthAsync()
Expand Down
12 changes: 5 additions & 7 deletions source/backend/api/Services/NotificationUserService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ public async Task PushNotificationUser(long notificationUserId)
}

userNotification.NotificationRetryCnt = userNotification.NotificationRetryCnt.HasValue ? ++userNotification.NotificationRetryCnt : 1;
var updatedNotification = _notificationUserOutputRepository.Update(userNotification);
_notificationUserOutputRepository.CommitTransaction();

if (userNotification.NotificationOutputTypeCode == NotificationOutputTypes.EMAIL.ToString())
{
Expand All @@ -77,24 +75,24 @@ public async Task PushNotificationUser(long notificationUserId)
{
case ExternalResponseStatus.Success:
{
updatedNotification.NotificationSentDt = DateTime.UtcNow;
userNotification.NotificationSentDt = DateTime.UtcNow;
}
break;
case ExternalResponseStatus.Error:
case ExternalResponseStatus.NotExecuted:
{
updatedNotification.NotificationErrorDt = DateOnly.FromDateTime(DateTime.UtcNow);
updatedNotification.NotificationErrorReason = chesResponse.Message;
userNotification.NotificationErrorDt = DateOnly.FromDateTime(DateTime.UtcNow);
userNotification.NotificationErrorReason = chesResponse.Message;
}
break;
}
}
else
{
updatedNotification.NotificationSentDt = DateTime.UtcNow;
userNotification.NotificationSentDt = DateTime.UtcNow;
}

_notificationUserOutputRepository.Update(updatedNotification);
_ = await _notificationUserOutputRepository.Update(userNotification);
_notificationUserOutputRepository.CommitTransaction();

return;
Expand Down
3 changes: 2 additions & 1 deletion source/backend/dal/Repositories/Interfaces/INotificationUserOutputRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections.Generic;
using System.Threading.Tasks;
using Pims.Api.Models.Models.Concepts.Notification;
using Pims.Dal.Entities;

Expand All @@ -10,6 +11,6 @@ public interface INotificationUserOutputRepository : IRepository

public PimsNotificationUserOutput GetById(long notificationUserOutputId);

public PimsNotificationUserOutput Update(PimsNotificationUserOutput userNotification);
public Task<PimsNotificationUserOutput> Update(PimsNotificationUserOutput userNotification);
}
}
23 changes: 12 additions & 11 deletions source/backend/dal/Repositories/NotificationUserOutputRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Claims;
using System.Threading.Tasks;
using LinqKit;
using Medallion.Threading;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Pims.Api.Models.Models.Concepts.Notification;
Expand All @@ -14,12 +15,16 @@ namespace Pims.Dal.Repositories
{
public class NotificationUserOutputRepository : BaseRepository<PimsNotificationUserOutput>, INotificationUserOutputRepository
{
private readonly IDistributedLockProvider _synchronizationProvider;

public NotificationUserOutputRepository(
PimsContext dbContext,
ClaimsPrincipal user,
ILogger<DocumentRepository> logger)
IDistributedLockProvider synchronizationProvider,
ILogger<NotificationUserOutputRepository> logger)
: base(dbContext, user, logger)
{
_synchronizationProvider = synchronizationProvider;
}

public PimsNotificationUserOutput GetById(long notificationUserOutputId)
Expand Down Expand Up @@ -75,25 +80,21 @@ public PimsNotificationUserOutput GetById(long notificationUserOutputId)
.FirstOrDefault(x => x.NotificationUserOutputId == notificationUserOutputId) ?? throw new KeyNotFoundException();
}

public PimsNotificationUserOutput Update(PimsNotificationUserOutput userNotification)
public async Task<PimsNotificationUserOutput> Update(PimsNotificationUserOutput userNotification)

@asanchezr asanchezr Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://learn.microsoft.com/en-us/dotnet/csharp/asynchronous-programming/task-asynchronous-programming-model#naming-convention

@eddherrera eddherrera Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should do on a another version.

{
try
var @lock = _synchronizationProvider.CreateLock("NotificationUserOutputLock");

@asanchezr asanchezr Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment here goes a long way in explaining future developers why we put a lock in place here:

Suggested change
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");

@eddherrera eddherrera Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not always if code if self explanatory.

await using (await @lock.AcquireAsync())
{
using var scope = Logger.QueryScope();
userNotification.ThrowIfNull(nameof(userNotification));

var existingUserNotification = Context.PimsNotificationUserOutputs
.FirstOrDefault(x => x.NotificationUserOutputId == userNotification.NotificationUserOutputId);

Context.Entry(existingUserNotification).CurrentValues.SetValues(userNotification);
Context.Update(existingUserNotification);

@asanchezr asanchezr Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@eddherrera eddherrera Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried it, didnt work. not needed.


return existingUserNotification;
}
catch(Exception ex)
{
Logger.LogError(ex.Message);
return null;
}
}

public IEnumerable<PimsNotificationUserOutput> GetAllByFilter(NotificationUserSearchFilterModel filter)
Expand Down Expand Up @@ -122,7 +123,7 @@ public IEnumerable<PimsNotificationUserOutput> GetAllByFilter(NotificationUserSe

if(filter.NotificationTriggerDate is not null)
{
predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate == filter.NotificationTriggerDate);
predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate <= filter.NotificationTriggerDate);

@asanchezr asanchezr Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Suggested change
predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate <= filter.NotificationTriggerDate);
predicateBuilder.And(x => x.NotificationUser.Notification.NotificationTriggerDate <= filter.NotificationTriggerDate).And(x => x.NotificationSentDt == null);

@asanchezr asanchezr Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise we risk the query returning ALL outputs from this day on (including all previously sent)

@eddherrera eddherrera Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotificationSentDate == null is already in the predicate builder.

@asanchezr asanchezr Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see

}

var query = Context.PimsNotificationUserOutputs
Expand Down
4 changes: 2 additions & 2 deletions source/frontend/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion source/frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "frontend",
"version": "6.3.0-123.13",
"version": "6.3.1-123.13",
"private": true,
"dependencies": {
"@bcgov/bc-sans": "1.0.1",
Expand Down
Loading