Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add gherkin unit tests #173 #176

Merged
merged 2 commits into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

8 changes: 0 additions & 8 deletions src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/BmbRoles.cs

This file was deleted.

36 changes: 0 additions & 36 deletions src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/JwtExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,48 +1,12 @@
using System.Security.Claims;
using System.Text;
using FIAP.TechChallenge.ByteMeBurger.Controllers.Dto;
using Microsoft.IdentityModel.Tokens;

namespace FIAP.TechChallenge.ByteMeBurger.Api.Auth;

/// <summary>
/// Jwt token extensions methods
/// </summary>
public static class JwtExtensions
{
/// <summary>
/// Configure Jtw token validation
/// </summary>
/// <param name="services">Service collection</param>
/// <param name="configuration">Configuration</param>
public static void ConfigureJwt(this IServiceCollection services, IConfiguration configuration)
{
var jwtOptions = configuration
.GetSection("JwtOptions")
.Get<JwtOptions>();

services.AddAuthentication()
.AddJwtBearer(options =>
{
if (jwtOptions.UseAccessToken)
{
options.Events = AccessTokenAuthEventsHandler.Instance;
}

options.TokenValidationParameters = new TokenValidationParameters
{
ValidIssuer = jwtOptions.Issuer,
ValidAudience = jwtOptions.Audience,
ValidateAudience = true,
ValidateIssuer = true,
ValidateLifetime = true,
LogValidationExceptions = true,
IssuerSigningKey =
new SymmetricSecurityKey(Encoding.UTF8.GetBytes(jwtOptions.SigningKey))
};
});
}

// https://stackoverflow.com/a/55740879/2921329
/// <summary>
/// Get customer details from Jwt Claims
Expand Down
20 changes: 0 additions & 20 deletions src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/JwtOptions.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
using System.ComponentModel.DataAnnotations;
using FIAP.TechChallenge.ByteMeBurger.Api.Auth;
using Bmb.Auth;
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent namespace change detected

The namespace change from FIAP.TechChallenge.ByteMeBurger.Api.Auth to Bmb.Auth appears incomplete and inconsistent:

