Skip to content

Conversation

@SasakiTakatsugu
Copy link
Contributor

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements plan management functionality by replacing centralized error handling with local error handling and adding new API endpoints for plan operations.

  • Removed the centralized HandleApiException method from helper classes across both .NET 8 and .NET Framework 4.8 projects
  • Replaced all API exception handling with local error handling using standardized error messages and logging
  • Added new API endpoints for pricing plans, tax rates, and tenant plan management

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
SampleWebAppDotNet8/Program.cs Replaced centralized exception handling with local error handling and console logging
SampleWebAppDotNet8/Helpers/SaasusApiHelpers.cs Removed HandleApiException methods
SampleWebAppDotNet8/Controllers/BillingController.cs Added new pricing plan endpoints and replaced exception handling
SampleWebAppDotNet48/Helpers/SaasusApiHelpers.cs Updated error response format from "error" to "detail" property
SampleWebAppDotNet48/Controllers/MainController.cs Replaced centralized exception handling with local error handling
SampleWebAppDotNet48/Controllers/BillingController.cs Added new pricing plan endpoints and replaced exception handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

var handled = SaasusApiHelpers.HandleApiException(ex);
return Content(handled.Item1, handled.Item2);
System.Diagnostics.Debug.WriteLine($"Failed to update metering count now: {ex.Message}");
return Content(HttpStatusCode.InternalServerError, new { error = "Failed to update metering count" });
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

The error message 'Failed to update metering count' doesn't match the method name 'UpdateMeteringCountNow'. It should be 'Failed to update metering count now' to be consistent with the method purpose.

Suggested change
return Content(HttpStatusCode.InternalServerError, new { error = "Failed to update metering count" });
return Content(HttpStatusCode.InternalServerError, new { error = "Failed to update metering count now" });

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,6 @@
using Microsoft.AspNetCore.Mvc;
using Newtonsoft.Json;
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Both Newtonsoft.Json and System.Text.Json.Serialization are imported, but only System.Text.Json.Serialization attributes are used in the code. Remove the unused Newtonsoft.Json import.

Suggested change
using Newtonsoft.Json;

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SDKのモデルをJSON化する際にNewtonsoft.Jsonが必要

Comment on lines +644 to +645
var jsonResponse = JsonConvert.SerializeObject(plans.VarPricingPlans);
return Content(jsonResponse, "application/json");
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Using Newtonsoft.Json's JsonConvert.SerializeObject is inconsistent with the System.Text.Json attributes used elsewhere. Consider using the built-in JSON serialization or return Ok() directly instead of manual serialization.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SDKのモデルをJSON化する際にNewtonsoft.Jsonが必要

Comment on lines +676 to +677
var jsonResponse = JsonConvert.SerializeObject(taxRates.VarTaxRates);
return Content(jsonResponse, "application/json");
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

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

Same inconsistency as above - using Newtonsoft.Json serialization with System.Text.Json attributes. Consider using Ok() to return the object directly for consistent JSON serialization.

Suggested change
var jsonResponse = JsonConvert.SerializeObject(taxRates.VarTaxRates);
return Content(jsonResponse, "application/json");
return Ok(taxRates.VarTaxRates);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SDKのモデルをJSON化する際にNewtonsoft.Jsonが必要

@SasakiTakatsugu
Copy link
Contributor Author

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -1,4 +1,6 @@
using Microsoft.AspNetCore.Mvc;
using Newtonsoft.Json;
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Two different JSON serialization libraries are imported. Line 2 imports Newtonsoft.Json (used at line 644) and line 3 imports System.Text.Json.Serialization (used at line 839). This creates an inconsistent approach to JSON handling. Consider standardizing on one library throughout the codebase.

Suggested change
using Newtonsoft.Json;

Copilot uses AI. Check for mistakes.
Comment on lines +644 to +645
var jsonResponse = JsonConvert.SerializeObject(plans.VarPricingPlans);
return Content(jsonResponse, "application/json");
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Manually serializing to JSON and returning Content() bypasses ASP.NET Core's built-in content negotiation and serialization. Consider returning Ok(plans.VarPricingPlans) directly, which is consistent with other endpoints in this controller (e.g., lines 749, 810) and leverages the framework's serialization.

Suggested change
var jsonResponse = JsonConvert.SerializeObject(plans.VarPricingPlans);
return Content(jsonResponse, "application/json");
return Ok(plans.VarPricingPlans);

Copilot uses AI. Check for mistakes.
Comment on lines +676 to +677
var jsonResponse = JsonConvert.SerializeObject(taxRates.VarTaxRates);
return Content(jsonResponse, "application/json");
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Manually serializing to JSON and returning Content() bypasses ASP.NET Core's built-in content negotiation and serialization. Consider returning Ok(taxRates.VarTaxRates) directly, which is consistent with other endpoints in this controller and leverages the framework's serialization.

Suggested change
var jsonResponse = JsonConvert.SerializeObject(taxRates.VarTaxRates);
return Content(jsonResponse, "application/json");
return Ok(taxRates.VarTaxRates);

Copilot uses AI. Check for mistakes.
catch (Exception ex)
{
_logger.LogError(ex, "Failed to get pricing plans");
return StatusCode(500, new { detail = "Internal server error" });
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The error message 'Internal server error' is generic and doesn't help identify the issue. Other error handlers in this file use descriptive messages like 'Failed to get pricing plans'. Consider using 'Failed to get pricing plans' for consistency with the logging statement at line 649.

Suggested change
return StatusCode(500, new { detail = "Internal server error" });
return StatusCode(500, new { detail = "Failed to get pricing plans" });

Copilot uses AI. Check for mistakes.
catch (Exception ex)
{
_logger.LogError(ex, "Failed to get tax rates");
return StatusCode(500, new { detail = "Internal server error" });
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The error message 'Internal server error' is generic and doesn't help identify the issue. Other error handlers in this file use descriptive messages like 'Failed to get tax rates'. Consider using 'Failed to get tax rates' for consistency with the logging statement at line 681.

Suggested change
return StatusCode(500, new { detail = "Internal server error" });
return StatusCode(500, new { detail = "Failed to get tax rates" });

Copilot uses AI. Check for mistakes.
@KooriyamaHiroki KooriyamaHiroki merged commit eac3c62 into main Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants