Added jwt generator #101
Added jwt generator #101
Conversation
WalkthroughThis update introduces OpenAI integration and refactors several data structures and query parameter names for clarity and consistency. It adds new records and interfaces to support OpenAI report generation and analysis, including service implementations, MediatR handlers, and supporting data models. The environment and configuration files are updated to include OpenAI API credentials. The salary data model is unified and extended, and the property Changes
Sequence Diagram(s)OpenAI Report Analysis Request FlowsequenceDiagram
participant Client
participant Controller as TelegramSubscriptionsController
participant Mediator
participant Handler as GetOpenAiReportAnalysisHandler
participant Db as DatabaseContext
participant ProfCache as IProfessionsCacheService
participant OpenAi as IOpenAiService
Client->>Controller: GET /{id}/open-ai-analysis
Controller->>Mediator: Send(GetOpenAiReportAnalysisQuery)
Mediator->>Handler: Handle(query)
Handler->>Db: Query StatDataChangeSubscription by id
Handler->>ProfCache: GetAllProfessionsAsync()
Handler->>OpenAi: GetAnalysisAsync()
Handler->>OpenAi: GetBearer()
Handler-->>Mediator: GetOpenAiReportAnalysisResponse
Mediator-->>Controller: Response
Controller-->>Client: 200 OK + Analysis Response
OpenAI Service Bearer and AnalysissequenceDiagram
participant OpenAiService
participant JwtGen as TechinterviewJwtTokenGenerator
participant HttpClient
OpenAiService->>JwtGen: Generate JWT (GetBearer)
OpenAiService->>HttpClient: GET OpenAI API (GetAnalysisAsync)
HttpClient-->>OpenAiService: Response
OpenAiService-->>Caller: Analysis string
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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 (24)
src/Web.Api/Views/ChartSharePageViewModel.cs (1)
67-69: Consider renaming internal properties for consistencyWhile the external references have been updated, the internal property is still named
ProfessionsToInclude. Consider renaming this and related methods likeHasProfessionsToIncludeto maintain consistency across the codebase.- public string ProfessionsToInclude { get; init; } + public string SelectedProfessionIds { get; init; } - public bool HasProfessionsToInclude => !string.IsNullOrEmpty(ProfessionsToInclude); + public bool HasSelectedProfessionIds => !string.IsNullOrEmpty(SelectedProfessionIds);src/Web.Api/Features/Telegram/ProcessMessage/UserCommands/TelegramBotUserCommandParameters.cs (1)
29-37: Consider renaming the constructor parameter for better consistencyFor full naming consistency, consider renaming the parameter
professionsToIncludetoselectedProfessionsto match the property names.public TelegramBotUserCommandParameters( - List<Profession> professionsToInclude) + List<Profession> selectedProfessions) { Grade = null; - SelectedProfessions = professionsToInclude; - SelectedProfessionIds = professionsToInclude + SelectedProfessions = selectedProfessions; + SelectedProfessionIds = selectedProfessions .Select(x => x.Id) .ToList(); }src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReportMetadata.cs (1)
6-21: Consider enhancing flexibility and standardizing currency formatThe implementation is good for the current requirements, but consider these improvements:
- The hardcoded
PeriodTypevalue "weekly" might limit flexibility. Consider accepting this as a parameter.- For currency representation, consider using ISO 4217 standard format (e.g., "USD" instead of "Usd").
public record OpenAiBodyReportMetadata { public string ReportDate { get; } public string Currency { get; } public string PeriodType { get; } public OpenAiBodyReportMetadata( - Currency currency) + Currency currency, + string periodType = "weekly") { - Currency = currency.ToString(); + Currency = currency.ToString().ToUpperInvariant(); ReportDate = DateTime.UtcNow.ToString("yyyy-MM-dd"); - PeriodType = "weekly"; + PeriodType = periodType; } }src/InfrastructureTests/Jwt/TechinterviewJwtTokenGeneratorTests.cs (1)
6-17: Test looks good, but could be more comprehensiveThe test validates that the token generator produces a string containing the expected JWT header format. This is a good start, but consider adding more assertions to validate other aspects of the token.
You could enhance the test by:
- Validating token structure (header, payload, signature)
- Checking that specific claims are present
- Testing with different input parameters
[Fact] public void ToString_ShouldReturnToken() { const string secretKey = "hey-girl-its-super-secret-key-for-jwt"; var generator = new TechinterviewJwtTokenGenerator(secretKey); var result = generator.ToString(); Assert.Contains("eyJhbGciOiJodHRwOi8vd3d3LnczLm9yZy8y", result); + + // Validate token structure (3 parts separated by dots) + var parts = result.Split('.'); + Assert.Equal(3, parts.Length); }src/Web.Api/Features/Telegram/ProcessMessage/ProcessTelegramMessageHandler.cs (1)
278-368: Method could benefit from further refactoringWhile the changes look good, the
ReplyWithSalariesAsyncmethod is quite long (about 90 lines). Consider breaking it down into smaller, focused methods to improve readability and maintainability.You could extract the following logic into separate methods:
- Cache key generation
- Salary data retrieval
- Currency formatting
- Response text building
src/Infrastructure/Services/OpenAi/OpenAiService.cs (1)
24-28: Consider makingGetBearer()privateThe
GetBearer()method appears to only be used internally within this class (inGetAnalysisAsync). Consider making it private to better encapsulate the service's implementation details.-public string GetBearer() +private string GetBearer() { var secret = _configuration["OpenAiApi:Secret"]; return new TechinterviewJwtTokenGenerator(secret).ToString(); }src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReport.cs (1)
17-23: Handle edge cases with null or empty profession listsThe code doesn't explicitly handle the case where
subscriptionData.FilterData.SelectedProfessionsis null or empty, which could lead to an empty roles list.Consider adding a check and potentially a log message or default behavior when no professions are selected:
_subscriptionData = subscriptionData.IsInitializedOrFail(); ReportMetadata = new OpenAiBodyReportMetadata(currency); Roles = new List<OpenAiBodyReportRole>(); var now = DateTimeOffset.UtcNow; +if (subscriptionData.FilterData.SelectedProfessions == null || !subscriptionData.FilterData.SelectedProfessions.Any()) +{ + // Log warning or set default behavior +}src/Web.Api/Features/Subscribtions/GetOpenAiReport/GetOpenAiReportHandler.cs (2)
15-26: Useclassinstead ofrecordfor DI-managed handlers.
recordtypes generate value-based equality members and deconstruction helpers that are unnecessary (and sometimes surprising) for services instantiated by the DI container.
Changing this tosealed class(orclass) avoids unintended semantics and makes the intent clearer.
13-13: Spelling:Subscribtions→Subscriptions.A consistent namespace spelling helps discoverability and avoids typos in future imports.
src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReportRole.cs (1)
22-33: Weekly slices overlap; consider using non-overlapping intervals.
Where(x => x.CreatedAt <= now.AddDays(-daysCount))selects all salaries older than n days, so each successive interval includes data from earlier ones, skewing averages and trends.
A typical approach is to use half-open ranges, e.g.:var from = now.AddDays(-daysCount); var to = now.AddDays(-i * 7); var salariesForDate = salaries .Where(x => x.CreatedAt > from && x.CreatedAt <= to) .ToList();This yields three distinct one-week buckets (7–14, 14–21, 21–28 days ago).
src/Web.Api/Features/BackgroundJobs/StatDataChangeSubscriptionCalculateJob.cs (1)
118-124: Simplify zero-difference check.
if (diffInPercent is > 0 or < 0)is equivalent todiffInPercent != 0and is easier to read.- if (diffInPercent is > 0 or < 0) + if (diffInPercent != 0)src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisHandler.cs (4)
20-20: Inconsistent naming convention between field and parameter.The field name
_openApiServicedoesn't match the parameter nameopenApiServiceon line 25 (case mismatch). Consistency in naming is important for code readability and maintainability.- private readonly IOpenAiService _openApiService; + private readonly IOpenAiService _openAiService;
25-25: Field name doesn't match parameter.The parameter name uses 'Ai' while the field uses 'Api'. Update for consistency.
IOpenAiService openApiService) { _context = context; _professionsCacheService = professionsCacheService; - _openApiService = openApiService; + _openAiService = openApiService; }Also applies to: 29-29
16-16: Class lacks XML documentation.Adding XML documentation for public classes and methods improves code maintainability. Consider documenting this handler class with a summary of its purpose and responsibility.
+/// <summary> +/// Handles requests to retrieve an OpenAI report analysis for a specific subscription. +/// </summary> public class GetOpenAiReportAnalysisHandler : IRequestHandler<GetOpenAiReportAnalysisQuery, GetOpenAiReportAnalysisResponse>
32-34: Method lacks XML documentation.Add XML documentation for the Handle method to describe its purpose, parameters, and return value.
+ /// <summary> + /// Handles the request to get an OpenAI report analysis for the specified subscription. + /// </summary> + /// <param name="request">The query containing the subscription ID.</param> + /// <param name="cancellationToken">Cancellation token.</param> + /// <returns>A response containing the OpenAI analysis, report data, and bearer token.</returns> public async Task<GetOpenAiReportAnalysisResponse> Handle( GetOpenAiReportAnalysisQuery request, CancellationToken cancellationToken)src/Web.Api/Features/BackgroundJobs/SalarySubscriptionData.cs (6)
22-22: Fix typo in variable name.The variable
_isInitianlizedhas a spelling error and should be_isInitialized.- private bool _isInitianlized; + private bool _isInitialized;
42-42: Fix typo consistently throughout the code.The misspelled variable
_isInitianlizedis used in multiple places. Fix all occurrences to maintain consistency.- _isInitianlized = false; + _isInitialized = false; // Line 83 - _isInitianlized = true; + _isInitialized = true; // Line 89 - if (_isInitianlized) + if (_isInitialized)Also applies to: 83-83, 89-89
44-50: Consider using more explicit filtering logic.The use of the
Whenextension method with multiple conditions makes the code less obvious. Consider using more explicit filtering for better readability.- FilterData = new TelegramBotUserCommandParameters( - allProfessions - .When( - subscription.ProfessionIds != null && - subscription.ProfessionIds.Count > 0, - x => subscription.ProfessionIds.Contains(x.Id)) - .ToList()); + var filteredProfessions = allProfessions; + if (subscription.ProfessionIds != null && subscription.ProfessionIds.Count > 0) + { + filteredProfessions = allProfessions + .Where(x => subscription.ProfessionIds.Contains(x.Id)) + .ToList(); + } + + FilterData = new TelegramBotUserCommandParameters(filteredProfessions);
16-16: Class lacks XML documentation.Adding XML documentation for public classes improves code maintainability. Consider documenting this record class with a summary of its purpose and responsibility.
+/// <summary> +/// Encapsulates salary subscription data management for background jobs and report generation. +/// Provides methods for loading and accessing salary data filtered by professions. +/// </summary> public record SalarySubscriptionData
58-60: Add XML documentation for public methods.Public methods should have XML documentation to improve code understandability.
+ /// <summary> + /// Asynchronously initializes the subscription data by loading cache records, + /// calculating total salary count, and retrieving salary data with grades. + /// </summary> + /// <param name="cancellationToken">A token to cancel the operation.</param> + /// <returns>The current instance for fluent chaining.</returns> public async Task<SalarySubscriptionData> InitializeAsync( CancellationToken cancellationToken)
74-81: Consider creating a mapper function for SalaryBaseData.The inline creation of SalaryBaseData objects could be encapsulated in a separate mapper method for better reusability and testing.
Salaries = await _salariesForChartQuery .ToQueryable(CompanyType.Local) .Where(x => x.Grade.HasValue) - .Select(x => new SalaryBaseData - { - ProfessionId = x.ProfessionId, - Grade = x.Grade.Value, - Value = x.Value, - CreatedAt = x.CreatedAt, - }) + .Select(MapToSalaryBaseData) .ToListAsync(cancellationToken); + private static SalaryBaseData MapToSalaryBaseData(/* appropriate input type here */) + { + return new SalaryBaseData + { + ProfessionId = /* source */.ProfessionId, + Grade = /* source */.Grade.Value, + Value = /* source */.Value, + CreatedAt = /* source */.CreatedAt, + }; + }src/Infrastructure/Services/OpenAi/IOpenAiService.cs (3)
3-9: Improve interface design clarity and documentationWhile the interface is concise, there are several areas for improvement:
Method naming could be more descriptive:
GetBearer()would be clearer asGenerateJwtToken()orGetAuthorizationToken()GetAnalysisAsync()doesn't indicate what is being analyzedThe
GetAnalysisAsync()method lacks parameters that would explain what data is being analyzed or what kind of analysis is performedConsider adding XML documentation comments to explain the purpose and behavior of these methods
For consistency, if
GetBearer()performs any I/O operations, consider making it asynchronousnamespace Infrastructure.Services.OpenAi; +/// <summary> +/// Provides functionality for interacting with OpenAI services +/// </summary> public interface IOpenAiService { + /// <summary> + /// Generates a JWT bearer token for authenticating with the OpenAI service + /// </summary> + /// <returns>A JWT token string</returns> string GetBearer(); + /// <summary> + /// Retrieves AI analysis for salary data + /// </summary> + /// <param name="cancellationToken">Optional cancellation token</param> + /// <returns>Analysis results as a string</returns> Task<string> GetAnalysisAsync( CancellationToken cancellationToken = default); }
5-5: Consider type safety for authentication tokenThe
GetBearer()method returns a simple string. Consider creating a dedicated type to represent the JWT token for better type safety and to make the token's purpose more explicit in the code.
7-8: Consider structured return type for analysis resultsThe
GetAnalysisAsync()method returns a simple string for the analysis results. Consider creating a dedicated response type that provides structure to the analysis data, which would improve type safety and make the structure of the analysis results explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
src/.env.example(1 hunks)src/Domain/Entities/StatData/SalaryBaseData.cs(1 hunks)src/Domain/Entities/StatData/SalaryGraveValue.cs(0 hunks)src/Domain/Entities/StatData/StatDataCacheItemSalaryData.cs(2 hunks)src/Domain/ValueObjects/ISalariesChartQueryParams.cs(2 hunks)src/Infrastructure/Currencies/Contracts/ICurrencyService.cs(1 hunks)src/Infrastructure/Currencies/CurrencyService.cs(1 hunks)src/Infrastructure/Infrastructure.csproj(1 hunks)src/Infrastructure/Jwt/TechinterviewJwtTokenGenerator.cs(1 hunks)src/Infrastructure/Salaries/ChartPageLink.cs(1 hunks)src/Infrastructure/Salaries/SalariesForChartQuery.cs(3 hunks)src/Infrastructure/Services/OpenAi/IOpenAiService.cs(1 hunks)src/Infrastructure/Services/OpenAi/OpenAiService.cs(1 hunks)src/InfrastructureTests/Jwt/TechinterviewJwtTokenGeneratorTests.cs(1 hunks)src/TestUtils/Mocks/CurrenciesServiceFake.cs(1 hunks)src/Web.Api.Tests/Features/Telegram/ProcessMessage/UserCommands/QaAndTestersTelegramBotUserCommandParametersTests.cs(1 hunks)src/Web.Api/Features/BackgroundJobs/SalarySubscriptionData.cs(1 hunks)src/Web.Api/Features/BackgroundJobs/StatDataChangeSubscriptionCalculateJob.cs(5 hunks)src/Web.Api/Features/Historical/GetSurveyHistoricalChart/GetSurveyHistoricalChartHandler.cs(1 hunks)src/Web.Api/Features/Historical/HistoricalChartsController.cs(2 hunks)src/Web.Api/Features/Salaries/Admin/SalariesAdminQuery.cs(1 hunks)src/Web.Api/Features/Salaries/GetSalaries/GetSalariesPaginatedQuery.cs(2 hunks)src/Web.Api/Features/Salaries/GetSalariesChart/Charts/SalariesChartQueryParams.cs(2 hunks)src/Web.Api/Features/Salaries/SalariesChartQueryParamsBase.cs(2 hunks)src/Web.Api/Features/Salaries/SalariesController.cs(2 hunks)src/Web.Api/Features/SalariesChartShare/GetChartSharePage/GetChartSharePageHandler.cs(1 hunks)src/Web.Api/Features/Subscribtions/GetOpenAiReport/GetOpenAiReportHandler.cs(1 hunks)src/Web.Api/Features/Subscribtions/GetOpenAiReport/GetOpenAiReportQuery.cs(1 hunks)src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisHandler.cs(1 hunks)src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisQuery.cs(1 hunks)src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisResponse.cs(1 hunks)src/Web.Api/Features/Subscribtions/TelegramSubscriptionsController.cs(2 hunks)src/Web.Api/Features/Telegram/ProcessMessage/ProcessTelegramMessageHandler.cs(2 hunks)src/Web.Api/Features/Telegram/ProcessMessage/UserCommands/TelegramBotUserCommandParameters.cs(2 hunks)src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReport.cs(1 hunks)src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReportMetadata.cs(1 hunks)src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReportRole.cs(1 hunks)src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReportRoleHistoricalDataItem.cs(1 hunks)src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReportRoleSalaryData.cs(1 hunks)src/Web.Api/Properties/launchSettings.json(1 hunks)src/Web.Api/Setup/ServiceRegistration.cs(2 hunks)src/Web.Api/Views/ChartSharePageViewModel.cs(1 hunks)src/Web.Api/appsettings.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/Domain/Entities/StatData/SalaryGraveValue.cs
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/Infrastructure/Services/OpenAi/IOpenAiService.cs (1)
src/Infrastructure/Services/OpenAi/OpenAiService.cs (1)
GetBearer(24-28)
src/Infrastructure/Currencies/Contracts/ICurrencyService.cs (1)
src/Infrastructure/Currencies/Contracts/CurrencyContent.cs (3)
CurrencyContent(11-18)CurrencyContent(20-35)CurrencyContent(37-45)
src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisResponse.cs (1)
src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReport.cs (1)
OpenAiBodyReport(13-35)
src/Domain/Entities/StatData/SalaryBaseData.cs (1)
src/Web.Api/Features/Import/ImportKolesaCsv/KolesaDeveloperCsvLine.cs (1)
DeveloperGrade(45-55)
src/Web.Api/Features/Telegram/ProcessMessage/UserCommands/TelegramBotUserCommandParameters.cs (4)
src/Web.Api/Features/Import/ImportKolesaCsv/KolesaDeveloperCsvLine.cs (1)
DeveloperGrade(45-55)src/Domain/Entities/Salaries/Profession.cs (1)
List(39-45)src/Web.Api/Features/Telegram/ProcessMessage/UserCommands/ProductManagersTelegramBotUserCommandParameters.cs (1)
List(22-36)src/Web.Api/Features/Telegram/ProcessMessage/UserCommands/QaAndTestersTelegramBotUserCommandParameters.cs (1)
List(34-49)
src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisQuery.cs (1)
src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisResponse.cs (1)
GetOpenAiReportAnalysisResponse(7-15)
src/InfrastructureTests/Jwt/TechinterviewJwtTokenGeneratorTests.cs (1)
src/Infrastructure/Jwt/TechinterviewJwtTokenGenerator.cs (2)
TechinterviewJwtTokenGenerator(14-18)ToString(20-23)
src/Web.Api/Features/Subscribtions/GetOpenAiReport/GetOpenAiReportQuery.cs (1)
src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReport.cs (1)
OpenAiBodyReport(13-35)
src/Web.Api/Features/Subscribtions/GetOpenAiReport/GetOpenAiReportHandler.cs (3)
src/Web.Api/Features/Subscribtions/GetOpenAiReport/GetOpenAiReportQuery.cs (1)
GetOpenAiReportQuery(9-13)src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReport.cs (1)
OpenAiBodyReport(13-35)src/Web.Api/Features/BackgroundJobs/SalarySubscriptionData.cs (3)
Task(58-85)SalarySubscriptionData(34-56)SalarySubscriptionData(87-96)
🔇 Additional comments (43)
src/Infrastructure/Infrastructure.csproj (1)
24-24: Added JWT library to support token generation for OpenAI integration.The addition of the
System.IdentityModel.Tokens.Jwtpackage aligns with the PR objective of adding JWT token generation capabilities. This package provides the necessary functionality for creating and managing JWT tokens that will be used to authenticate requests to the OpenAI API.src/.env.example (1)
4-4: LGTM: Added OpenAI API secret environment variable.The addition of the
OpenAiApi__Secretenvironment variable is appropriate for the OpenAI integration. This follows good security practices by storing API credentials in environment variables instead of hardcoding them in the application.src/Web.Api/Features/SalariesChartShare/GetChartSharePage/GetChartSharePageHandler.cs (1)
41-41: Property renaming for consistency.The change from
ProfessionsToIncludetoSelectedProfessionIdsis consistent with a broader renaming effort across the codebase. This improves naming clarity and consistency.Consider also renaming the local variable
professionsToIncludeto match the new property name for better readability:-var professionsToInclude = new DeveloperProfessionsCollection(request.SelectedProfessionIds).ToList(); +var selectedProfessionIds = new DeveloperProfessionsCollection(request.SelectedProfessionIds).ToList();And updating the usage:
-SelectedProfessionIds = professionsToInclude, +SelectedProfessionIds = selectedProfessionIds,Also applies to: 46-46
src/Domain/ValueObjects/ISalariesChartQueryParams.cs (2)
12-12: Improved property naming for better clarity.Renaming from
ProfessionsToIncludetoSelectedProfessionIdsprovides better clarity about what the property contains (IDs of selected professions rather than profession objects).
25-25: Correctly updated HasAnyFilter logic to match property rename.The property rename was consistently applied in the HasAnyFilter expression, maintaining the same logical behavior.
src/Web.Api/Features/Historical/GetSurveyHistoricalChart/GetSurveyHistoricalChartHandler.cs (1)
104-108: Consistent property renaming in filter logic.The filter conditions correctly use the renamed property
SelectedProfessionIdsboth for checking the count and filtering profession IDs, maintaining the original behavior.src/Web.Api.Tests/Features/Telegram/ProcessMessage/UserCommands/QaAndTestersTelegramBotUserCommandParametersTests.cs (1)
24-27: Updated test assertions to match renamed property.Test assertions now correctly reference the renamed
SelectedProfessionIdsproperty, ensuring the tests validate the same behavior as before.src/Web.Api/Features/Salaries/Admin/SalariesAdminQuery.cs (1)
61-61: Updated query filter logic to use renamed property.The filter clause in
ApplyFiltersmethod now correctly usesSelectedProfessionIdsfor consistency with the interface changes, maintaining the same filtering behavior.src/Infrastructure/Salaries/ChartPageLink.cs (1)
48-50: Property name refactoring looks good.The change from
ProfessionsToIncludetoSelectedProfessionIdsis consistent with the broader system-wide renaming mentioned in the summary. This standardizes the naming convention for profession filters throughout the application.src/Web.Api/Setup/ServiceRegistration.cs (2)
30-41: Good refactoring to method chaining style.The refactoring to method chaining makes the service registration more concise and readable while maintaining the same functionality.
37-37: OpenAI service registration aligns with PR objectives.The registration of
IOpenAiServicewith implementationOpenAiServiceproperly integrates the new OpenAI functionality mentioned in the PR objectives about JWT token generation.src/Infrastructure/Currencies/CurrencyService.cs (1)
32-32: Method renaming improves clarity.Renaming
GetCurrencyAsynctoGetCurrencyOrNullAsyncmakes the API more self-documenting by explicitly indicating that the method may return null when a currency is not found.src/TestUtils/Mocks/CurrenciesServiceFake.cs (1)
41-41: Consistent method renaming in test mock.The mock implementation correctly mirrors the interface change from
GetCurrencyAsynctoGetCurrencyOrNullAsync, maintaining consistency across the codebase.src/Web.Api/Features/Salaries/SalariesChartQueryParamsBase.cs (1)
18-18: Consistent property renaming across the codebase.The renaming of
ProfessionsToIncludetoSelectedProfessionIdsis consistently applied here as well, maintaining the improved naming convention observed in the first file. This consistency across related classes is excellent for code maintainability and readability. The external API contract is preserved through the unchanged[FromQuery(Name = "profsInclude")]attribute.Also applies to: 36-36, 41-41
src/Web.Api/Features/Salaries/GetSalariesChart/Charts/SalariesChartQueryParams.cs (3)
18-18: Good rename for improved clarityThe change from
ProfessionsToIncludetoSelectedProfessionIdsimproves naming clarity while maintaining API compatibility through the unchangedprofsIncludequery parameter name.
35-36: Consistent property renamingThe update in
HasAnyFilterproperty is consistent with the renamed property.
41-41: Consistent property renaming in methodThe
GetKeyPostfixmethod correctly uses the renamed property.src/Domain/Entities/StatData/SalaryBaseData.cs (1)
6-15: Well-designed record for salary dataThis new record is well-structured with appropriate property types:
- Nullable
ProfessionIdallows for cases without an associated professionGradeuses the appropriate enum typeValueas double is suitable for salary amountsCreatedAtusing DateTimeOffset is appropriate for timestampsThe use of init-only properties promotes immutability, which is a good practice for data models.
src/Web.Api/Features/Historical/HistoricalChartsController.cs (2)
35-35: Consistent property renamingThe rename from
ProfessionsToIncludetoSelectedProfessionIdsis consistent with changes in other files while maintaining the same functionality.
56-56: Consistent property renamingThe rename from
ProfessionsToIncludetoSelectedProfessionIdsis consistent with changes in other files while maintaining the same functionality.src/Web.Api/Views/ChartSharePageViewModel.cs (2)
27-27: Consistent property reference updateThe condition now correctly references the renamed property from
requestParams.
30-30: Consistent property reference in URL generationThe URL parameter generation now correctly uses the renamed property while maintaining the same URL parameter name for backward compatibility.
src/Web.Api/Features/Subscribtions/GetOpenAiReport/GetOpenAiReportQuery.cs (1)
7-16: LGTM - Simple, focused MediatR query recordThe record is well-structured with an immutable property and follows good practice for MediatR requests. The query properly encapsulates the single parameter needed to fetch an OpenAI report for a specific subscription.
src/Web.Api/Features/Telegram/ProcessMessage/UserCommands/TelegramBotUserCommandParameters.cs (3)
34-36: LGTM - Property initialization updated correctlyThe initialization of the
SelectedProfessionIdsproperty has been properly updated to match the renamed property.
41-41: LGTM - Property renamed for consistencyThe property has been renamed from
ProfessionsToIncludetoSelectedProfessionIdsto maintain consistency with the naming conventions used across the codebase.
58-58: LGTM - Method updated to use renamed propertyThe
GetKeyPostfixmethod has been properly updated to use the renamedSelectedProfessionIdsproperty.src/Web.Api/Features/Salaries/SalariesController.cs (2)
72-72: Property name consistently updatedThe property name has been renamed from
ProfessionsToIncludetoSelectedProfessionIdsfor improved clarity and consistency with the rest of the codebase.
93-93: Property name consistently updatedThe property name has been renamed from
ProfessionsToIncludetoSelectedProfessionIdsfor improved clarity and consistency with the rest of the codebase.src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisQuery.cs (1)
6-15: Well-structured query recordThe query record follows best practices:
- Properly implements MediatR's
IRequestinterface- Uses immutable design with a constructor-initialized property
- Clear and descriptive naming
src/Web.Api/Features/Telegram/ProcessMessage/ProcessTelegramMessageHandler.cs (3)
301-307: Updated data model with better structureThe code now uses
SalaryBaseDatainstead of the previous data type, adding important properties likeProfessionIdandCreatedAt. This aligns with the domain model changes in the rest of the codebase.
322-324: Improved null handling in currency serviceMethod name changed from
GetCurrencyAsynctoGetCurrencyOrNullAsync, which better reflects that the method can return null, improving code readability and intent clarity.
328-328: Using centralized grade groups collectionThe code now uses the static collection
StatDataCacheItemSalaryData.GradeGroupsForRegularStatsinstead of a locally defined collection. This improves consistency and maintainability.src/Domain/Entities/StatData/StatDataCacheItemSalaryData.cs (3)
21-28: Constructor signature updated to match domain model changesThe constructor now accepts
List<SalaryBaseData>instead of the previousSalaryGraveValuetype, aligning with the broader domain model refactoring described in the summary. The extraction of salary values for calculation is done efficiently.
32-33: Added min/max salary calculationsNew properties to track minimum and maximum salary values have been added with proper null handling when the list is empty.
59-61: New properties for min/max salary valuesThe new nullable properties properly complement the existing salary statistics and will provide additional insights for data analysis.
src/Web.Api/Integrations/OpenAiAnalysisIntegration/OpenAiBodyReportRoleSalaryData.cs (1)
8-43: Well-structured salary data record for OpenAI reportingThe record is well-implemented with:
- Proper empty list handling
- Clear separation of input processing and property storage
- Appropriate usage of nullable types for min/max values
- Consistent extraction of salary values using the same approach as in StatDataCacheItemSalaryData
This seems to be part of a new OpenAI reporting feature and follows good practices for data encapsulation.
src/Web.Api/Features/Subscribtions/TelegramSubscriptionsController.cs (2)
12-12: Added using statement for OpenAI integrationThe import has been correctly added to support the new OpenAI analysis functionality.
88-97: New endpoint for OpenAI analysis looks goodThe implementation follows the controller's existing patterns for endpoint structure and mediator usage.
src/Infrastructure/Salaries/SalariesForChartQuery.cs (2)
15-15: LGTM: Good use of constants to avoid magic numbersCreating a named constant for the previously hardcoded value
18is a good practice that improves code readability and maintainability.
74-74: LGTM: Consistent use of the new constantGood job on using the newly defined
MonthsToShowconstant here instead of the hardcoded value.src/Web.Api/Features/Salaries/GetSalaries/GetSalariesPaginatedQuery.cs (2)
20-20: Property name improved for better semantic clarityRenaming from
ProfessionsToIncludetoSelectedProfessionIdsbetter reflects that this property contains IDs rather than profession objects. Good change that maintains API compatibility by keeping the external query parameter name asprofsInclude.
38-38: References consistently updatedAll references to the renamed property are consistently updated throughout the class, including the
HasAnyFilterproperty and theGetKeyPostfix()method.Also applies to: 43-43
src/Infrastructure/Currencies/Contracts/ICurrencyService.cs (1)
7-7: Method name improved to clarify nullabilityRenaming from
GetCurrencyAsynctoGetCurrencyOrNullAsyncexplicitly indicates that the method may return null, making the nullability contract clearer to callers. This helps prevent potential null reference exceptions by encouraging proper null checking.
| "ConnectionStrings__Elasticsearch": "http://localhost:9200", | ||
| "GenerateStubData": "true" | ||
| "GenerateStubData": "true", | ||
| "OpenAiApi__Secret": "my-secret" |
There was a problem hiding this comment.
Remove hardcoded secret value from launchSettings.json.
While this is a development configuration, including even placeholder secrets in files that are committed to version control is a security risk. If the repository were ever to become public or be compromised, this would expose the secret pattern.
Consider replacing the literal value with a placeholder notation that clearly indicates a value needs to be supplied:
-"OpenAiApi__Secret": "my-secret"
+"OpenAiApi__Secret": "REPLACE_WITH_ACTUAL_SECRET"Alternatively, consider using user secrets for development environments:
dotnet user-secrets set "OpenAiApi:Secret" "your-actual-secret"There was a problem hiding this comment.
That's ok, I am going to replace it in Production.json file
There was a problem hiding this comment.
@maximgorbatyuk Understood! As long as you're planning to handle the secret properly in the production environment, that's what matters most. Just remember that the development file is still typically committed to version control, but it's your call on how to manage development secrets.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| "OpenAiApi": { | ||
| "Url": "https://wf.belyaev.live/webhook/e69fa36a-adff-410c-8cd0-04ec833d849e", | ||
| "Secret": "my-secret" | ||
| }, |
There was a problem hiding this comment.
Security concern: Hardcoded API configuration should be moved to environment-specific settings.
The OpenAI API configuration contains two issues:
- A hardcoded secret value (
"my-secret") should never be included in appsettings.json as it will be committed to source control. - The hardcoded URL contains what appears to be a specific instance identifier (
e69fa36a-adff-410c-8cd0-04ec833d849e), which may need to change between environments.
Consider using placeholder values and loading the actual values from environment variables or user secrets:
"OpenAiApi": {
- "Url": "https://wf.belyaev.live/webhook/e69fa36a-adff-410c-8cd0-04ec833d849e",
- "Secret": "my-secret"
+ "Url": "",
+ "Secret": ""
},This approach ensures sensitive configuration doesn't get committed to source control and provides flexibility for different environments.
There was a problem hiding this comment.
@maximgorbatyuk Understood. I'll respect your decision to keep the current configuration with the hardcoded values.
| public record GetOpenAiReportAnalysisResponse | ||
| { | ||
| public GetOpenAiReportAnalysisResponse( | ||
| string analysis, | ||
| OpenAiBodyReport report, | ||
| string bearer) | ||
| { | ||
| Analysis = analysis; | ||
| Report = report; | ||
| Bearer = bearer; | ||
| } | ||
|
|
||
| public string Analysis { get; } | ||
|
|
||
| public OpenAiBodyReport Report { get; } | ||
|
|
||
| public string Bearer { get; } | ||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider security implications of including the Bearer token in the response body
While the response structure is well-designed with immutable properties, exposing the JWT bearer token in the response body might present security risks if the client isn't properly secured. Bearer tokens are typically transmitted via HTTP headers.
Consider either:
- Moving the token to headers-only authentication
- Implementing token expiration with a short lifespan
- Adding documentation to ensure proper client-side token handling
🏁 Script executed:
#!/bin/bash
# Locate all references to GetOpenAiReportAnalysisResponse in the codebase
rg "GetOpenAiReportAnalysisResponse" -nLength of output: 1029
🏁 Script executed:
#!/bin/bash
# Display the handler to inspect how the Bearer token is used in the response
sed -n '1,200p' src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisHandler.csLength of output: 2338
🏁 Script executed:
#!/bin/bash
# Locate the IOpenAiService interface definition
rg "interface IOpenAiService" -n srcLength of output: 125
🏁 Script executed:
#!/bin/bash
# Display the IOpenAiService interface to inspect its methods
sed -n '1,200p' src/Infrastructure/Services/OpenAi/IOpenAiService.cs
# Find where GetBearer is implemented
rg "GetBearer" -n src/Infrastructure/Services/OpenAiLength of output: 605
Remove exposed OpenAI API key from response payload
The Bearer property on GetOpenAiReportAnalysisResponse contains your internal OpenAI API key. Returning it in the JSON response leaks a secret and poses a critical security risk.
Please remove the Bearer token from the response object and keep it strictly server-side. If clients need to authenticate future calls, implement one of the following instead:
- Proxy their requests through your API (so the client never sees the OpenAI key).
- Issue short-lived JWTs or OAuth tokens scoped for the client’s operations.
- If you must send a token, place it in a secure HTTP header (not in the JSON body), with proper CORS and header protections.
Locations to update:
- src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisResponse.cs – remove the
Bearerconstructor parameter, property, and related documentation. - src/Web.Api/Features/Subscribtions/GetOpenAiReportAnalysis/GetOpenAiReportAnalysisHandler.cs – stop passing
_openApiService.GetBearer()into the response. - Review any client-side code or documentation that expects
Bearerin the response.
| public record TechinterviewJwtTokenGenerator | ||
| { | ||
| private readonly string _secretKey; | ||
|
|
||
| private string _result; | ||
|
|
||
| public TechinterviewJwtTokenGenerator( | ||
| string secretKey) | ||
| { | ||
| _secretKey = secretKey; | ||
| } | ||
|
|
||
| public override string ToString() | ||
| { | ||
| return _result ??= GenerateInternal(); | ||
| } | ||
|
|
||
| private string GenerateInternal() | ||
| { | ||
| var securityKey = new SymmetricSecurityKey(Encoding.ASCII.GetBytes(_secretKey)); | ||
| var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256Signature); | ||
|
|
||
| var claims = new List<Claim> | ||
| { | ||
| new Claim(JwtRegisteredClaimNames.Sub, Guid.NewGuid().ToString()), | ||
| new Claim(JwtRegisteredClaimNames.Name, "TechInterview.space Bot"), | ||
| new Claim(JwtRegisteredClaimNames.Iat, DateTimeOffset.UtcNow.ToUnixTimeSeconds().ToString(), ClaimValueTypes.Integer64) | ||
| }; | ||
|
|
||
| var token = new JwtSecurityToken( | ||
| claims: claims, | ||
| expires: DateTime.UtcNow.AddHours(1), | ||
| signingCredentials: credentials); | ||
|
|
||
| return new JwtSecurityTokenHandler().WriteToken(token); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
JWT token generator uses unconventional design patterns
While the token generation logic works correctly, there are several design concerns:
- Using a record with mutable state (
_result) is unconventional - records are typically used for immutable data - The token is generated via
ToString()which is not intuitive for JWT token generation - The generated token has a fixed 1-hour expiration but is cached indefinitely, potentially causing authentication failures if used after expiration
- No validation is performed on the secret key to ensure it meets security requirements
Consider these improvements:
-public record TechinterviewJwtTokenGenerator
+public class TechinterviewJwtTokenGenerator
{
private readonly string _secretKey;
-
- private string _result;
+ private string _cachedToken;
+ private DateTime _tokenExpiration;
public TechinterviewJwtTokenGenerator(
string secretKey)
{
+ if (string.IsNullOrWhiteSpace(secretKey) || secretKey.Length < 32)
+ {
+ throw new ArgumentException("Secret key must be at least 32 characters long for security", nameof(secretKey));
+ }
_secretKey = secretKey;
}
- public override string ToString()
+ public string GenerateToken()
{
- return _result ??= GenerateInternal();
+ // Regenerate if token doesn't exist or is close to expiration (10 min buffer)
+ if (_cachedToken == null || DateTime.UtcNow.AddMinutes(10) > _tokenExpiration)
+ {
+ _cachedToken = GenerateInternal();
+ _tokenExpiration = DateTime.UtcNow.AddHours(1);
+ }
+ return _cachedToken;
}
private string GenerateInternal()
{
var securityKey = new SymmetricSecurityKey(Encoding.ASCII.GetBytes(_secretKey));
var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256Signature);
var claims = new List<Claim>
{
new Claim(JwtRegisteredClaimNames.Sub, Guid.NewGuid().ToString()),
new Claim(JwtRegisteredClaimNames.Name, "TechInterview.space Bot"),
new Claim(JwtRegisteredClaimNames.Iat, DateTimeOffset.UtcNow.ToUnixTimeSeconds().ToString(), ClaimValueTypes.Integer64)
};
var token = new JwtSecurityToken(
claims: claims,
expires: DateTime.UtcNow.AddHours(1),
signingCredentials: credentials);
return new JwtSecurityTokenHandler().WriteToken(token);
}
+
+ // Keep ToString override for backward compatibility
+ public override string ToString()
+ {
+ return GenerateToken();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record TechinterviewJwtTokenGenerator | |
| { | |
| private readonly string _secretKey; | |
| private string _result; | |
| public TechinterviewJwtTokenGenerator( | |
| string secretKey) | |
| { | |
| _secretKey = secretKey; | |
| } | |
| public override string ToString() | |
| { | |
| return _result ??= GenerateInternal(); | |
| } | |
| private string GenerateInternal() | |
| { | |
| var securityKey = new SymmetricSecurityKey(Encoding.ASCII.GetBytes(_secretKey)); | |
| var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256Signature); | |
| var claims = new List<Claim> | |
| { | |
| new Claim(JwtRegisteredClaimNames.Sub, Guid.NewGuid().ToString()), | |
| new Claim(JwtRegisteredClaimNames.Name, "TechInterview.space Bot"), | |
| new Claim(JwtRegisteredClaimNames.Iat, DateTimeOffset.UtcNow.ToUnixTimeSeconds().ToString(), ClaimValueTypes.Integer64) | |
| }; | |
| var token = new JwtSecurityToken( | |
| claims: claims, | |
| expires: DateTime.UtcNow.AddHours(1), | |
| signingCredentials: credentials); | |
| return new JwtSecurityTokenHandler().WriteToken(token); | |
| } | |
| } | |
| public class TechinterviewJwtTokenGenerator | |
| { | |
| private readonly string _secretKey; | |
| private string _cachedToken; | |
| private DateTime _tokenExpiration; | |
| public TechinterviewJwtTokenGenerator( | |
| string secretKey) | |
| { | |
| if (string.IsNullOrWhiteSpace(secretKey) || secretKey.Length < 32) | |
| { | |
| throw new ArgumentException("Secret key must be at least 32 characters long for security", nameof(secretKey)); | |
| } | |
| _secretKey = secretKey; | |
| } | |
| public string GenerateToken() | |
| { | |
| // Regenerate if token doesn't exist or is close to expiration (10 min buffer) | |
| if (_cachedToken == null || DateTime.UtcNow.AddMinutes(10) > _tokenExpiration) | |
| { | |
| _cachedToken = GenerateInternal(); | |
| _tokenExpiration = DateTime.UtcNow.AddHours(1); | |
| } | |
| return _cachedToken; | |
| } | |
| private string GenerateInternal() | |
| { | |
| var securityKey = new SymmetricSecurityKey(Encoding.ASCII.GetBytes(_secretKey)); | |
| var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256Signature); | |
| var claims = new List<Claim> | |
| { | |
| new Claim(JwtRegisteredClaimNames.Sub, Guid.NewGuid().ToString()), | |
| new Claim(JwtRegisteredClaimNames.Name, "TechInterview.space Bot"), | |
| new Claim(JwtRegisteredClaimNames.Iat, DateTimeOffset.UtcNow.ToUnixTimeSeconds().ToString(), ClaimValueTypes.Integer64) | |
| }; | |
| var token = new JwtSecurityToken( | |
| claims: claims, | |
| expires: DateTime.UtcNow.AddHours(1), | |
| signingCredentials: credentials); | |
| return new JwtSecurityTokenHandler().WriteToken(token); | |
| } | |
| // Keep ToString override for backward compatibility | |
| public override string ToString() | |
| { | |
| return GenerateToken(); | |
| } | |
| } |
| var result = new OpenAiBodyReport( | ||
| new SalarySubscriptionData( | ||
| allProfessions, | ||
| subscription, | ||
| _context, | ||
| DateTimeOffset.UtcNow), | ||
| Currency.KZT); |
There was a problem hiding this comment.
Initialize SalarySubscriptionData before passing it to OpenAiBodyReport.
OpenAiBodyReport calls IsInitializedOrFail() on the SalarySubscriptionData instance; the latter returns false unless InitializeAsync() has been executed.
As it stands, this handler will reliably throw InvalidOperationException at runtime.
- var result = new OpenAiBodyReport(
- new SalarySubscriptionData(
- allProfessions,
- subscription,
- _context,
- DateTimeOffset.UtcNow),
- Currency.KZT);
+ var subscriptionData = await new SalarySubscriptionData(
+ allProfessions,
+ subscription,
+ _context,
+ DateTimeOffset.UtcNow)
+ .InitializeAsync(cancellationToken);
+
+ var result = new OpenAiBodyReport(
+ subscriptionData,
+ Currency.KZT);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = new OpenAiBodyReport( | |
| new SalarySubscriptionData( | |
| allProfessions, | |
| subscription, | |
| _context, | |
| DateTimeOffset.UtcNow), | |
| Currency.KZT); | |
| var subscriptionData = await new SalarySubscriptionData( | |
| allProfessions, | |
| subscription, | |
| _context, | |
| DateTimeOffset.UtcNow) | |
| .InitializeAsync(cancellationToken); | |
| var result = new OpenAiBodyReport( | |
| subscriptionData, | |
| Currency.KZT); |
| return new GetOpenAiReportAnalysisResponse( | ||
| await _openApiService.GetAnalysisAsync(cancellationToken), | ||
| report, | ||
| _openApiService.GetBearer()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for OpenAI service calls.
The calls to _openApiService.GetAnalysisAsync() and _openApiService.GetBearer() lack error handling. If these operations fail, the exception would propagate to the caller without proper context.
return new GetOpenAiReportAnalysisResponse(
- await _openApiService.GetAnalysisAsync(cancellationToken),
+ await _openApiService.GetAnalysisAsync(cancellationToken)
+ .ConfigureAwait(false),
report,
- _openApiService.GetBearer());
+ _openApiService.GetBearer());Consider implementing a try-catch block to handle potential exceptions:
try
{
var analysis = await _openApiService.GetAnalysisAsync(cancellationToken)
.ConfigureAwait(false);
var bearer = _openApiService.GetBearer();
return new GetOpenAiReportAnalysisResponse(
analysis,
report,
bearer);
}
catch (Exception ex)
{
// Log the exception
throw new ApplicationException("Failed to retrieve OpenAI analysis", ex);
}| public SalarySubscriptionData( | ||
| List<Profession> allProfessions, | ||
| StatDataChangeSubscription subscription, | ||
| DatabaseContext context, | ||
| DateTimeOffset now) | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null validation for constructor parameters.
The constructor doesn't check for null parameters, which could lead to NullReferenceExceptions later in the code.
public SalarySubscriptionData(
List<Profession> allProfessions,
StatDataChangeSubscription subscription,
DatabaseContext context,
DateTimeOffset now)
{
+ _subscription = subscription ?? throw new ArgumentNullException(nameof(subscription));
+ _context = context ?? throw new ArgumentNullException(nameof(context));
+ allProfessions ??= new List<Profession>();
- _subscription = subscription;
- _context = context;
_isInitianlized = false;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
30-30: Use a safer sed delimiter to handle potential “/” characters in secrets.Right now you’re using
/as the sed delimiter, which will break ifsecrets.OPEN_AI_SECRETcontains a/. Consider switching to a different delimiter (e.g.|) to avoid this pitfall:-sed -i 's/"__OPEN_AI_SECRET",/"${{ secrets.OPEN_AI_SECRET }}",/g' ${{env.PRODUCTION_SETTINGS_FILE}} +sed -i 's|"__OPEN_AI_SECRET",|"${{ secrets.OPEN_AI_SECRET }}",|g' ${{env.PRODUCTION_SETTINGS_FILE}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/deploy.yml(1 hunks)src/Web.Api/appsettings.Production.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Web.Api/appsettings.Production.json
| sed -i 's/"__S3_BUCKET_NAME",/"${{ vars.S3_BUCKET_NAME }}",/g' ${{env.PRODUCTION_SETTINGS_FILE}} | ||
| sed -i 's/"__TELEGRAM_BOT_KEY",/"${{ secrets.TELEGRAM_BOT_KEY }}",/g' ${{env.PRODUCTION_SETTINGS_FILE}} | ||
| sed -i 's/"__SENDGRID_API_KEY",/"${{ secrets.SENDGRID_API_KEY }}",/g' ${{env.PRODUCTION_SETTINGS_FILE}} | ||
| sed -i 's/"__OPEN_AI_SECRET",/"${{ secrets.OPEN_AI_SECRET }}",/g' ${{env.PRODUCTION_SETTINGS_FILE}} |
There was a problem hiding this comment.
Missing JWT secret injection for production settings.
You’ve added JWT generation in this PR, but there’s no replacement step for a __JWT_SECRET placeholder in appsettings.Production.json. Without it, the app will continue to see the literal placeholder and fail at runtime. Add something like:
sed -i 's/"__JWT_SECRET",/"${{ secrets.JWT_SECRET }}",/g' ${{env.PRODUCTION_SETTINGS_FILE}}right after the other sed commands.
There was a problem hiding this comment.
JWT is generated with secret that in settings now. That's ok
There was a problem hiding this comment.
@maximgorbatyuk I understand now. Since you're using the OpenAI API secret as the JWT secret, and that replacement is already handled in line 28, no additional secret injection is needed. Thanks for the clarification!
Summary by CodeRabbit
New Features
Improvements
SelectedProfessionIdsacross queries, controllers, and user commands.Tests
Chores