募金登録と更新APIの作成#996
募金登録と更新APIの作成#996hikahana wants to merge 7 commits intofeat/hikahana/add-get-department-by-donationfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements POST and PUT API endpoints for campus donation management, refactoring the existing GET-only functionality. The changes span the full stack from database schema to OpenAPI specification and frontend TypeScript code generation.
Changes:
- Added POST /campus_donations and PUT /campus_donations/{id} endpoints for creating and updating campus donations
- Updated database schema to add year_id column and remove is_first_check/is_last_check columns
- Generated TypeScript models and API hooks for the new endpoints
- Implemented controller, usecase, and repository layers for create/update operations
Reviewed changes
Copilot reviewed 20 out of 32 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| openapi/openapi.yaml | Added POST and PUT endpoint definitions with request/response schemas |
| api/externals/controller/cmapus_donation_controller.go | Implemented CreateCampusDonation and UpdateCampusDonation controller methods |
| api/internals/usecase/campus_donation_usecase.go | Added CreateCampusDonation and UpdateCampusDonation usecase methods |
| api/externals/repository/campus_donation_repository.go | Added CreateCampusDonation and UpdateCampusDonation repository methods with SQL generation |
| api/router/router.go | Registered new POST and PUT routes |
| api/generated/openapi_gen.go | Auto-generated Go types and interfaces from OpenAPI spec |
| mysql/db/20_campus_donations.sql | Updated table schema to add year_id and remove check columns |
| view/next-project/src/generated/* | Auto-generated TypeScript models, hooks, and API functions |
| er/* | Auto-generated ER diagrams and documentation reflecting schema changes |
| func (f *campusDonationController) UpdateCampusDonation(c echo.Context) error { | ||
| ctx := c.Request().Context() | ||
| id := c.QueryParam("id") | ||
| userId := c.QueryParam("user_id") | ||
| teacherId := c.QueryParam("teacher_id") | ||
| price := c.QueryParam("price") | ||
| receivedAt := c.QueryParam("received_at") | ||
| yearId := c.QueryParam("year_id") | ||
|
|
||
| if id == "" { | ||
| return c.String(http.StatusBadRequest, "id is required") | ||
| } | ||
| if userId == "" { | ||
| return c.String(http.StatusBadRequest, "user_id is required") | ||
| } | ||
| if teacherId == "" { | ||
| return c.String(http.StatusBadRequest, "teacher_id is required") | ||
| } | ||
| if price == "" { | ||
| return c.String(http.StatusBadRequest, "price is required") | ||
| } | ||
| if receivedAt == "" { | ||
| return c.String(http.StatusBadRequest, "received_at is required") | ||
| } | ||
| if yearId == "" { | ||
| return c.String(http.StatusBadRequest, "year_id is required") | ||
| } | ||
|
|
||
| err := f.u.UpdateCampusDonation(ctx, id, userId, teacherId, price, receivedAt, yearId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return c.NoContent(http.StatusOK) | ||
| } |
There was a problem hiding this comment.
The UpdateCampusDonation method is reading parameters from query parameters using c.QueryParam instead of from the JSON request body using c.Bind. According to the OpenAPI specification, this endpoint expects a JSON request body (application/json with schema putRequestBodyCampusDonation), not query parameters.
Additionally, the id parameter should be read from the path parameter using c.Param("id") as defined in the route "/campus_donations/:id", not from query parameters.
This is inconsistent with the OpenAPI specification and with other controllers in the codebase (e.g., activity_controller.go line 68) which use c.Bind to parse JSON request bodies and c.Param for path parameters in PUT requests.
| CREATE TABLE campus_donations ( | ||
| id int(10) unsigned not null auto_increment, | ||
| user_id int(10) not null, | ||
| teacher_id int(10) not null, | ||
| price int(10) not null, | ||
| remark varchar(255), | ||
| received_at varchar(255) not null, | ||
| year_id INT(10) UNSIGNED NOT NULL, | ||
| created_at datetime not null default current_timestamp, | ||
| updated_at datetime not null default current_timestamp on update current_timestamp, | ||
| PRIMARY KEY (id) | ||
| ); |
There was a problem hiding this comment.
The database schema shows that is_first_check and is_last_check columns still exist in the database (lines 197-229 in the HTML ER diagram show these as BIT type nullable columns). However, the SQL migration file removed these columns. This indicates a discrepancy between the actual database schema documentation and the SQL migration script.
If the intention is to remove these columns, the ER diagram should not show them. If they should be kept, the SQL migration should include them.
| putRequestBodyCampusDonation: | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: integer | ||
| example: 1 | ||
| user_id: | ||
| type: integer | ||
| example: 1 | ||
| teacher_id: | ||
| type: integer | ||
| example: 1 | ||
| price: | ||
| type: integer | ||
| example: 10000 | ||
| received_at: | ||
| type: string | ||
| example: 2024-01-01 | ||
| year_id: | ||
| type: integer | ||
| example: 1 |
There was a problem hiding this comment.
The putRequestBodyCampusDonation schema includes an 'id' field in the request body. This is redundant and inconsistent with REST API best practices, as the ID is already specified in the URL path parameter (/campus_donations/{id}). The ID in the request body could potentially conflict with the ID in the URL path, leading to confusion or errors.
Best practice is to only use the path parameter for the resource ID in PUT requests, not include it in the request body.
| postRequestBodyCampusDonation: | ||
| type: object | ||
| properties: | ||
| user_id: | ||
| type: integer | ||
| example: 1 | ||
| teacher_id: | ||
| type: integer | ||
| example: 1 | ||
| price: | ||
| type: integer | ||
| example: 10000 | ||
| received_at: | ||
| type: string | ||
| example: 2024-01-01 | ||
| year_id: | ||
| type: integer | ||
| example: 1 |
There was a problem hiding this comment.
All fields in the OpenAPI schema (postRequestBodyCampusDonation and putRequestBodyCampusDonation) are marked as optional (using the '?' in TypeScript and no 'required' array in the OpenAPI spec). However, in the database schema, most of these fields are NOT NULL (user_id, teacher_id, price, received_at, year_id).
This mismatch means the API could accept requests with missing required fields, which would then fail at the database layer with less helpful error messages. The OpenAPI schema should include a 'required' array listing the mandatory fields to ensure proper validation at the API layer.
| postRequestBodyCampusDonation: | ||
| type: object | ||
| properties: | ||
| user_id: | ||
| type: integer | ||
| example: 1 | ||
| teacher_id: | ||
| type: integer | ||
| example: 1 | ||
| price: | ||
| type: integer | ||
| example: 10000 | ||
| received_at: | ||
| type: string | ||
| example: 2024-01-01 | ||
| year_id: | ||
| type: integer | ||
| example: 1 | ||
| putRequestBodyCampusDonation: | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: integer | ||
| example: 1 | ||
| user_id: | ||
| type: integer | ||
| example: 1 | ||
| teacher_id: | ||
| type: integer | ||
| example: 1 | ||
| price: | ||
| type: integer | ||
| example: 10000 | ||
| received_at: | ||
| type: string | ||
| example: 2024-01-01 | ||
| year_id: | ||
| type: integer | ||
| example: 1 |
There was a problem hiding this comment.
The database schema includes a 'remark' column (varchar(255), nullable), but the OpenAPI request body schemas (postRequestBodyCampusDonation and putRequestBodyCampusDonation) do not include this field. This means clients cannot set the remark field when creating or updating campus donations through the API, even though the database supports it.
Consider whether the remark field should be included in the API schemas, or if it's intentionally excluded for business logic reasons.
| func (fir *campusDonationRepository) CreateCampusDonation(c context.Context, userId string, teacherId string, price string, receivedAt string, yearId string) error { | ||
| dbDialect := goqu.Dialect("mysql") | ||
| ds := dbDialect.Insert("campus_donations").Rows(goqu.Record{ | ||
| "user_id": userId, | ||
| "teacher_id": teacherId, | ||
| "price": price, | ||
| "received_at": receivedAt, | ||
| "year_id": yearId, | ||
| }) | ||
| query, _, err := ds.ToSQL() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return fir.crud.UpdateDB(c, query) | ||
| } | ||
|
|
||
| func (fir *campusDonationRepository) UpdateCampusDonation(c context.Context, id string, userId string, teacherId string, price string, receivedAt string, yearId string) error { | ||
| dbDialect := goqu.Dialect("mysql") | ||
| ds := dbDialect.Update("campus_donations").Set(goqu.Record{ | ||
| "user_id": userId, | ||
| "teacher_id": teacherId, | ||
| "price": price, | ||
| "received_at": receivedAt, | ||
| "year_id": yearId, | ||
| }).Where(goqu.Ex{"id": id}) | ||
| query, _, err := ds.ToSQL() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return fir.crud.UpdateDB(c, query) | ||
| } |
There was a problem hiding this comment.
The repository methods CreateCampusDonation and UpdateCampusDonation accept all parameters as strings, which differs from the pattern used in other repositories in this codebase. Other repositories (e.g., festival_item_repository.go) accept properly typed structs or individual parameters with appropriate types.
While goqu.Record can handle type conversion, passing strings for integer fields (userId, teacherId, price, yearId) is inconsistent with the codebase conventions and makes the type system less effective. Consider accepting properly typed parameters (int for IDs and price, string for receivedAt) or a struct like the generated PostRequestBodyCampusDonation/PutRequestBodyCampusDonation types.
| func (f *campusDonationUseCase) CreateCampusDonation(c context.Context, userId string, teacherId string, price string, receivedAt string, yearId string) error { | ||
| err := f.rep.CreateCampusDonation(c, userId, teacherId, price, receivedAt, yearId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (f *campusDonationUseCase) UpdateCampusDonation(c context.Context, id string, userId string, teacherId string, price string, receivedAt string, yearId string) error { | ||
| err := f.rep.UpdateCampusDonation(c, id, userId, teacherId, price, receivedAt, yearId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The usecase methods CreateCampusDonation and UpdateCampusDonation accept all parameters as strings and pass them directly to the repository without any validation or type checking. This violates the single responsibility principle and misses an opportunity to validate business rules at the usecase layer.
The usecase layer should validate that required fields are present, that numeric strings are valid integers, that dates are in the correct format, etc. Currently, if invalid data is passed (e.g., non-numeric strings for user_id), the error will only be caught at the database layer, resulting in less helpful error messages.
| func (f *campusDonationController) CreateCampusDonation(c echo.Context) error { | ||
| ctx := c.Request().Context() | ||
| userId := c.QueryParam("user_id") | ||
| teacherId := c.QueryParam("teacher_id") | ||
| price := c.QueryParam("price") | ||
| receivedAt := c.QueryParam("received_at") | ||
| yearId := c.QueryParam("year_id") | ||
|
|
||
| if userId == "" { | ||
| return c.String(http.StatusBadRequest, "user_id is required") | ||
| } | ||
| if teacherId == "" { | ||
| return c.String(http.StatusBadRequest, "teacher_id is required") | ||
| } | ||
| if price == "" { | ||
| return c.String(http.StatusBadRequest, "price is required") | ||
| } | ||
| if receivedAt == "" { | ||
| return c.String(http.StatusBadRequest, "received_at is required") | ||
| } | ||
| if yearId == "" { | ||
| return c.String(http.StatusBadRequest, "year_id is required") | ||
| } | ||
|
|
||
| err := f.u.CreateCampusDonation(ctx, userId, teacherId, price, receivedAt, yearId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return c.NoContent(http.StatusCreated) | ||
| } |
There was a problem hiding this comment.
The CreateCampusDonation method is reading parameters from query parameters using c.QueryParam instead of from the JSON request body using c.Bind. According to the OpenAPI specification, this endpoint expects a JSON request body (application/json with schema postRequestBodyCampusDonation), not query parameters.
This is inconsistent with the OpenAPI specification and with other controllers in the codebase (e.g., activity_controller.go, receipt_controller.go) which use c.Bind to parse JSON request bodies for POST requests.
The method should bind the JSON request body to a struct (generated.PostRequestBodyCampusDonation) and extract the values from there.
1 participant
対応Issue
resolve #0
概要
本プルリクエストでは、キャンパス寄付機能に関する全体的な実装とリファクタリングを行いました。主な変更点は以下の通りです。
データベース・リポジトリ層
ユースケース層
コントローラー層
OpenAPI定義およびルーティング
これらの変更により、キャンパス寄付機能の作成、更新、取得が一貫したインターフェースで実現され、システム全体の整合性が向上しました。
画面スクリーンショット等
テスト項目
備考