Skip to content

Conversation

@sigridbra
Copy link
Contributor

@sigridbra sigridbra commented Nov 28, 2025

Description

Related Issue(s)

  • #{issue number}

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

Summary by CodeRabbit

  • Refactor
    • Internal code optimizations and improvements to SMS and Kafka integration components for enhanced reliability and maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 28, 2025

📝 Walkthrough

Walkthrough

These changes include: a logger type adjustment in CommonProducer from interface to implementation, error message formatting normalization in KafkaConsumerBase, topic creation logic optimization to filter non-existing topics before iteration, and a constructor parameter change in RequestFilterProcessor requiring non-null dependency injection.

Changes

Cohort / File(s) Summary
Logging format alignment
src/Altinn.Notifications.Sms.Integrations/Consumers/KafkaConsumerBase.cs
Error message placeholder formatting updated to compact token names with removed internal spaces
Logger type and message updates
src/Altinn.Notifications.Sms.Integrations/Producers/CommonProducer.cs
Logger generic type changed from ICommonProducer to CommonProducer; logging message placeholders updated from message/result.Status to Message/Status
Topic creation logic optimization
src/Altinn.Notifications.Sms.Integrations/Producers/CommonProducer.cs
EnsureTopicsExist refactored to filter non-existing topics before iteration, simplified topic creation API usage, moved success logging, and removed per-topic error handling
Dependency injection enforcement
src/Altinn.Notifications.Sms/Telemetry/RequestFilterProcessor.cs
Constructor parameter IHttpContextAccessor changed to require non-null, removing optional default behavior

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

  • Topic creation refactor in CommonProducer — verify filtering logic correctly excludes existing topics and that error handling removal does not mask exceptions
  • Logger type change from interface to implementation — confirm no downstream codebase dependencies or dependency injection configuration conflicts
  • Error handling removal — confirm outer exception propagation is sufficient and no logging is inadvertently lost
  • Constructor parameter non-null enforcement — verify all call sites in RequestFilterProcessor can provide non-null context accessor

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Handle all but one sonar issue' is vague and generic. It doesn't clearly describe what specific sonar issues were fixed or what the main changes are; it uses non-descriptive language that requires context to understand. Revise the title to be more specific about the main changes, such as 'Fix logger generic type and formatting in Kafka consumers and producers' or 'Refactor CommonProducer logger and topic creation logic'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/try-fix-sonar-issues

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link

@sigridbra sigridbra requested a review from a team November 28, 2025 14:15
@sigridbra sigridbra marked this pull request as ready for review November 28, 2025 14:15
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 797af1c and 192e804.

📒 Files selected for processing (3)
  • src/Altinn.Notifications.Sms.Integrations/Consumers/KafkaConsumerBase.cs (1 hunks)
  • src/Altinn.Notifications.Sms.Integrations/Producers/CommonProducer.cs (3 hunks)
  • src/Altinn.Notifications.Sms/Telemetry/RequestFilterProcessor.cs (1 hunks)
🔇 Additional comments (4)
src/Altinn.Notifications.Sms/Telemetry/RequestFilterProcessor.cs (1)

18-21: LGTM! Non-null constructor parameter enforces proper DI contract.

The change correctly requires IHttpContextAccessor at construction time rather than accepting null. The existing null-conditional on HttpContext in OnStart (line 31) remains appropriate since HttpContext itself can be null even when the accessor is not.

src/Altinn.Notifications.Sms.Integrations/Producers/CommonProducer.cs (2)

19-24: LGTM! Logger type correctly uses concrete class.

Using ILogger<CommonProducer> instead of ILogger<ICommonProducer> is the correct pattern for structured logging, as the logger category should reflect the concrete implementation.


57-57: LGTM! Consistent placeholder naming.

Capitalizing {Message} and {Status} aligns with structured logging conventions.

src/Altinn.Notifications.Sms.Integrations/Consumers/KafkaConsumerBase.cs (1)

39-39: LGTM! Proper structured logging format.

Removing spaces from placeholder names ({Class}, {Reason}) ensures correct structured logging behavior and fixes the Sonar issue.

@github-project-automation github-project-automation bot moved this to 🆕 New in Team Core Nov 28, 2025
@sigridbra sigridbra moved this from 🆕 New to 🧾 Review in Team Core Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🧾 Review

Development

Successfully merging this pull request may close these issues.

1 participant