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 adds plan configuration functionality to the billing system, introducing endpoints for managing tenant pricing plans and tax rates. The changes enable users to view available pricing plans, tax rates, and current tenant plan information, as well as update tenant plans with reservation capabilities.

  • Added four new REST endpoints for plan management operations
  • Introduced UpdateTenantPlanRequest model for plan update requests
  • Implemented plan reservation functionality using the SaaS SDK's PlanReservation model

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

Comment on lines 40 to 41
tax_rate_id: str = None
using_next_plan_from: int = None
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.

Use Optional[str] and Optional[int] instead of assigning None as default values. Import Optional from typing module for better type safety and clarity.

Copilot uses AI. Check for mistakes.
Comment on lines 384 to 385
except Exception as e:
raise HTTPException(status_code=500, detail="Internal server error")
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 'Internal server error' is too generic and doesn't provide useful debugging information. Consider logging the actual exception and providing a more specific error message.

Copilot uses AI. Check for mistakes.
Comment on lines 401 to 402
except Exception as e:
raise HTTPException(status_code=500, detail="Internal server error")
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 'Internal server error' is too generic and doesn't provide useful debugging information. Consider logging the actual exception and providing a more specific error message.

Copilot uses AI. Check for mistakes.
)

# 税率IDが指定されている場合のみ設定
if request.tax_rate_id and request.tax_rate_id != "":
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 condition request.tax_rate_id and request.tax_rate_id != \"\" can be simplified to just request.tax_rate_id since an empty string is falsy in Python, or use request.tax_rate_id.strip() if you want to handle whitespace-only strings.

Suggested change
if request.tax_rate_id and request.tax_rate_id != "":
if request.tax_rate_id:

Copilot uses AI. Check for mistakes.
plan_reservation.next_plan_tax_rate_id = request.tax_rate_id

# using_next_plan_fromが指定されている場合のみ設定
if request.using_next_plan_from and request.using_next_plan_from > 0:
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 condition checks both truthiness and > 0, but since using_next_plan_from is an int, checking > 0 alone would be sufficient and clearer (0 is falsy anyway).

Suggested change
if request.using_next_plan_from and request.using_next_plan_from > 0:
if request.using_next_plan_from > 0:

Copilot uses AI. Check for mistakes.
Comment on lines 493 to 494
except Exception as e:
raise HTTPException(status_code=500, detail="Failed to update tenant plan")
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 tenant plan' is generic and doesn't provide useful debugging information. Consider logging the actual exception and providing more specific error details.

Copilot uses AI. Check for mistakes.
@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 1 out of 1 changed files in this pull request and generated 8 comments.


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

Comment on lines 377 to 378
if not auth_user.tenants:
raise HTTPException(status_code=400, detail="No tenants found for the user")
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 tenant check in get_pricing_plans is unnecessary since pricing plans are not tenant-specific. This endpoint retrieves all available pricing plans in the system, not tenant-specific data. The check should be removed as it incorrectly prevents users without tenants from viewing available plans.

Suggested change
if not auth_user.tenants:
raise HTTPException(status_code=400, detail="No tenants found for the user")

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.

対応しました。

Comment on lines 394 to 396
if not auth_user.tenants:
raise HTTPException(status_code=400, detail="No tenants found for the user")

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 tenant check in get_tax_rates is unnecessary since tax rates are not tenant-specific. This endpoint retrieves all available tax rates in the system, not tenant-specific data. The check should be removed as it incorrectly prevents users without tenants from viewing available tax rates.

Suggested change
if not auth_user.tenants:
raise HTTPException(status_code=400, detail="No tenants found for the user")

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.

対応しました。

tax_rates = TaxRateApi(api_client=pricing_api_client).get_tax_rates()
return tax_rates.tax_rates
except Exception as e:
raise HTTPException(status_code=500, detail="Failed to retrieve tax rates")
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 lacks context about the actual error. Consider including the exception details or a more specific error message to aid in debugging. For example: detail=f\"Failed to retrieve tax rates: {str(e)}\"

Suggested change
raise HTTPException(status_code=500, detail="Failed to retrieve tax rates")
raise HTTPException(status_code=500, detail=f"Failed to retrieve tax rates: {str(e)}")

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.

エラー詳細はprintでコンソール出力、フロントには出さない形式で対応。


return response
except Exception as e:
raise HTTPException(status_code=500, detail="Failed to retrieve tenant detail")
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 lacks context about the actual error. Consider including the exception details or a more specific error message to aid in debugging. For example: detail=f\"Failed to retrieve tenant detail: {str(e)}\"

Suggested change
raise HTTPException(status_code=500, detail="Failed to retrieve tenant detail")
raise HTTPException(status_code=500, detail=f"Failed to retrieve tenant detail: {str(e)}")

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.

エラー詳細はprintでコンソール出力、フロントには出さない形式で対応。


return {"message": "Tenant plan updated successfully"}
except Exception as e:
raise HTTPException(status_code=500, detail="Failed to update tenant plan")
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 lacks context about the actual error. Consider including the exception details or a more specific error message to aid in debugging. For example: detail=f\"Failed to update tenant plan: {str(e)}\"

Suggested change
raise HTTPException(status_code=500, detail="Failed to update tenant plan")
raise HTTPException(status_code=500, detail=f"Failed to update tenant plan: {str(e)}")

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.

エラー詳細はprintでコンソール出力、フロントには出さない形式で対応。

Comment on lines 414 to 415
if not tenant_id:
raise HTTPException(status_code=400, detail="tenant_id is required")
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.

This validation is redundant since tenant_id is a required path parameter in the route definition. FastAPI automatically ensures path parameters are present and non-empty, so this check will never trigger and should be removed.

Suggested change
if not tenant_id:
raise HTTPException(status_code=400, detail="tenant_id is required")

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.

対応しました。

Comment on lines 465 to 466
if not tenant_id:
raise HTTPException(status_code=400, detail="tenant_id is required")
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.

This validation is redundant since tenant_id is a required path parameter in the route definition. FastAPI automatically ensures path parameters are present and non-empty, so this check will never trigger and should be removed.

Suggested change
if not tenant_id:
raise HTTPException(status_code=400, detail="tenant_id is required")

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.

対応しました。

Comment on lines +482 to +483
# 税率IDが指定されている場合のみ設定
if request.tax_rate_id:
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 condition checks for truthiness (if request.tax_rate_id:) which will be False for empty strings, but the comment on line 476 indicates empty strings should be handled. Change to if request.tax_rate_id is not None: to match the behavior of next_plan_id and allow empty strings to be set.

Suggested change
# 税率IDが指定されている場合のみ設定
if request.tax_rate_id:
# 税率IDがNoneでない場合は設定(空文字も含む)
if request.tax_rate_id is not None:

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

@SasakiTakatsugu SasakiTakatsugu Oct 23, 2025

Choose a reason for hiding this comment

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

next_plan_idは解除予約で空文字が必要、tax_rate_idは不要のため対応しない。

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.

2 participants