functionality for sbl bridge #547
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive set of changes to enhance the Altinn Correspondence system's functionality, focusing on legacy party processing and configuration management. The changes span multiple components, including Bicep templates, GitHub Actions workflows, application settings, and core services. A new mechanism for processing legacy parties has been implemented, involving the addition of new interfaces, services, repositories, and handlers to manage party information across different system components. Additionally, new parameters and properties have been introduced to improve deployment configurations. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Andreass2
added
kind/feature-request
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (11)
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs (2)
33-33: Use 'LogInformation' for consistencyFor consistency and clarity, use
logger.LogInformationinstead oflogger.Log(LogLevel.Information, ...).Apply this diff to update the logging statement:
-logger.Log(LogLevel.Information, "Party {partyId} added to SBL", partyId); +logger.LogInformation("Party {partyId} added to SBL", partyId);
21-22: Use specific exception typesUsing specific exception types instead of the generic
Exceptionimproves error handling and debugging.Apply this diff to use more specific exceptions:
-throw new Exception("Failed to look up party in Altinn Register"); +throw new InvalidOperationException("Failed to look up party in Altinn Register"); -throw new Exception("Failed to send party to SBL"); +throw new InvalidOperationException("Failed to send party to SBL");Also applies to: 31-32
src/Altinn.Correspondence.Core/Models/Entities/LegacyPartyEntity.cs (1)
7-12: Add XML documentation to explain the purpose of this entityConsider adding XML documentation to explain:
- The purpose of this entity
- The relationship between
IdandPartyId- Any business rules or constraints that apply
+ /// <summary> + /// Represents a legacy party entity in the system. + /// </summary> public class LegacyPartyEntity { + /// <summary> + /// Gets or sets the unique identifier for the legacy party entity. + /// </summary> [Key] public Guid Id { get; set; } + /// <summary> + /// Gets or sets the party identifier from the legacy system. + /// </summary> public int PartyId { get; set; } }src/Altinn.Correspondence.Core/Options/GeneralSettings.cs (1)
9-9: Add URL validation and consider required value handlingThe
AltinnSblBridgeBaseUrlproperty should:
- Include URL format validation
- Consider using
Uritype instead ofstring- Handle the case when the URL is not provided (empty string might not be appropriate)
- public string AltinnSblBridgeBaseUrl { get; set; } = string.Empty; + private Uri? _altinnSblBridgeBaseUrl; + public string AltinnSblBridgeBaseUrl + { + get => _altinnSblBridgeBaseUrl?.ToString() ?? string.Empty; + set => _altinnSblBridgeBaseUrl = !string.IsNullOrEmpty(value) ? new Uri(value) : null; + }src/Altinn.Correspondence.Persistence/Repositories/LegacyPartyRepository.cs (1)
11-15: Consider adding transaction handlingThe AddLegacyPartyId method should use a transaction to ensure data consistency, especially since it involves multiple database operations.
public async Task AddLegacyPartyId(int id, CancellationToken cancellationToken) { + await using var transaction = await _context.Database.BeginTransactionAsync(cancellationToken); + try + { await _context.LegacyParties.AddAsync(new LegacyPartyEntity { PartyId = id }, cancellationToken); await _context.SaveChangesAsync(cancellationToken); + await transaction.CommitAsync(cancellationToken); + } + catch + { + await transaction.RollbackAsync(cancellationToken); + throw; + } }src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeService.cs (2)
24-24: Consider extracting API path to configurationThe API endpoint path is hardcoded. Consider moving it to configuration for better maintainability.
11-15: Consider adding retry policyFor better resilience, consider adding a retry policy for transient failures using Polly.
Example implementation:
using Polly; using Polly.Extensions.Http; public class AltinnSblBridgeService : IAltinnSblBridgeService { private static readonly IAsyncPolicy<HttpResponseMessage> RetryPolicy = HttpPolicyExtensions .HandleTransientHttpError() .WaitAndRetryAsync(3, retryAttempt => TimeSpan.FromSeconds(Math.Pow(2, retryAttempt))); // ... rest of the implementation }src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs (1)
97-97: Consider error handling for the background job.While the fire-and-forget pattern is appropriate here (similar to the Dialogporten activity creation), consider:
- Adding error handling in the
ProcessLegacyPartyHandler- Implementing retry policies for transient failures
- Adding monitoring for failed jobs
src/Altinn.Correspondence.Integrations/DependencyInjection.cs (1)
62-66: LGTM! Consider improving readability.The service registration follows the established pattern and properly handles both production and development scenarios.
Consider reformatting the else clause for better readability:
- else services.AddHttpClient<IAltinnSblBridgeService, AltinnSblBridgeService>(client => client.BaseAddress = new Uri(generalSettings.AltinnSblBridgeBaseUrl)); + else + { + services.AddHttpClient<IAltinnSblBridgeService, AltinnSblBridgeService>(client => + client.BaseAddress = new Uri(generalSettings.AltinnSblBridgeBaseUrl)); + }src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs (1)
349-361: Consider adding an index on PartyId for better query performance.The
LegacyPartyEntitytable will likely be queried using thePartyIdfield. Adding an index on this column would improve query performance.Apply this diff to add an index:
modelBuilder.Entity("Altinn.Correspondence.Core.Models.Entities.LegacyPartyEntity", b => { b.Property<Guid>("Id") .ValueGeneratedOnAdd() .HasColumnType("uuid"); b.Property<int>("PartyId") .HasColumnType("integer"); b.HasKey("Id"); + b.HasIndex("PartyId"); b.ToTable("LegacyParties", "correspondence"); });.github/workflows/ci-cd.yaml (1)
87-87: Remove trailing spaces.There are trailing spaces on lines 87 and 89. While this is a minor issue, it's good practice to maintain clean formatting.
- if: always() && !failure() && !cancelled() + if: always() && !failure() && !cancelled() - permissions: + permissions:Also applies to: 89-89
🧰 Tools
🪛 yamllint (1.35.1)
[error] 87-87: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.azure/applications/api/main.bicep(2 hunks).azure/applications/api/params.bicepparam(1 hunks).azure/modules/containerApp/main.bicep(2 hunks).github/actions/deploy-to-environment/action.yml(2 hunks).github/actions/release-version/action.yml(2 hunks).github/workflows/ci-cd.yaml(3 hunks).github/workflows/deploy-to-environment.yml(1 hunks).github/workflows/publish-branch.yml(1 hunks)src/Altinn.Correspondence.API/appsettings.Development.json(1 hunks)src/Altinn.Correspondence.API/appsettings.json(1 hunks)src/Altinn.Correspondence.Application/DependencyInjection.cs(2 hunks)src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs(1 hunks)src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs(2 hunks)src/Altinn.Correspondence.Core/Models/Entities/LegacyPartyEntity.cs(1 hunks)src/Altinn.Correspondence.Core/Options/GeneralSettings.cs(1 hunks)src/Altinn.Correspondence.Core/Repositories/ILegacyPartyRepository.cs(1 hunks)src/Altinn.Correspondence.Core/Services/IAltinnRegisterService.cs(1 hunks)src/Altinn.Correspondence.Core/Services/IAltinnSblBridgeService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/Events/AltinnEventBus.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterDevService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeDevService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeService.cs(1 hunks)src/Altinn.Correspondence.Integrations/DependencyInjection.cs(1 hunks)src/Altinn.Correspondence.Persistence/Data/ApplicationDbContext.cs(1 hunks)src/Altinn.Correspondence.Persistence/DependencyInjection.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/20241209072946_LegacyPartyId.Designer.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/20241209072946_LegacyPartyId.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs(1 hunks)src/Altinn.Correspondence.Persistence/Repositories/LegacyPartyRepository.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Altinn.Correspondence.API/appsettings.json
🧰 Additional context used
📓 Learnings (3)
src/Altinn.Correspondence.Application/DependencyInjection.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#413
File: src/Altinn.Correspondence.Application/DependencyInjection.cs:0-0
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In Altinn.Correspondence.Application, handlers with similar names—such as UploadAttachmentHandler, GetAttachmentDetailsHandler, GetAttachmentOverviewHandler, and PurgeAttachmentHandler—may have multiple distinct implementations and are intentionally registered separately in DependencyInjection.cs.
src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#483
File: src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs:75-75
Timestamp: 2024-11-12T14:46:34.110Z
Learning: In `LegacyUpdateCorrespondenceStatusHandler.cs`, the call to `_updateCorrespondenceStatusHelper.ReportActivityToDialogporten(...)` is intended to be a fire-and-forget event and does not need to be awaited.
src/Altinn.Correspondence.Core/Models/Entities/LegacyPartyEntity.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#413
File: src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs:17-21
Timestamp: 2024-11-12T05:02:53.065Z
Learning: The `PartyId` and `AuthenticationLevel` properties in the `LegacyUser` class (in `src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs`) are still under development, and the correct validation has not been determined yet.
🪛 actionlint (1.7.4)
.github/workflows/ci-cd.yaml
98-98: missing input "CONTACT_RESERVATION_REGISTRY_BASE_URL" which is required by action "Deploy to Environment" defined at "./.github/actions/deploy-to-environment". all required inputs are "ACCESS_MANAGEMENT_SUBSCRIPTION_KEY", "AZURE_CLIENT_ID", "AZURE_ENVIRONMENT_KEY_VAULT_NAME", "AZURE_NAME_PREFIX", "AZURE_STORAGE_ACCOUNT_NAME", "AZURE_SUBSCRIPTION_ID", "AZURE_TENANT_ID", "AZURE_TEST_ACCESS_CLIENT_ID", "CONTACT_RESERVATION_REGISTRY_BASE_URL", "CORRESPONDENCE_BASE_URL", "DIALOGPORTEN_ISSUER", "IDPORTEN_CLIENT_ID", "IDPORTEN_CLIENT_SECRET", "IDPORTEN_ISSUER", "MASKINPORTEN_CLIENT_ID", "MASKINPORTEN_JWK", "PLATFORM_BASE_URL", "PLATFORM_SUBSCRIPTION_KEY", "SBL_BRIDGE_BASE_URL", "SLACK_URL", "environment", "imageTag"
(action)
🪛 yamllint (1.35.1)
.github/workflows/ci-cd.yaml
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test application
🔇 Additional comments (28)
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs (1)
9-13:⚠️ Potential issueFix the class declaration syntax
The class declaration uses an invalid syntax for constructor parameters in C#. Classes cannot have constructor parameters specified directly in the class declaration. Instead, define a constructor within the class body.
Apply this diff to correct the class declaration:
-public class ProcessLegacyPartyHandler( - ILogger<ProcessLegacyPartyHandler> logger, - IAltinnRegisterService altinnRegisterService, - IAltinnSblBridgeService sblBridgeService, - ILegacyPartyRepository legacyPartyRepository) : IHandler<string, Task> +public class ProcessLegacyPartyHandler : IHandler<string, Task> +{ + private readonly ILogger<ProcessLegacyPartyHandler> logger; + private readonly IAltinnRegisterService altinnRegisterService; + private readonly IAltinnSblBridgeService sblBridgeService; + private readonly ILegacyPartyRepository legacyPartyRepository; + + public ProcessLegacyPartyHandler( + ILogger<ProcessLegacyPartyHandler> logger, + IAltinnRegisterService altinnRegisterService, + IAltinnSblBridgeService sblBridgeService, + ILegacyPartyRepository legacyPartyRepository) + { + this.logger = logger; + this.altinnRegisterService = altinnRegisterService; + this.sblBridgeService = sblBridgeService; + this.legacyPartyRepository = legacyPartyRepository; + }Likely invalid or redundant comment.
src/Altinn.Correspondence.Core/Services/IAltinnSblBridgeService.cs (1)
1-8: Interface 'IAltinnSblBridgeService' looks goodThe interface is well-defined and follows proper conventions.
src/Altinn.Correspondence.Core/Repositories/ILegacyPartyRepository.cs (1)
1-10: Interface 'ILegacyPartyRepository' looks goodThe interface methods are clearly defined and appropriately named.
src/Altinn.Correspondence.Core/Models/Entities/LegacyPartyEntity.cs (1)
11-11: Consider adding validation for PartyIdBased on the retrieved learnings about PartyId validation being under development, consider adding appropriate validation attributes or implementing validation logic for the PartyId property.
Would you like me to help implement the validation rules once they are determined?
src/Altinn.Correspondence.Core/Services/IAltinnRegisterService.cs (1)
7-7: Verify impact of breaking change in LookUpPartyId return typeThe return type change from
Task<string?>toTask<int?>is a breaking change. Ensure all implementations and consumers are updated accordingly.Run this script to find potential impacts:
✅ Verification successful
Breaking change in LookUpPartyId return type has been properly implemented
All implementations and consumers have been updated to handle the
Task<int?>return type correctly:
- Both
AltinnRegisterServiceandAltinnRegisterDevServiceimplementations returnTask<int?>- Consumers in
AltinnEventBusandProcessLegacyPartyHandlerhandle the integer type appropriately🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all files that might be affected by the breaking change # Look for method calls echo "Files potentially calling LookUpPartyId:" rg "LookUpPartyId\(" -l # Look for implementations echo -e "\nPotential implementations of IAltinnRegisterService:" rg "class.*:.*IAltinnRegisterService" -lLength of output: 829
Script:
#!/bin/bash # Check implementations and usage patterns echo "=== Implementation in AltinnRegisterDevService ===" ast-grep --pattern 'async Task<$_> LookUpPartyId($_) { $$$ }' src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterDevService.cs echo -e "\n=== Implementation in AltinnRegisterService ===" ast-grep --pattern 'async Task<$_> LookUpPartyId($_) { $$$ }' src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterService.cs echo -e "\n=== Usage in AltinnEventBus ===" rg -A 5 "LookUpPartyId\(" src/Altinn.Correspondence.Integrations/Altinn/Events/AltinnEventBus.cs echo -e "\n=== Usage in ProcessLegacyPartyHandler ===" rg -A 5 "LookUpPartyId\(" src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.csLength of output: 1529
Script:
#!/bin/bash echo "=== Implementation in AltinnRegisterDevService ===" rg -A 10 "LookUpPartyId" src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterDevService.cs echo -e "\n=== Implementation in AltinnRegisterService ===" rg -A 10 "LookUpPartyId" src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterService.csLength of output: 1364
.azure/applications/api/params.bicepparam (1)
8-8: LGTM!The new parameter follows the existing pattern and is correctly integrated.
src/Altinn.Correspondence.Persistence/DependencyInjection.cs (1)
28-28: LGTM! Repository registration follows established patterns.The registration of
ILegacyPartyRepositoryas a scoped service aligns with the existing repository registration pattern and maintains consistency with other repository lifetimes.src/Altinn.Correspondence.Persistence/Data/ApplicationDbContext.cs (1)
40-40: LGTM! DbSet property follows EF Core conventions.The addition of
LegacyPartiesDbSet property maintains consistency with existing Entity Framework Core patterns and naming conventions.src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterDevService.cs (1)
13-20: LGTM! Type change improves type safety.The change from
Task<string?>toTask<int?>for party IDs is more type-safe and eliminates unnecessary string conversion.Let's verify that this change is consistent across all implementations of
IAltinnRegisterService:✅ Verification successful
Return type is consistent across all implementations
The change to
Task<int?>in AltinnRegisterDevService matches both:
- The interface definition in IAltinnRegisterService
- The implementation in the main AltinnRegisterService
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all implementations of LookUpPartyId to ensure consistent return types ast-grep --pattern 'Task<$_?> LookUpPartyId'Length of output: 46
Script:
#!/bin/bash # Find the interface definition rg "interface IAltinnRegisterService" -A 5 # Find all implementations rg "class.*: .*IAltinnRegisterService" -A 5 # Backup search for LookUpPartyId method rg "LookUpPartyId.*Task<" -A 2Length of output: 2604
Script:
#!/bin/bash # Check AltinnRegisterService implementation rg "LookUpPartyId.*Task<.*>" -B 2 -A 5 src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterService.csLength of output: 120
Script:
#!/bin/bash # Simple search for LookUpPartyId in the main service implementation cat src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterService.csLength of output: 5001
src/Altinn.Correspondence.Application/DependencyInjection.cs (1)
14-14: LGTM! Handler registration follows established patterns.The registration of
ProcessLegacyPartyHandlerand its using directive maintain consistency with existing patterns. Based on previous feedback, separate handler registration is intentional in this codebase.Also applies to: 51-51
src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterService.cs (1)
24-27: LGTM! Type change improves type safety.The change from
Task<string?>toTask<int?>better represents the domain model where PartyId is inherently an integer.Let's verify all callers have been updated to handle the new return type:
✅ Verification successful
All callers properly handle the new
Task<int?>return typeThe codebase has been correctly updated to work with the integer return type, with appropriate null checks and safe type conversions in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of LookUpPartyId to ensure they handle the int? return type rg -A 3 "LookUpPartyId\(" --type csLength of output: 2808
src/Altinn.Correspondence.Integrations/Altinn/Events/AltinnEventBus.cs (1)
33-33: LGTM! Null-safe type conversion.The change correctly handles the new
int?return type fromLookUpPartyIdby using the null-conditional operator andToString().src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs (1)
3-3: LGTM! Clear dependency.The using directive clearly indicates the new dependency on legacy party processing functionality.
src/Altinn.Correspondence.Persistence/Migrations/20241209072946_LegacyPartyId.Designer.cs (1)
1-524: Auto-generated migration designer file.This is an auto-generated file that mirrors the model snapshot. No issues found.
src/Altinn.Correspondence.API/appsettings.Development.json (1)
37-37: LGTM!The new setting
AltinnSblBridgeBaseUrlis correctly placed underGeneralSettingsand follows the existing naming convention..github/workflows/publish-branch.yml (1)
43-43: LGTM!The new secret
SBL_BRIDGE_BASE_URLis correctly referenced and follows the existing naming convention..github/workflows/deploy-to-environment.yml (1)
88-89: LGTM! Secret addition follows existing patterns.The
SBL_BRIDGE_BASE_URLsecret is correctly added and follows the established pattern for secret declarations in the workflow..azure/applications/api/main.bicep (2)
20-21: LGTM! Secure parameter declaration is appropriate.The
sblBridgeBaseUrlis correctly declared with the@secure()decorator, ensuring the URL is handled securely throughout the deployment pipeline.
104-104: LGTM! Parameter correctly passed to containerApp module.The
sblBridgeBaseUrlis properly passed to the containerApp module, maintaining the secure context..github/actions/release-version/action.yml (2)
45-47: LGTM! Input parameter properly defined.The
SBL_BRIDGE_BASE_URLinput parameter is correctly defined with appropriate description and required flag.
83-83: LGTM! Environment variable correctly mapped.The input parameter is properly mapped to the environment variable for deployment.
.github/actions/deploy-to-environment/action.yml (2)
69-71: LGTM! Input parameter properly defined.The
SBL_BRIDGE_BASE_URLinput parameter is correctly defined with appropriate description and required flag.
132-132: LGTM! Parameter correctly passed to release-version action.The
SBL_BRIDGE_BASE_URLis properly passed through to the release-version action..azure/modules/containerApp/main.bicep (2)
14-16: LGTM! Secure parameter properly declared.The
sblBridgeBaseUrlparameter is correctly declared with the@secure()decorator.
84-84: LGTM! Environment variable follows .NET configuration pattern.The environment variable
GeneralSettings__AltinnSblBridgeBaseUrlfollows the correct configuration pattern for .NET applications, using double underscores for section delimiters..github/workflows/ci-cd.yaml (3)
81-81: LGTM! Consistent secret management across environments.The SBL_BRIDGE_BASE_URL secret has been properly added to all deployment environments (test, staging, production), maintaining consistency in the configuration.
Also applies to: 164-164, 208-208
86-86: Environment configuration mismatch.The job is named 'deploy-at22' but uses the 'test' environment. This could lead to confusion and potential deployment issues. Consider creating a dedicated 'at22' environment or clarify why 'test' environment is being used.
83-93: Verify deployment sequence for at22.The new at22 deployment runs in parallel with the test deployment (same needs configuration). Please verify if this is the intended behavior or if at22 should be part of the sequential deployment pipeline (test → at22 → staging → production).
🧰 Tools
🪛 yamllint (1.35.1)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeDevService.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeDevService.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Persistence/Repositories/LegacyPartyRepository.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Persistence/Migrations/20241209072946_LegacyPartyId.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeService.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeDevService.cs (1)
4-5: Add XML documentation to clarify development service purpose.Since this is a development implementation, add XML documentation to clearly indicate its purpose and usage limitations.
+/// <summary> +/// Development implementation of IAltinnSblBridgeService for testing and local development. +/// This implementation should not be used in production environments. +/// </summary> public class AltinnSblBridgeDevService : IAltinnSblBridgeService.github/workflows/ci-cd.yaml (1)
87-87: Remove trailing spaces.Remove trailing spaces from the following lines:
- Line 87:
if: always() && !failure() && !cancelled()- Line 89:
permissions:- Line 122:
CONTACT_RESERVATION_REGISTRY_BASE_URL: ${{ secrets.CONTACT_RESERVATION_REGISTRY_BASE_URL }}Also applies to: 89-89, 122-122
🧰 Tools
🪛 yamllint (1.35.1)
[error] 87-87: trailing spaces
(trailing-spaces)
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs (2)
15-15: Remove unused parameter.The
ClaimsPrincipal? userparameter is not used in the method implementation.- public async Task Process(string recipient, ClaimsPrincipal? user, CancellationToken cancellationToken) + public async Task Process(string recipient, CancellationToken cancellationToken)
17-17: Use consistent logging levels.The logging levels should be consistent. Both logs are informational, but use different methods.
- logger.LogInformation("Process legacy party {recipient}", recipient); + logger.LogInformation("Processing legacy party {recipient}", recipient); - logger.Log(LogLevel.Information, "Party {partyId} added to SBL", partyId); + logger.LogInformation("Party {partyId} added to SBL", partyId);Also applies to: 31-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ci-cd.yaml(3 hunks)src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeDevService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeService.cs(1 hunks)src/Altinn.Correspondence.Integrations/DependencyInjection.cs(3 hunks)src/Altinn.Correspondence.Persistence/Migrations/20241209072946_LegacyPartyId.cs(1 hunks)src/Altinn.Correspondence.Persistence/Repositories/LegacyPartyRepository.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Altinn.Correspondence.Persistence/Repositories/LegacyPartyRepository.cs
- src/Altinn.Correspondence.Integrations/DependencyInjection.cs
- src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeService.cs
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci-cd.yaml
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 122-122: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test application
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
.github/workflows/ci-cd.yaml (2)
81-81: LGTM! SBL bridge URL configuration added consistently.The SBL_BRIDGE_BASE_URL secret has been properly added to all deployment environments, which aligns with the PR objective of enabling SBL bridge functionality for correspondence marking.
Also applies to: 165-165, 209-209
97-120: Missing required input parameter.The deploy-to-environment action requires CONTACT_RESERVATION_REGISTRY_BASE_URL, but it's not provided in the inputs.
src/Altinn.Correspondence.Persistence/Migrations/20241209072946_LegacyPartyId.cs (1)
14-33: LGTM! Well-structured migration with proper constraints.The migration correctly implements:
- UUID primary key
- Integer PartyId with unique constraint
- Index on PartyId for query optimization
- Proper schema naming
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs (1)
26-33: Verify transaction handling.Ensure that adding the party to both SBL bridge and legacy repository is transactional to maintain data consistency.
Run the following script to check for transaction handling in the repository implementation:
✅ Verification successful
Current implementation maintains data consistency.
The implementation is safe because:
- SBL bridge operation fails fast before any DB changes
- DB operation is atomic using EF Core's SaveChangesAsync
- No partial/inconsistent state is possible with current operation order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for transaction handling in repository implementation # Look for transaction handling in repository implementation rg -A 10 "class LegacyPartyRepository" # Look for transaction handling in DbContext rg -A 10 "SaveChangesAsync"Length of output: 16423
src/Altinn.Correspondence.Integrations/Altinn/SblBridge/AltinnSblBridgeDevService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cs
Show resolved
Hide resolved
Description
When a correspondence is published, we mark it in sbl bridge as having Altinn3Messages.
Occurs only the first time for a given org or SSN
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Infrastructure
Configuration
Development