  • The old namespace FIAP.TechChallenge.ByteMeBurger.Api.Auth is still in use in multiple files
  • The new namespace Bmb.Auth is not defined anywhere in the codebase
  • The BmbRoles class referenced in the controllers is not found in either namespace

This change needs to be reverted or properly implemented across the entire codebase to maintain consistency.

🔗 Analysis chain

Verify the namespace change impact across the codebase

The authentication namespace has been changed from FIAP.TechChallenge.ByteMeBurger.Api.Auth to Bmb.Auth. This change needs verification to ensure consistency.


Let me gather more information about the BmbRoles class and its usage.


Let me check the content of the BmbRoles class and related files.


Let me check the actual content of the files in the Auth folders.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the namespace change impact and BmbRoles class existence

# Check for any remaining references to the old namespace
echo "Checking for old namespace references..."
rg "FIAP\.TechChallenge\.ByteMeBurger\.Api\.Auth"

# Verify BmbRoles class exists in the new namespace
echo "Verifying BmbRoles class in new namespace..."
rg -p "namespace Bmb.Auth" -A 10 | rg "class BmbRoles"

# Check for any mixed usage of auth namespaces
echo "Checking for mixed namespace usage..."
rg "Auth.*Roles"

Length of output: 1511


Script:

#!/bin/bash
# Search for BmbRoles class definition
echo "Searching for BmbRoles class definition..."
ast-grep --pattern 'class BmbRoles'

# Check for files in Bmb.Auth namespace
echo "Checking files in Bmb.Auth namespace..."
rg "namespace Bmb.Auth"

# Check for any Auth folder in the codebase
echo "Looking for Auth folders..."
fd "Auth$" --type d

Length of output: 14754


Script:

#!/bin/bash
# Check the content of files in Auth folders
echo "Checking content of Auth files..."
rg -p "class.*Roles" src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/
rg -p "class.*Roles" tests/FIAP.TechChallenge.ByteMeBurger.Api.Test/Auth/

# Check for any roles-related constants or enums
echo "Checking for roles-related code..."
ast-grep --pattern 'const.*Roles'

Length of output: 336


Script:

#!/bin/bash
# Check content of all files in Auth folders
echo "Content of files in src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/..."
cat src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/*

echo -e "\nContent of files in tests/FIAP.TechChallenge.ByteMeBurger.Api.Test/Auth/..."
cat tests/FIAP.TechChallenge.ByteMeBurger.Api.Test/Auth/*

Length of output: 4129

using FIAP.TechChallenge.ByteMeBurger.Api.Model.Customers;
using FIAP.TechChallenge.ByteMeBurger.Controllers.Contracts;
using FIAP.TechChallenge.ByteMeBurger.Controllers.Dto;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
using System.Collections.ObjectModel;
using FIAP.TechChallenge.ByteMeBurger.Api.Auth;
using Bmb.Auth;
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove redundant namespace import

The Bmb.Auth namespace appears to be unnecessary. The codebase shows that auth-related code is under the FIAP.TechChallenge.ByteMeBurger.Api.Auth namespace, which is already imported. The BmbRoles type is used in authorization attributes across multiple controllers, and while we can't see its direct definition, the namespace organization suggests it belongs to the FIAP namespace structure.

  • Remove the using Bmb.Auth; directive to avoid potential namespace conflicts
  • Keep the using FIAP.TechChallenge.ByteMeBurger.Api.Auth; directive
🔗 Analysis chain

Verify namespace usage to avoid conflicts

The code includes both Bmb.Auth and FIAP.TechChallenge.ByteMeBurger.Api.Auth namespaces. This could lead to ambiguous references, especially for types like BmbRoles used in authorization attributes.

Let's verify the namespace organization:

Also applies to: 7-7

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for BmbRoles usage across the codebase
rg -l "BmbRoles" 

# Check the actual namespace of BmbRoles
ast-grep --pattern 'namespace $_ { $$$ class BmbRoles { $$$ } $$$ }'

Length of output: 308


Script:

#!/bin/bash
# Let's check the actual content of files using BmbRoles to understand its namespace
rg "BmbRoles" -B 2 -A 2

# Let's also check if there are any Auth-related files in both namespaces
fd -e cs -e fs -e vb "Auth"

Length of output: 3691


Script:

#!/bin/bash
# Let's check the content of any Auth-related files to understand the namespace structure
rg -g "*.cs" "namespace.*Auth" -B 1 -A 1

# Also check for any BmbRoles class definition
rg -g "*.cs" "class.*BmbRoles" -B 2 -A 2

Length of output: 660

using FIAP.TechChallenge.ByteMeBurger.Api.Model.Orders;
using FIAP.TechChallenge.ByteMeBurger.Controllers.Contracts;
using FIAP.TechChallenge.ByteMeBurger.Controllers.Dto;
using Bmb.Domain.Core.ValueObjects;
using FIAP.TechChallenge.ByteMeBurger.Api.Auth;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Caching.Hybrid;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using FIAP.TechChallenge.ByteMeBurger.Api.Auth;
using Bmb.Auth;
using FIAP.TechChallenge.ByteMeBurger.Api.Model.Products;
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent namespace usage detected

The namespace change from FIAP.TechChallenge.ByteMeBurger.Api.Auth to Bmb.Auth is incomplete. The following files still use the old namespace:

  • src/FIAP.TechChallenge.ByteMeBurger.Api/Controllers/OrdersController.cs
  • src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/JwtExtensions.cs
  • tests/FIAP.TechChallenge.ByteMeBurger.Api.Test/Auth/JwtExtensionsTest.cs

This creates an inconsistent state where some files use the new Bmb.Auth namespace while others still reference the old one, despite using the new BmbRoles enum.

🔗 Analysis chain

Verify namespace change consistency across the codebase

The auth namespace has been changed to Bmb.Auth. Let's ensure this change is consistent across all controllers and auth-related files.


Let me check the specific content of these files to understand the namespace inconsistency better.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old auth namespace
# and verify consistent usage of the new namespace

echo "Checking for old namespace references..."
rg -l "FIAP\.TechChallenge\.ByteMeBurger\.Api\.Auth"

echo "Verifying consistent usage of new namespace..."
rg -l "Bmb\.Auth"

Length of output: 830


Script:

#!/bin/bash
# Check the content of files that still use the old namespace
echo "=== Content of OrdersController.cs ==="
rg -A 2 -B 2 "FIAP\.TechChallenge\.ByteMeBurger\.Api\.Auth" "src/FIAP.TechChallenge.ByteMeBurger.Api/Controllers/OrdersController.cs"

echo "=== Content of JwtExtensions.cs ==="
rg -A 2 -B 2 "FIAP\.TechChallenge\.ByteMeBurger\.Api\.Auth" "src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/JwtExtensions.cs"

# Check if there are any references to BmbRoles in these files
echo "=== BmbRoles usage ==="
rg "BmbRoles" "src/FIAP.TechChallenge.ByteMeBurger.Api/Controllers/OrdersController.cs" "src/FIAP.TechChallenge.ByteMeBurger.Api/Auth/JwtExtensions.cs"

Length of output: 1382

using FIAP.TechChallenge.ByteMeBurger.Controllers.Contracts;
using FIAP.TechChallenge.ByteMeBurger.Controllers.Dto;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
<ItemGroup>
<PackageReference Include="AspNetCore.HealthChecks.MySql" Version="8.0.1" />
<PackageReference Include="AspNetCore.HealthChecks.UI.Client" Version="8.0.1" />
<PackageReference Include="Bmb.Auth" Version="0.0.1" />
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Early version package with incomplete integration detected

The verification reveals several concerns:

  • Bmb.Auth package is imported but its configuration is not visible in the codebase
  • The application still heavily relies on JWT configuration (JwtOptions and ConfigureJwt)
  • Mixed authentication mechanisms could lead to security vulnerabilities

Program.cs: Contains both JWT configuration and Bmb.Auth imports
OrdersController.cs: Uses authorization but the mechanism is unclear
• Authentication middleware is present but the configuration is ambiguous

[security_warning]
Recommendations:

  1. Complete the migration to Bmb.Auth or rollback to JWT
  2. Remove unused JWT configurations if Bmb.Auth is the intended solution
  3. Wait for a stable version of Bmb.Auth before production use
🔗 Analysis chain

Consider the stability of Bmb.Auth package

The package version 0.0.1 indicates a very early development stage. This could pose stability and security risks in a production environment.

Consider:

  • Waiting for a more stable version
  • Thoroughly testing the authentication mechanism
  • Having a rollback plan in case of issues

This will help us understand:

  1. Where and how Bmb.Auth is being used
  2. Authentication configuration in the application
  3. Usage of authentication/authorization in controllers

This will help us understand the authentication configuration details and how Bmb.Auth is integrated with the existing JWT setup.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the usage and implementation of Bmb.Auth
# Look for authentication-related configurations and implementations
rg -l "Bmb.Auth" | while read -r file; do
  echo "=== $file ==="
  rg "Bmb.Auth" "$file" -A 5
done

Length of output: 1721


Script:

#!/bin/bash
# Let's check for authentication-related configurations and usages
rg -l "using.*Bmb.Auth|AddBmbAuth|UseBmbAuth" --type cs

# Check Program.cs and Startup.cs for auth configuration
fd -e cs "(Program|Startup)" --exec cat {}

# Check for authentication attributes and middleware usage
rg "(\[Authorize\]|UseAuthentication|UseAuthorization)" --type cs -A 3

Length of output: 6891


Script:

#!/bin/bash
# Let's check the ConfigureJwt extension method and JwtOptions
rg -p "ConfigureJwt|JwtOptions" --type cs -A 5

# Check for any Bmb.Auth specific configurations
rg "Bmb\.Auth\." --type cs -A 5

# Look for authentication-related services registration
rg "AddAuthentication|services\.Configure<.*Auth" --type cs -A 5

Length of output: 869

<PackageReference Include="Bmb.Domain.Core" Version="0.0.12" />
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.2" />
<PackageReference Include="Microsoft.AspNetCore.OpenApi" Version="8.0.3"/>
<PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" Version="8.0.2" />
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove unused Microsoft.IdentityModel.JsonWebTokens package

The search results confirm that while the application uses authentication and authorization through [Authorize] attributes, there are no direct usages of JWT-related code or the Microsoft.IdentityModel.JsonWebTokens package. Since authentication has been migrated to Bmb.Auth as mentioned, this package can be safely removed.

🔗 Analysis chain

Verify if Microsoft.IdentityModel.JsonWebTokens is still needed

Since JWT authentication has been removed in favor of Bmb.Auth, this package might no longer be necessary.


Let's check for any remaining JWT or authentication-related code to make a more informed decision about whether this package is still needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining JWT token usage
rg -l "JsonWebToken|JWT" | while read -r file; do
  echo "=== $file ==="
  rg "JsonWebToken|JWT" "$file" -A 5
done

Length of output: 1721


Script:

#!/bin/bash
# Search for any JWT or token-related code in C# files
rg -t cs "using.*IdentityModel|JsonWebToken|JWT|JwtBearer" -A 5

# Search for authentication-related configuration
rg -t cs "AddAuthentication|UseAuthentication|[Aa]uthorize|Bearer" -A 5

Length of output: 7563

<PackageReference Include="Microsoft.VisualStudio.Web.CodeGeneration.Design" Version="8.0.2" />
Expand Down
2 changes: 1 addition & 1 deletion src/FIAP.TechChallenge.ByteMeBurger.Api/Program.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Text.Json.Serialization;
using FIAP.TechChallenge.ByteMeBurger.Api.Auth;
using Bmb.Auth;
using FIAP.TechChallenge.ByteMeBurger.DI;
using HealthChecks.UI.Client;
using Microsoft.AspNetCore.Diagnostics.HealthChecks;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,10 @@
<ProjectReference Include="..\FIAP.TechChallenge.ByteMeBurger.Test.Common\FIAP.TechChallenge.ByteMeBurger.Test.Common.csproj" />
</ItemGroup>

<ItemGroup>
<None Update="UseCases\Orders\Gherkin\CreateOrder.feature">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Feature: Create new order

Scenario: Create a new order successfully
Given Selected product exists
And Tracking code is created
When UseCase is called
Then it should create the order
And it should publish integration event
Comment on lines +3 to +8
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance scenario coverage and step specificity

The current scenario lacks specific details and covers only the happy path. Consider the following improvements:

  1. Add concrete examples with actual data
  2. Include validation of order details
  3. Add scenarios for edge cases and error conditions
-  Scenario: Create a new order successfully
-    Given Selected product exists
-    And Tracking code is created
-    When UseCase is called
-    Then it should create the order
-    And it should publish integration event
+  Scenario: Successfully create a single item order
+    Given the following product exists in the menu:
+      | Id  | Name           | Price  | Category |
+      | 123 | Classic Burger | 15.99  | Burgers  |
+    And a tracking code "ORDER-2024-001" is generated
+    When I create an order with:
+      | ProductId | Quantity |
+      | 123      | 1        |
+    Then the order should be created with status "Pending"
+    And an "OrderCreated" event should be published with tracking code "ORDER-2024-001"
+
+  Scenario: Attempt to create order with non-existent product
+    Given no product exists with id "999"
+    When I create an order with:
+      | ProductId | Quantity |
+      | 999      | 1        |
+    Then it should fail with error "Product not found"
+
+  Scenario: Attempt to create order with invalid quantity
+    Given the following product exists in the menu:
+      | Id  | Name           | Price  | Category |
+      | 123 | Classic Burger | 15.99  | Burgers  |
+    When I create an order with:
+      | ProductId | Quantity |
+      | 123      | 0        |
+    Then it should fail with error "Invalid quantity"
📝 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.

Suggested change
Scenario: Create a new order successfully
Given Selected product exists
And Tracking code is created
When UseCase is called
Then it should create the order
And it should publish integration event
Scenario: Successfully create a single item order
Given the following product exists in the menu:
| Id | Name | Price | Category |
| 123 | Classic Burger | 15.99 | Burgers |
And a tracking code "ORDER-2024-001" is generated
When I create an order with:
| ProductId | Quantity |
| 123 | 1 |
Then the order should be created with status "Pending"
And an "OrderCreated" event should be published with tracking code "ORDER-2024-001"
Scenario: Attempt to create order with non-existent product
Given no product exists with id "999"
When I create an order with:
| ProductId | Quantity |
| 999 | 1 |
Then it should fail with error "Product not found"
Scenario: Attempt to create order with invalid quantity
Given the following product exists in the menu:
| Id | Name | Price | Category |
| 123 | Classic Burger | 15.99 | Burgers |
When I create an order with:
| ProductId | Quantity |
| 123 | 0 |
Then it should fail with error "Invalid quantity"

Comment on lines +1 to +8
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Test coverage needs improvement for error scenarios

Based on the implementation and current test coverage, there are missing test scenarios:

  • The implementation throws EntityNotFoundException when a product is not found, but there's no test scenario for this case
  • The test only covers the happy path where the order is created successfully

Required test scenarios to add:

  • Product not found scenario
  • Invalid product ID scenario
  • Empty product list scenario
🔗 Analysis chain

Verify test coverage against implementation

Let's ensure the test scenarios align with all possible outcomes in the use case implementation.


Let me gather more information about the implementation to verify all test scenarios.


Let me try one more search to find the complete implementation details.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find the order creation use case implementation to verify all paths are covered

# Look for the order creation use case implementation
ast-grep --pattern 'class $_ : ICreateOrderUseCase {
  $$$
}'

# Look for possible validation rules or error conditions
rg -A 5 "throw new \w+Exception"

Length of output: 9444


Script:

#!/bin/bash
# Find the complete implementation of CreateOrderUseCase
ast-grep --pattern 'class CreateOrderUseCase {
  $$$
}'

# Find all test scenarios for order creation
rg -A 10 "Scenario:" "tests/FIAP.TechChallenge.ByteMeBurger.Application.Test/UseCases/Orders/Gherkin/"

Length of output: 970


Script:

#!/bin/bash
# Find the complete implementation with a more specific pattern
ast-grep --pattern 'class CreateOrderUseCase : ICreateOrderUseCase {
  $$$
}'

# Also search for the implementation file directly
rg -A 20 "class CreateOrderUseCase" "src/FIAP.TechChallenge.ByteMeBurger.Application/UseCases/Orders/"

Length of output: 2874

Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
using Bmb.Domain.Core.Events;
using Bmb.Domain.Core.Interfaces;
using FIAP.TechChallenge.ByteMeBurger.Application.UseCases.Orders;
using Xunit.Gherkin.Quick;
using OrderCreated = Bmb.Domain.Core.Events.Integration.OrderCreated;

namespace FIAP.TechChallenge.ByteMeBurger.Application.Test.UseCases.Orders.Gherkin;

[FeatureFile("./UseCases/Orders/Gherkin/CreateOrder.feature")]
public class CreateOrderSteps : Feature
{
// Given Selected product exists
// And Tracking code is created
// When UseCase is called
// Then it should create the order
private readonly CreateOrderUseCase _useCase;
private readonly Mock<IProductRepository> _mockProductRepository;
private readonly Mock<IOrderTrackingCodeService> _mockOrderTrackingCodeService;
private readonly Mock<IDispatcher> _mockDispatcher;
private Order _order;
private Product _product;

public CreateOrderSteps()
{
_mockDispatcher = new Mock<IDispatcher>();
_mockProductRepository = new Mock<IProductRepository>();
_mockOrderTrackingCodeService = new Mock<IOrderTrackingCodeService>();

_useCase = new CreateOrderUseCase(_mockProductRepository.Object, _mockOrderTrackingCodeService.Object,
_mockDispatcher.Object);
}

[Given("Selected product exists")]
public void SetupExistingProduct()
{
_product = new Product(Guid.NewGuid(), "product", "description", ProductCategory.Drink, 10, []);
_mockProductRepository.Setup(p => p.FindByIdAsync(_product.Id))
.ReturnsAsync(_product)
.Verifiable();
}

[And("Tracking code is created")]
public void SetupTrackingCode()
{
_mockOrderTrackingCodeService.Setup(s => s.GetNextAsync())
.ReturnsAsync(new OrderTrackingCode("code"))
.Verifiable();
}

[When("UseCase is called")]
public async Task WhenUseCaseIsCalled()
{
_order = await _useCase.Execute(null, new List<SelectedProduct>
{
new(_product.Id, 1)
});
}
Comment on lines +50 to +57
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve test clarity and parameterization.

The use case execution could be improved by:

  1. Documenting why the customer is null or using a valid test customer
  2. Making the product quantity configurable
+private const int DefaultProductQuantity = 1;
+
 [When("UseCase is called")]
 public async Task WhenUseCaseIsCalled()
 {
+    // TODO: Consider adding a test for authenticated customers
     _order = await _useCase.Execute(null, new List<SelectedProduct>
     {
-        new(_product.Id, 1)
+        new(_product.Id, DefaultProductQuantity)
     });
 }
📝 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.

Suggested change
[When("UseCase is called")]
public async Task WhenUseCaseIsCalled()
{
_order = await _useCase.Execute(null, new List<SelectedProduct>
{
new(_product.Id, 1)
});
}
private const int DefaultProductQuantity = 1;
[When("UseCase is called")]
public async Task WhenUseCaseIsCalled()
{
// TODO: Consider adding a test for authenticated customers
_order = await _useCase.Execute(null, new List<SelectedProduct>
{
new(_product.Id, DefaultProductQuantity)
});
}


[Then("it should create the order")]
public void ThenItShouldCreateTheOrder()
{
_order.Should().NotBeNull();
_order.TrackingCode.Should().Be(new OrderTrackingCode("code"));
_order.OrderItems[0].ProductId.Should().Be(_product.Id);
_mockProductRepository.VerifyAll();
}

[And("it should publish integration event")]
public void ThenItShouldPublishTheEvent()
{
_mockDispatcher.Verify(d => d.PublishIntegrationAsync(It.IsAny<OrderCreated>(), default), Times.Once);
}
Comment on lines +59 to +72
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance assertion specificity and consistency.

The assertions could be improved by:

  1. Using the same tracking code constant defined earlier
  2. Adding more specific event verification
 [Then("it should create the order")]
 public void ThenItShouldCreateTheOrder()
 {
     _order.Should().NotBeNull();
-    _order.TrackingCode.Should().Be(new OrderTrackingCode("code"));
+    _order.TrackingCode.Should().Be(new OrderTrackingCode(TestTrackingCode));
     _order.OrderItems[0].ProductId.Should().Be(_product.Id);
+    _order.OrderItems[0].Quantity.Should().Be(DefaultProductQuantity);
     _mockProductRepository.VerifyAll();
 }

 [And("it should publish integration event")]
 public void ThenItShouldPublishTheEvent()
 {
-    _mockDispatcher.Verify(d => d.PublishIntegrationAsync(It.IsAny<OrderCreated>(), default), Times.Once);
+    _mockDispatcher.Verify(d => d.PublishIntegrationAsync(
+        It.Is<OrderCreated>(e => 
+            e.OrderId == _order.Id && 
+            e.TrackingCode == TestTrackingCode), 
+        default), 
+        Times.Once);
 }

Committable suggestion skipped: line range outside the PR's diff.

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<PackageReference Include="coverlet.collector" Version="6.0.0"/>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0"/>
<PackageReference Include="xunit" Version="2.5.3"/>
<PackageReference Include="Xunit.Gherkin.Quick" Version="4.5.0" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.5.3"/>
</ItemGroup>

Expand Down
17 changes: 9 additions & 8 deletions tf/variables.tf
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
variable "rds_cluster_identifier" {
type = string
default = "techchallenge-mysql-default"
default = "gh-techchallenge-mysql"
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Resource naming convention inconsistency detected

The current naming patterns show inconsistency across resources:

  • New RDS identifier uses gh-techchallenge-mysql pattern
  • New EKS cluster uses eks_dev_quixada pattern
  • Other resources use different patterns like fiap-orders, configmap-api, etc.

Consider standardizing the naming convention across all resources to follow a consistent pattern that includes:

  • Environment indicator (dev/prod)
  • Service/project identifier
  • Resource type
🔗 Analysis chain

Verify infrastructure naming convention alignment

The changes to resource identifiers (gh-techchallenge-mysql and eks_dev_quixada) suggest environment-specific resources. Ensure these align with your organization's naming conventions and environment separation strategy.

Also applies to: 20-20

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for consistent naming patterns across infrastructure files
rg -g '*.tf' -A 2 'gh-techchallenge|eks_dev'

Length of output: 218


Script:

#!/bin/bash
# Check for any existing naming patterns in other resources
rg -g '*.tf' '(resource|name|identifier)\s*=\s*".+?"' -A 1

# Also check for any environment-specific variables or tags
rg -g '*.tf' '(environment|env|stage)\s*=\s*".+?"' -A 1

Length of output: 1289

}

variable "profile" {
Expand All @@ -17,7 +17,7 @@ variable "region" {

variable "eks_cluster_name" {
type = string
default = "quixada"
default = "eks_dev_quixada"
}

variable "apgw_name" {
Expand All @@ -28,6 +28,7 @@ variable "apgw_name" {
variable "jwt_signing_key" {
type = string
sensitive = true
default = ""
}

variable "jwt_issuer" {
Expand All @@ -44,7 +45,7 @@ variable "jwt_aud" {

variable "api_docker_image" {
type = string
default = "ghcr.io/soat-fiap/fiap.techchallenge.bytemeburger/api:sha-b83177c"
default = "ghcr.io/soat-fiap/fiap.techchallenge.bytemeburger/api:latest"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using 'latest' tag for Docker images

Using the latest tag (ghcr.io/soat-fiap/fiap.techchallenge.bytemeburger/api:latest) is not recommended as it:

  1. Makes deployments non-deterministic
  2. Complicates rollback procedures
  3. May cause inconsistencies across environments

Consider using specific version tags or SHA digests for better reproducibility and security.

 variable "api_docker_image" {
   type    = string
-  default = "ghcr.io/soat-fiap/fiap.techchallenge.bytemeburger/api:latest"
+  default = "ghcr.io/soat-fiap/fiap.techchallenge.bytemeburger/api:sha-<specific-commit>"
 }

Committable suggestion skipped: line range outside the PR's diff.

}

variable "internal_elb_name" {
Expand All @@ -55,28 +56,28 @@ variable "internal_elb_name" {
variable "db_user" {
type = string
sensitive = true
default = "db_user"
default = "test"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

SECURITY RISK: Remove hardcoded credentials and use secure secret management

Several critical security issues found:

  1. Database credentials are hardcoded to "test"
  2. Empty API access keys could cause runtime issues
  3. Sensitive credentials should not have default values in version control

These practices pose significant security risks if accidentally used in production environments.

Consider these security improvements:

  1. Remove default values for all sensitive credentials
  2. Use AWS Secrets Manager or similar service for credential management
  3. Document the required variables in README.md
  4. Implement validation to prevent empty API keys in production
 variable "db_user" {
   type      = string
   sensitive = true
-  default   = "test"
 }
 variable "db_pwd" {
   type      = string
   sensitive = true
-  default   = "test"
 }

 variable "api_access_key_id" {
   type      = string
   nullable  = false
   sensitive = true
-  default = ""
 }

 variable "api_secret_access_key" {
   type      = string
   nullable  = false
   sensitive = true
-  default = ""
 }

Also applies to: 64-64, 71-71, 78-78

}

variable "db_pwd" {
type = string
sensitive = true
default = "db_password"
default = "test"
}

variable "api_access_key_id" {
type = string
nullable = false
sensitive = true
default = ""
}

variable "api_secret_access_key" {
type = string
nullable = false
sensitive = true
default = ""
}

variable "user_pool_name" {
type = string
default = "bmb-users-pool-local"
default = "bmb-users-pool-dev"
}
Loading