-
Notifications
You must be signed in to change notification settings - Fork 1
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
Admin CLI for sending announcements to all identities #1031
Changes from all commits
6c765b9
1815528
030d9e1
06f88f4
27e09a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -10,11 +10,22 @@ | |||||||||||||||||||||||||||||||||||||
</PropertyGroup> | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
<ItemGroup> | ||||||||||||||||||||||||||||||||||||||
<PackageReference Include="Autofac.Extensions.DependencyInjection" Version="10.0.0" /> | ||||||||||||||||||||||||||||||||||||||
<PackageReference Include="System.CommandLine" Version="2.0.0-beta4.22272.1" /> | ||||||||||||||||||||||||||||||||||||||
</ItemGroup> | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
<ItemGroup> | ||||||||||||||||||||||||||||||||||||||
<ProjectReference Include="..\..\..\..\Infrastructure\Infrastructure.csproj" /> | ||||||||||||||||||||||||||||||||||||||
<ProjectReference Include="..\..\..\..\Modules\Announcements\src\Announcements.Application\Announcements.Application.csproj" /> | ||||||||||||||||||||||||||||||||||||||
<ProjectReference Include="..\..\..\..\Modules\Announcements\src\Announcements.Infrastructure\Announcements.Infrastructure.csproj" /> | ||||||||||||||||||||||||||||||||||||||
<ProjectReference Include="..\..\..\..\Modules\Devices\src\Devices.Infrastructure\Devices.Infrastructure.csproj" /> | ||||||||||||||||||||||||||||||||||||||
</ItemGroup> | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
<ItemGroup> | ||||||||||||||||||||||||||||||||||||||
<None Remove="appsettings.json" /> | ||||||||||||||||||||||||||||||||||||||
<Content Include="appsettings.json"> | ||||||||||||||||||||||||||||||||||||||
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | ||||||||||||||||||||||||||||||||||||||
</Content> | ||||||||||||||||||||||||||||||||||||||
</ItemGroup> | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should add the following lines (it's done like this in all the other application
And I just noticed that I forgot one major thing: since so far the Admin CLI only used two parameters (DB provider and DB connection string), I didn't feel the need to add the full configuration stuff). But now that we need a lot more than that, we should add configuration as we do it in the other projects, like in the ConsumerApi. See backbone/Applications/ConsumerApi/src/Program.cs Lines 219 to 236 in 019ea5b
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add that |
||||||||||||||||||||||||||||||||||||||
</Project> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,134 @@ | ||||||
using System.CommandLine; | ||||||
using System.Text.Json; | ||||||
using Backbone.AdminCli.Commands.BaseClasses; | ||||||
using Backbone.Modules.Announcements.Application.Announcements.Commands.CreateAnnouncement; | ||||||
using Backbone.Modules.Announcements.Domain.Entities; | ||||||
using MediatR; | ||||||
|
||||||
namespace Backbone.AdminCli.Commands.Announcements; | ||||||
|
||||||
public class AnnouncementCommand : AdminCliCommand | ||||||
{ | ||||||
public AnnouncementCommand(ServiceLocator serviceLocator) : base("announcement", serviceLocator) | ||||||
{ | ||||||
AddCommand(new SendAnnouncementCommand(serviceLocator)); | ||||||
} | ||||||
} | ||||||
|
||||||
public class SendAnnouncementCommand : AdminCliDbCommand | ||||||
{ | ||||||
public SendAnnouncementCommand(ServiceLocator serviceLocator) : base("send", serviceLocator) | ||||||
{ | ||||||
var expiresAt = new Option<string?>("--expiration") | ||||||
{ | ||||||
IsRequired = false, | ||||||
Description = "The expiration date of the announcement." | ||||||
}; | ||||||
|
||||||
var severity = new Option<string?>("--severity") | ||||||
{ | ||||||
IsRequired = true, | ||||||
Description = "The severity of the announcement." | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to add a list of possible values to the description of this option. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add that suggestion |
||||||
}; | ||||||
|
||||||
AddOption(expiresAt); | ||||||
AddOption(severity); | ||||||
|
||||||
this.SetHandler(SendAnnouncement, DB_PROVIDER_OPTION, DB_CONNECTION_STRING_OPTION, severity, expiresAt); | ||||||
} | ||||||
|
||||||
private async Task SendAnnouncement(string dbProvider, string dbConnectionString, string? severityInput, string? expiresAtInput) | ||||||
{ | ||||||
try | ||||||
{ | ||||||
var severity = severityInput switch | ||||||
{ | ||||||
_ when string.IsNullOrWhiteSpace(severityInput) => AnnouncementSeverity.Low, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't we agree on removing the default value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. Will be removed. |
||||||
_ when Enum.TryParse<AnnouncementSeverity>(severityInput, ignoreCase: true, out var parsedSeverity) => parsedSeverity, | ||||||
_ => throw new ArgumentException($@"Specified severity '{severityInput}' is not a valid severity.") | ||||||
}; | ||||||
|
||||||
DateTime? expiresAt = expiresAtInput switch | ||||||
{ | ||||||
_ when string.IsNullOrWhiteSpace(expiresAtInput) => null, | ||||||
_ when DateTime.TryParse(expiresAtInput, out var parsedDateTime) => parsedDateTime, | ||||||
_ => throw new ArgumentException($@"Specified expiration datetime '{expiresAtInput}' is not a valid DateTime.") | ||||||
}; | ||||||
|
||||||
var texts = ReadTextsFromCommandLineInput(); | ||||||
|
||||||
if (texts.Count == 0) | ||||||
{ | ||||||
Console.WriteLine(@"No texts provided. Exiting..."); | ||||||
return; | ||||||
} | ||||||
Comment on lines
+60
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not necessary to do this check. This is already done by the MediatR command validator. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see no good reson why not do an early exit, before the MeditR validator gets called, but ok I can remove that check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I usually don't like duplicating the validation logic. More places where you have to remember to change things if you want to. |
||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to print all entered data in order to give the user a chance to review it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean print before send takes place with an option to cancel or what? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing is: once an announcement is created, a push notification is sent to ALL identities. It would be a shame if you do this by accident. :) |
||||||
Console.WriteLine(@"Sending announcement..."); | ||||||
|
||||||
var mediator = _serviceLocator.GetService<IMediator>(dbProvider, dbConnectionString); | ||||||
|
||||||
var response = await mediator.Send(new CreateAnnouncementCommand | ||||||
{ | ||||||
Texts = texts, | ||||||
Severity = severity, | ||||||
ExpiresAt = expiresAt | ||||||
}, CancellationToken.None); | ||||||
|
||||||
Console.WriteLine(@"Announcement sent successfully"); | ||||||
Console.WriteLine(JsonSerializer.Serialize(response, JSON_SERIALIZER_OPTIONS)); | ||||||
} | ||||||
catch (Exception e) | ||||||
{ | ||||||
Console.WriteLine($@"An error occurred: {e.Message}"); | ||||||
} | ||||||
} | ||||||
|
||||||
private static List<CreateAnnouncementCommandText> ReadTextsFromCommandLineInput() | ||||||
{ | ||||||
var texts = new List<CreateAnnouncementCommandText>(); | ||||||
bool addAnotherLanguage; | ||||||
do | ||||||
{ | ||||||
var language = PromptForInput(@"Enter language (e.g. en, de, it, nl). At least 1 must be in english: "); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll adjust it that way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just had an idea. Since it's required to add title and body in English, it may be good to add two required parameters to the command ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'll add that |
||||||
var title = PromptForInput(@"Enter title: "); | ||||||
var body = PromptForInput(@"Enter body: "); | ||||||
|
||||||
if (language == null || title == null || body == null) | ||||||
{ | ||||||
break; | ||||||
} | ||||||
Comment on lines
+96
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you break in this case? This might lead to unintentionally sending the announcement if you for example double-press Enter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll check that |
||||||
|
||||||
texts.Add(new CreateAnnouncementCommandText | ||||||
{ | ||||||
Language = language, | ||||||
Title = title, | ||||||
Body = body | ||||||
}); | ||||||
|
||||||
var input = PromptForInput(@"Do you want to add another language? ([y]es/[n]o): "); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably capitalize the "N" of "[n]o" to make it clear that it's the default option.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||||||
addAnotherLanguage = input?.Trim().ToLower() is "yes" or "y"; | ||||||
} while (addAnotherLanguage); | ||||||
|
||||||
return texts; | ||||||
} | ||||||
|
||||||
private static string? PromptForInput(string prompt) | ||||||
{ | ||||||
Console.Write(prompt); | ||||||
var input = Console.ReadLine(); | ||||||
|
||||||
while (string.IsNullOrWhiteSpace(input)) | ||||||
{ | ||||||
Console.WriteLine($@"Input cannot be empty. Press x to exit."); | ||||||
Console.Write(prompt); | ||||||
input = Console.ReadLine(); | ||||||
|
||||||
if (input == null || !input.Trim().Equals("x", StringComparison.CurrentCultureIgnoreCase)) continue; | ||||||
|
||||||
input = null; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if I'm missing something, but to me it looks as if with this line you always return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah looks little wired. That term is not required and gets removed |
||||||
break; | ||||||
} | ||||||
|
||||||
return input; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using System.ComponentModel.DataAnnotations; | ||
using Backbone.Infrastructure.EventBus; | ||
|
||
namespace Backbone.AdminCli.Configuration; | ||
|
||
public class AdminCliConfiguration | ||
{ | ||
[Required] | ||
public AdminInfrastructureConfiguration Infrastructure { get; set; } = new(); | ||
|
||
[Required] | ||
public ModulesConfiguration Modules { get; set; } = new(); | ||
|
||
public class AdminInfrastructureConfiguration | ||
{ | ||
[Required] | ||
public EventBusConfiguration EventBus { get; set; } = new(); | ||
} | ||
|
||
public class ModulesConfiguration | ||
{ | ||
[Required] | ||
public DevicesCliConfiguration Devices { get; set; } = new(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,16 @@ | ||||||
using System.ComponentModel.DataAnnotations; | ||||||
using Backbone.Modules.Devices.Infrastructure.PushNotifications; | ||||||
|
||||||
namespace Backbone.AdminCli.Configuration; | ||||||
|
||||||
public class DevicesCliConfiguration | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can rename it. |
||||||
{ | ||||||
[Required] | ||||||
public InfrastructureConfiguration Infrastructure { get; set; } = new(); | ||||||
|
||||||
public class InfrastructureConfiguration | ||||||
{ | ||||||
[Required] | ||||||
public PushNotificationOptions PushNotifications { get; set; } = new(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ COPY ["Modules/Devices/src/Devices.Application/Devices.Application.csproj", "Mod | |
COPY ["BuildingBlocks/src/BuildingBlocks.Application/BuildingBlocks.Application.csproj", "BuildingBlocks/src/BuildingBlocks.Application/"] | ||
COPY ["BuildingBlocks/src/Crypto/Crypto.csproj", "BuildingBlocks/src/Crypto/"] | ||
COPY ["Modules/Devices/src/Devices.Domain/Devices.Domain.csproj", "Modules/Devices/src/Devices.Domain/"] | ||
COPY ["Modules/Announcements/src/Announcements.Application/Announcements.Application.csproj", "Modules/Announcements/src/Announcements.Application/"] | ||
COPY ["Modules/Announcements/src/Announcements.Infrastructure/Announcements.Infrastructure.csproj", "Modules/Announcements/src/Announcements.Infrastructure/"] | ||
COPY ["Modules/Announcements/src/Announcements.Domain/Announcements.Domain.csproj", "Modules/Announcements/src/Announcements.Domain/"] | ||
COPY ["Infrastructure/Infrastructure.csproj", "Infrastructure/"] | ||
|
||
|
||
RUN dotnet restore /p:ContinuousIntegrationBuild=true "Applications/AdminCli/src/AdminCli/AdminCli.csproj" | ||
|
||
|
@@ -32,6 +37,9 @@ FROM base AS final | |
WORKDIR /app | ||
COPY --from=build /app/publish/Backbone.AdminCli ./backbone | ||
|
||
# Ensure the appsettings.json file is copied to the correct location | ||
COPY appsettings.json /app/appsettings.json | ||
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, and BTW it doesn't work :-( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's see if it works once you're done with the other configuration related changes |
||
|
||
ENV PATH="$PATH:/app" | ||
|
||
LABEL org.opencontainers.image.source="https://github.com/nmshd/backbone" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,20 @@ | ||
using Autofac; | ||
using Autofac.Extensions.DependencyInjection; | ||
using Backbone.AdminCli.Configuration; | ||
using Backbone.BuildingBlocks.Application.QuotaCheck; | ||
using Backbone.Infrastructure.EventBus; | ||
using Backbone.Modules.Announcements.Infrastructure.Persistence.Database; | ||
using Backbone.Modules.Devices.Application.Extensions; | ||
using Backbone.Modules.Devices.Domain.Entities.Identities; | ||
using Backbone.Modules.Devices.Infrastructure.OpenIddict; | ||
using Backbone.Modules.Devices.Infrastructure.Persistence; | ||
using Backbone.Modules.Devices.Infrastructure.Persistence.Database; | ||
using Backbone.Modules.Devices.Infrastructure.PushNotifications; | ||
using Microsoft.AspNetCore.Identity; | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Options; | ||
using IServiceCollectionExtensions = Backbone.Modules.Devices.Infrastructure.Persistence.IServiceCollectionExtensions; | ||
|
||
|
||
namespace Backbone.AdminCli; | ||
|
||
|
@@ -14,14 +23,15 @@ public class ServiceLocator | |
public T GetService<T>(string dbProvider, string dbConnectionString) where T : notnull | ||
{ | ||
var services = ConfigureServices(dbProvider, dbConnectionString); | ||
|
||
var serviceProvider = services.BuildServiceProvider(); | ||
return serviceProvider.GetRequiredService<T>(); | ||
return services.GetRequiredService<T>(); | ||
} | ||
|
||
private static IServiceCollection ConfigureServices(string dbProvider, string dbConnectionString) | ||
private static IServiceProvider ConfigureServices(string dbProvider, string dbConnectionString) | ||
{ | ||
var services = new ServiceCollection(); | ||
|
||
services.AddAutofac(); | ||
|
||
services | ||
.AddIdentity<ApplicationUser, IdentityRole>() | ||
.AddEntityFrameworkStores<DevicesDbContext>(); | ||
|
@@ -41,14 +51,38 @@ private static IServiceCollection ConfigureServices(string dbProvider, string db | |
services.AddApplicationWithoutIdentityDeletion(); | ||
|
||
services.AddSingleton<IQuotaChecker, AlwaysSuccessQuotaChecker>(); | ||
|
||
services.AddLogging(); | ||
IServiceCollectionExtensions.AddDatabase(services, options => | ||
{ | ||
options.Provider = dbProvider; | ||
options.ConnectionString = dbConnectionString; | ||
}); | ||
|
||
var configuration = new ConfigurationBuilder() | ||
.SetBasePath(Directory.GetCurrentDirectory()) | ||
.AddJsonFile("appsettings.json", optional: false, reloadOnChange: true) | ||
.Build(); | ||
Comment on lines
+61
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be replaced with what I wrote in one of my first comments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||
services.AddSingleton<IConfiguration>(configuration); | ||
services.ConfigureAndValidate<AdminCliConfiguration>(configuration.Bind); | ||
|
||
var serviceProvider = services.BuildServiceProvider(); | ||
#pragma warning disable ASP0000 // We retrieve the Configuration via IOptions here so that it is validated | ||
var parsedConfiguration = serviceProvider.GetRequiredService<IOptions<AdminCliConfiguration>>().Value; | ||
#pragma warning restore ASP0000 | ||
|
||
services.AddEventBus(parsedConfiguration.Infrastructure.EventBus); | ||
services.AddPushNotifications(parsedConfiguration.Modules.Devices.Infrastructure.PushNotifications); | ||
|
||
Modules.Announcements.Application.Extensions.IServiceCollectionExtensions.AddApplication(services); | ||
services.AddDatabase(options => | ||
{ | ||
options.Provider = dbProvider; | ||
options.ConnectionString = dbConnectionString; | ||
}); | ||
|
||
return services; | ||
var containerBuilder = new ContainerBuilder(); | ||
containerBuilder.Populate(services); | ||
var container = containerBuilder.Build(); | ||
return new AutofacServiceProvider(container); | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding these settings into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll adopt it as it's done in AdminApi |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
{ | ||
"Infrastructure": { | ||
"EventBus": { | ||
"SubscriptionClientName": "admincli", | ||
"Vendor": "RabbitMQ", | ||
// possible values: InMemory, RabbitMQ, GoogleCloud, Azure | ||
"ConnectionInfo": "localhost", | ||
"RabbitMQEnableSsl": false, | ||
"RabbitMQUsername": "guest", | ||
// only available for RabbitMQ | ||
"RabbitMQPassword": "guest", | ||
// only available for RabbitMQ | ||
"ConnectionRetryCount": 5, | ||
// only available for RabbitMQ | ||
|
||
"GcpPubSubProjectId": "", | ||
// only available for Google Cloud Pub/Sub | ||
"GcpPubSubTopicName": "" | ||
// only available for Google Cloud Pub/Sub | ||
} | ||
}, | ||
"Modules": { | ||
"Devices": { | ||
"Infrastructure": { | ||
"PushNotifications": { | ||
"Providers": { | ||
"dummy": { | ||
"enabled": true | ||
}, | ||
"sse": { | ||
"enabled": true, | ||
"SseServerBaseAddress": "http://localhost:8083" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary. As far as I know, appsettings.json files are copied by default. At least that's how it works in the other projects (e.g. ConsumerApi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check that