Skip to content
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.

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

Update Papers and Reports #1206

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Update Papers and Reports #1206

merged 1 commit into from
Oct 31, 2023

Conversation

Fosol
Copy link
Collaborator

@Fosol Fosol commented Oct 31, 2023

The Papers page now provides a way to preview the Front Page Images report and the Morning report. It also provides a way to manually initiate the request to publish (send out) the Front Page Images report, Morning Report and the Top Story Notifications.

Notifications has been enhanced to use an Elasticsearch query filter. Administrators are able to create two types of notifications, those that are run when content is indexed (Basic Alert), and those that are run manually. Those that are run manually use their filter to determine what content will be included. They also will not return content older than the last notification sent out.

Run the make renew command to update your local environment.

Summary

  • Add send notifications by filter
  • Add Top Story Notification
  • Publish tno-core:0.0.355
  • Add DB migration 1.0.83
  • Notification admin can now reuse notification templates
  • Notification admin can now test notifications and preview template changes
  • Fixed implementation issues discovered while making changes to API and hooks
  • Updated Editor app
  • Updated Subscriber app
  • Updated API
  • Updated Reporting Service
  • Updated Notification Service

Papers

image

Notification Admin

image

Addsend notifications by filter
Add Top Story Notification
Publish tno-core:0.0.355
Add DB migration 1.0.83
@Fosol Fosol added bug Something isn't working enhancement New feature or request DB Migration A DB Migration may require refreshing or simply updating your database. subscriber PR contains changes towards the subscriber application, tno-core update Indicates that there have been changes to our tno-core package, which can pose concurrency issues. editor labels Oct 31, 2023
@Fosol Fosol self-assigned this Oct 31, 2023
@@ -147,7 +147,7 @@ public async Task<IActionResult> PreviewJsonAsync(ChartPreviewRequestModel model
Template = model.Template,
SectionSettings = model.Settings
};
var chartTemplate = new TemplateEngine.Models.Reports.ChartEngineContentModel("test", chart, model.Content?.Select(c => new TemplateEngine.Models.Reports.ContentModel(c)));
var chartTemplate = new TemplateEngine.Models.Reports.ChartEngineContentModel("test", chart, model.Content?.Select(c => new TemplateEngine.Models.ContentModel(c)));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved models to more generic location as they are used by more than one service.

{
return new JsonResult(_notificationService.FindAll().Select(ds => new NotificationModel(ds, _serializerOptions)));
var uri = new Uri(this.Request.GetDisplayUrl());
var query = Microsoft.AspNetCore.WebUtilities.QueryHelpers.ParseQuery(uri.Query);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can search for notifications now.

@@ -164,16 +164,16 @@ public IActionResult Delete(AVOverviewInstanceModel model)
/// </summary>
/// <param name="instanceId"></param>
/// <returns></returns>
[HttpPost("{instanceId}/preview")]
[HttpPost("{instanceId}/view")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

View is a better way to describe an instance. Preview is before an instance is created.

[SwaggerOperation(Tags = new[] { "Report" })]
public async Task<IActionResult> FindContentForReportIdAsync(int id)
public async Task<IActionResult> FindContentForReportIdAsync(int id, int? requestorId)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Including the requestor allows other users to generate instances from a report they do not own.

[Route("[area]/reports/av/overviews")]
[ProducesResponseType(typeof(ErrorResponseModel), (int)HttpStatusCode.Unauthorized)]
[ProducesResponseType(typeof(ErrorResponseModel), (int)HttpStatusCode.Forbidden)]
public class AVOverviewController : ControllerBase
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jdtoombs I thought the Evening Overview was visible on the Subscriber app? If it is, I don't know how it can be currently working correctly because there was no endpoints for it.

@@ -14,7 +14,7 @@ namespace TNO.API.Areas.Subscriber.Controllers;
/// </summary>
[ClientRoleAuthorize(ClientRole.Subscriber)]
[ApiController]
[Area("editor")]
[Area("subscriber")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed bug

/// <summary>
///
/// </summary>
public class BackgroundTaskQueue : IBackgroundTaskQueue
{
private ConcurrentQueue<Func<CancellationToken, Task>> _workItems = new ConcurrentQueue<Func<CancellationToken, Task>>();
private SemaphoreSlim _signal = new SemaphoreSlim(0);
private readonly ConcurrentQueue<Func<CancellationToken, Task>> _workItems = new();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kylgarmor you should add a linter to your vscode. It will also catch many of the nullable issues.

@@ -45,7 +45,7 @@ const DataLocationForm: React.FC = () => {
);

const dataLocationId = Number(id);
const connectionOptions = getSortableOptions(connections, [
const connectionOptions = getSortableOptions(connections, dataLocation.connectionId, [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to do this to fix the bug where the currently selected value has been disabled and it wasn't been included in the list.

isEnabled: false,
isPublic: false,
sortOrder: 0,
subscribers: [],
notificationType: NotificationTypeName.Email,
requireAlert: false,
alertOnIndex: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This replaces the logic of requireAlert. Now if a notification requires an alert it simply includes that as part of its filter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jdtoombs Keep common hooks used to communicate with the API in consistent location.

builder.Property(m => m.Settings).IsRequired().HasColumnType("jsonb").HasDefaultValueSql("'{}'::jsonb");
builder.Property(m => m.Template).IsRequired().HasColumnType("text");
builder.Property(m => m.Query).IsRequired().HasColumnType("jsonb").HasDefaultValueSql("'{}'::jsonb");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar to structure to Filters

using Microsoft.EntityFrameworkCore.Metadata.Builders;
using TNO.Entities;

public class NotificationTemplateConfiguration : BaseTypeConfiguration<NotificationTemplate, int>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extracted notification templates to be similar to report templates.

@@ -75,12 +124,60 @@ public override Notification Update(Notification entity)
}
});


// Add/Update the report template.
if (entity.Template != null)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Notification updates will update the template. Same thing happens with reports. I don't love this solution, but it works for us currently.

/// <param name="notification"></param>
/// <param name="requestorId"></param>
/// <returns></returns>
public async Task<Elastic.Models.SearchResultModel<API.Areas.Services.Models.Content.ContentModel>> FindContentWithElasticsearchAsync(Notification notification, int? requestorId)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main issue I see with this implementation is that the first time the notification runs it will get a lot of content... I'll need to create a story to handle this.

@Fosol Fosol merged commit e5aa56b into bcgov:dev Oct 31, 2023
3 checks passed
@Fosol Fosol deleted the papers branch October 31, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working DB Migration A DB Migration may require refreshing or simply updating your database. editor enhancement New feature or request subscriber PR contains changes towards the subscriber application, tno-core update Indicates that there have been changes to our tno-core package, which can pose concurrency issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant