プラン設定機能追加 #7
プラン設定機能追加 #7
Conversation
SasakiTakatsugu
requested review from
KooriyamaHiroki,
bigdragon0610 and
silent-bayline
There was a problem hiding this comment.
Pull Request Overview
This PR adds plan management functionality to the billing system, enabling retrieval of pricing plans, tax rates, and tenant plan information, as well as updating tenant plans.
- Adds new API endpoints for retrieving pricing plans and tax rates
- Implements tenant plan information retrieval with current and reservation details
- Adds tenant plan update functionality with support for plan changes, cancellations, and reservation management
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| routes/api.php | Adds four new API routes for plan management functionality |
| app/Http/Controllers/BillingController.php | Implements controller methods for pricing plans, tax rates, and tenant plan operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| $plans = $this->pricing->getPricingPlans(); | ||
| return response()->json($plans->getPricingPlans()); |
There was a problem hiding this comment.
The method getPricingPlans() is called twice - once on the service and once on the returned object. This creates confusion and potential for errors. Consider simplifying to either return the service result directly or clarify the method naming.
| $plans = $this->pricing->getPricingPlans(); | |
| return response()->json($plans->getPricingPlans()); | |
| $plansResponse = $this->pricing->getPricingPlans(); | |
| return response()->json($plansResponse->getPricingPlans()); |
| { | ||
| try { | ||
| $taxRates = $this->pricing->getTaxRates(); | ||
| return response()->json($taxRates->getTaxRates()); |
There was a problem hiding this comment.
Similar to the pricing plans endpoint, getTaxRates() is called twice which creates unnecessary complexity. Consider returning the service result directly or using clearer method naming.
| return response()->json($taxRates->getTaxRates()); | |
| return response()->json($taxRates); |
| $updateParam->next_plan_id = $nextPlanId; | ||
|
|
||
| // 税率IDが指定されている場合のみ設定 | ||
| if ($taxRateId && $taxRateId !== '') { |
There was a problem hiding this comment.
[nitpick] The condition $taxRateId && $taxRateId !== '' could be simplified to just !empty($taxRateId) for better readability and consistency.
| if ($taxRateId && $taxRateId !== '') { | |
| if (!empty($taxRateId)) { |
| } | ||
|
|
||
| // using_next_plan_fromが指定されている場合のみ設定 | ||
| if ($usingNextPlanFrom && $usingNextPlanFrom > 0) { |
There was a problem hiding this comment.
[nitpick] The validation logic mixes truthiness check with numeric comparison. Consider using explicit validation like if ($usingNextPlanFrom !== null && $usingNextPlanFrom > 0) for clearer intent.
| if ($usingNextPlanFrom && $usingNextPlanFrom > 0) { | |
| if ($usingNextPlanFrom !== null && $usingNextPlanFrom > 0) { |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!$tenantId) { | ||
| return response()->json(['error' => 'tenant_id is required'], Response::HTTP_BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
This validation is redundant. The $tenantId parameter comes from the route and will always have a value when this method is called. Laravel ensures route parameters are present, so this check will never trigger.
| if (!$tenantId) { | |
| return response()->json(['error' => 'tenant_id is required'], Response::HTTP_BAD_REQUEST); | |
| } |
| if (!$tenantId) { | ||
| return response()->json(['error' => 'tenant_id is required'], Response::HTTP_BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
This validation is redundant. The $tenantId parameter comes from the route and will always have a value when this method is called. Laravel ensures route parameters are present, so this check will never trigger.
| if (!$tenantId) { | |
| return response()->json(['error' => 'tenant_id is required'], Response::HTTP_BAD_REQUEST); | |
| } |
No description provided.