Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new endpoint to fetch per-building campus donation totals for a specified year, including handler, use case, repository, domain row model, DI wiring, OpenAPI spec, and minor frontend formatting and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as "HTTP Handler\n(api/externals)"
participant UseCase as "CampusDonationUseCase\n(api/internals)"
participant Repo as "CampusDonationRepository\n(api/externals)"
participant DB as "Database"
Client->>Handler: GET /campus_donations/buildings/{year}
Handler->>UseCase: GetBuildingTotalsByYear(ctx, year)
UseCase->>Repo: AllBuildingTotalsByYear(ctx, year)
Repo->>DB: QueryContext(SQL with JOINs, SUM, GROUP BY)
DB-->>Repo: rows
Repo-->>UseCase: rows
UseCase->>UseCase: scan rows -> map to BuildingTotal[]
UseCase-->>Handler: []BuildingTotal
Handler-->>Client: 200 OK + JSON (building totals)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Deploying finansu with
|
| Latest commit: |
b704750
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://055fd259.finansu.pages.dev |
| Branch Preview URL: | https://codex-campus-donations-build.finansu.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to retrieve campus donation totals grouped by building for a specific year, including the necessary API handlers, use cases, and repository logic. Feedback highlights a potential logic error in the SQL query that could lead to double-counting donations for teachers associated with multiple buildings. Additionally, a critical bug was identified in the repository where the database statement is closed prematurely, which will cause errors when reading query results in the use case layer. It is also recommended to use integer types for the year parameter across all layers to maintain type consistency.
| defer func() { | ||
| if err := stmt.Close(); err != nil { | ||
| fmt.Println(err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
stmt.Close() が defer で呼び出されているため、この関数が値を返した直後にステートメントがクローズされます。Goの database/sql パッケージでは、Stmt をクローズするとそのステートメントから生成された Rows もクローズされるため、呼び出し元の usecase で rows.Next() を実行する際にエラーが発生します。プリペアドステートメントを明示的に再利用する必要がない場合は、cdr.client 等を介して直接 QueryContext を実行するか、rows の処理が終わるまで stmt をクローズしないように制御する必要があります。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
openapi/openapi.yaml (1)
1295-1300: Consider constrainingyearto valid positive values.Adding
minimum: 1(or your valid lower bound) to the path parameter schema would reject invalid values earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi/openapi.yaml` around lines 1295 - 1300, The path parameter "year" currently allows any integer; constrain it by adding a lower bound to its schema (e.g., minimum: 1) so invalid/non-positive years are rejected early. Locate the parameter block for name: year in the OpenAPI spec and add a "minimum: 1" entry under its schema (keeping type: integer), and optionally adjust/add "maximum" or "format" if you have a specific valid range.api/externals/repository/campus_donation_repository.go (1)
18-19: Useintforyearacross repository/usecase boundaries.This API path parameter is already typed as integer, so keeping
yearasinthere removes unnecessary conversions and keeps contracts stricter end-to-end.Also applies to: 27-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/externals/repository/campus_donation_repository.go` around lines 18 - 19, Change the year parameter type from string to int across the repository/usecase boundary: update the CampusDonationRepository interface method AllBuildingTotalsByYear(ctx context.Context, year int) (*sql.Rows, error) and any other repository methods referenced on lines 27-28 to accept int, then update all implementing types/methods (e.g., concrete repository struct methods) and callers (usecases, handlers, tests) to pass an int instead of a string so there are no runtime conversions and the contract remains consistent end-to-end.api/internals/usecase/campus_donation_usecase.go (1)
17-19: Decouple use case from OpenAPI-generated transport models.Using
generated.BuildingTotalin the use case interface couples internal business logic to API schema regeneration. Prefer returning a domain/usecase DTO and map togenerated.*in the handler layer.Also applies to: 46-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/internals/usecase/campus_donation_usecase.go` around lines 17 - 19, The use case interface CampusDonationUseCase currently returns API transport models generated.BuildingTotal which couples business logic to OpenAPI codegen; introduce a domain/usecase DTO (e.g., type BuildingTotal in the usecase or domain package) and change the method signature of GetBuildingTotalsByYear to return []usecase.BuildingTotal (or domain.BuildingTotal) and error, then update the concrete implementation(s) of CampusDonationUseCase to populate and return that DTO; finally perform the mapping from the new usecase/domain BuildingTotal to generated.BuildingTotal inside the HTTP handler layer where the OpenAPI models are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/externals/repository/campus_donation_repository.go`:
- Around line 72-83: The code prepares a statement with cdr.crud.Prepare and
defers stmt.Close() before returning stmt.QueryContext(c, year), which can close
the statement (and underlying resources) before the caller reads *sql.Rows;
instead, avoid deferring stmt.Close() here and call QueryContext directly on the
database connection (e.g., use cdr.crud.QueryContext(c, query, year) or
equivalent) so the returned *sql.Rows remains valid; update the function to
remove the prepare + deferred Close and invoke QueryContext on cdr.crud (or
ensure the caller is responsible for closing rows) referencing cdr.crud.Prepare,
stmt.Close, and stmt.QueryContext.
In `@api/internals/usecase/campus_donation_usecase.go`:
- Around line 25-31: The GetBuildingTotalsByYear method on campusDonationUseCase
currently passes the raw year string to the repository
(AllBuildingTotalsByYear); add input validation at the start of
GetBuildingTotalsByYear to ensure year is a valid 4-digit year (e.g., regex
`^\d{4}$` or parsing to int and range-check) and return a clear validation error
(not a DB error) when invalid; keep the repository call
(AllBuildingTotalsByYear) unchanged and only call it after the year passes
validation to prevent downstream query failures.
---
Nitpick comments:
In `@api/externals/repository/campus_donation_repository.go`:
- Around line 18-19: Change the year parameter type from string to int across
the repository/usecase boundary: update the CampusDonationRepository interface
method AllBuildingTotalsByYear(ctx context.Context, year int) (*sql.Rows, error)
and any other repository methods referenced on lines 27-28 to accept int, then
update all implementing types/methods (e.g., concrete repository struct methods)
and callers (usecases, handlers, tests) to pass an int instead of a string so
there are no runtime conversions and the contract remains consistent end-to-end.
In `@api/internals/usecase/campus_donation_usecase.go`:
- Around line 17-19: The use case interface CampusDonationUseCase currently
returns API transport models generated.BuildingTotal which couples business
logic to OpenAPI codegen; introduce a domain/usecase DTO (e.g., type
BuildingTotal in the usecase or domain package) and change the method signature
of GetBuildingTotalsByYear to return []usecase.BuildingTotal (or
domain.BuildingTotal) and error, then update the concrete implementation(s) of
CampusDonationUseCase to populate and return that DTO; finally perform the
mapping from the new usecase/domain BuildingTotal to generated.BuildingTotal
inside the HTTP handler layer where the OpenAPI models are used.
In `@openapi/openapi.yaml`:
- Around line 1295-1300: The path parameter "year" currently allows any integer;
constrain it by adding a lower bound to its schema (e.g., minimum: 1) so
invalid/non-positive years are rejected early. Locate the parameter block for
name: year in the OpenAPI spec and add a "minimum: 1" entry under its schema
(keeping type: integer), and optionally adjust/add "maximum" or "format" if you
have a specific valid range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddd6b8dc-ceb9-4919-ba88-0490989a8fe1
⛔ Files ignored due to path filters (4)
api/generated/openapi_gen.gois excluded by!**/generated/**view/next-project/src/generated/hooks.tsis excluded by!**/generated/**view/next-project/src/generated/model/buildingTotal.tsis excluded by!**/generated/**view/next-project/src/generated/model/index.tsis excluded by!**/generated/**
📒 Files selected for processing (9)
api/externals/handler/campus_donation_handler.goapi/externals/handler/handler.goapi/externals/repository/campus_donation_repository.goapi/externals/repository/wire.goapi/internals/di/wire_gen.goapi/internals/domain/campus_donation.goapi/internals/usecase/campus_donation_usecase.goapi/internals/usecase/wire.goopenapi/openapi.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/externals/repository/campus_donation_repository.go (1)
17-18: Keep*sql.Rowsinside the repository boundary.Returning
*sql.Rowsleaks DB lifecycle handling to the use case and couples the caller to SQL column order. Since this is a new repository API, I'd return[]domain.CampusDonationBuildingTotalRow(or a repository DTO) from here and scan inside the repository instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/externals/repository/campus_donation_repository.go` around lines 17 - 18, The method signature on the CampusDonationRepository interface (AllBuildingTotalsByYear) returns *sql.Rows which leaks DB lifecycle and couples callers to SQL scanning; change the interface to return ([]domain.CampusDonationBuildingTotalRow, error) (or a repo DTO slice), implement the repository method to perform the query, iterate and sql.Scan each row into a domain.CampusDonationBuildingTotalRow, close rows in a defer, handle and wrap query/scan errors, and update any callers to consume the returned slice instead of managing *sql.Rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/externals/repository/campus_donation_repository.go`:
- Around line 17-18: The method signature on the CampusDonationRepository
interface (AllBuildingTotalsByYear) returns *sql.Rows which leaks DB lifecycle
and couples callers to SQL scanning; change the interface to return
([]domain.CampusDonationBuildingTotalRow, error) (or a repo DTO slice),
implement the repository method to perform the query, iterate and sql.Scan each
row into a domain.CampusDonationBuildingTotalRow, close rows in a defer, handle
and wrap query/scan errors, and update any callers to consume the returned slice
instead of managing *sql.Rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b54f6899-5070-4cbd-b38d-a5ee5ec62a12
📒 Files selected for processing (1)
api/externals/repository/campus_donation_repository.go
…ations-buildings-api
|
|
||
| groupedBuildingTotals := make([]generated.BuildingTotal, 0, len(campusDonationBuildingGroups)) | ||
| for _, group := range campusDonationBuildingGroups { | ||
| totalPrice, ok := totalPriceByGroupID[group.ID] |
There was a problem hiding this comment.
金額0円の棟も0円としてレスポンス返してほしいかも。
その方がフロントで扱いやすいんじゃないかと思った。
これしたいけど、たしか、事務棟とかは一つの部屋だけど複数人いるとかだった気がするからこのままでいきます！ |
2 participants
対応Issue
resolve #0
概要
GET /campus_donations/buildings/{year}を追加campus_donations -> teachers -> room_teachers -> rooms -> buildings -> yearsを用いて集計room_teachers経由の重複計上を避けるため、棟と募金の組み合わせを一度DISTINCT化してから合算画面スクリーンショット等
URLAPI追加のみのためなし
スクリーンショット
テスト項目
go build ./...が通ることGET /campus_donations/buildings/{year}で各棟の合計募金額が取得できること2025指定時に電気棟: 3000,機械・建設棟: 5000が返ること以下をコマンド実行したらデータ追加されます。レスポンスも下に貼ります。
レスポンス
備考
developにはbuildings/rooms統合後のマイグレーションがまだ入っていないため、HTTP動作確認は新スキーマ相当を当てた確認用DBで実施しています。a80e374b61eb1eb19b7cd17a808d89231e1f43d3Summary by CodeRabbit
New Features
Documentation
Style