Conversation
v0関連の型・ハンドラーを削除し、Createレスポンスを201に変更、 新規のDetailエンドポイントハンドラーを追加した。 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request adapts the codebase to schema changes by removing deprecated v0 API endpoints and fields while adding a new detail endpoint for announcements.
Changes:
- Removed deprecated v0
/announcementsendpoint and associated handlers - Added new
GET /v1/announcements/{id}detail endpoint - Removed deprecated
dateandisActivefields from domain, database, and API models - Updated create endpoint response code from 200 to 201 (more RESTful)
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openapi/openapi.yaml | Removed v0 endpoint, added v1 detail endpoint, removed deprecated fields from schema, changed create response to 201 |
| internal/domain/announcement.go | Removed deprecated Date and IsActive fields |
| internal/database/announcement.go | Removed deprecated fields, removed explicit table name method, removed varchar length constraints |
| internal/repository/announcement.go | Updated query to use available_until instead of is_active, updated sort to use available_from instead of date |
| internal/handler/get_announcements_v0.go | Deleted v0 list handler |
| internal/handler/get_announcements_v1.go | Renamed converter function from toDomainAnnouncementQueryV1 to toDomainAnnouncementQuery |
| internal/handler/get_announcement_v1.go | New detail handler for fetching single announcement by ID |
| internal/handler/create_announcement.go | Removed time.Now() parameter, changed response type to 201 |
| internal/handler/update_announcement.go | Removed time.Now() parameter from converter call |
| internal/handler/converter.go | Removed deprecated field mappings and v0 converter functions, removed time.Now() parameter from converter |
| internal/handler/announcement_test.go | Renamed test from V0 to V1, updated test data and assertions |
| generated/api.gen.go | Removed v0 endpoint code, added v1 detail endpoint code, removed deprecated fields, changed create response to 201 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| announcement, err := h.announcementService.GetAnnouncementByID(ctx, request.Id) | ||
| if err != nil { | ||
| if errors.Is(err, domain.ErrNotFound) { | ||
| return nil, err // TODO: 404レスポンスを返すべき |
There was a problem hiding this comment.
The TODO comment indicates incomplete error handling. The handler should return a proper 404 response when an announcement is not found, but currently it's returning the error directly which may not provide the correct HTTP status code. Consider implementing proper 404 response handling similar to other HTTP handlers in the codebase.
| return nil, err // TODO: 404レスポンスを返すべき | |
| return api.AnnouncementsV1Detail404JSONResponse{}, nil |
| func (h *Handler) AnnouncementsV1Detail(ctx context.Context, request api.AnnouncementsV1DetailRequestObject) (api.AnnouncementsV1DetailResponseObject, error) { | ||
| announcement, err := h.announcementService.GetAnnouncementByID(ctx, request.Id) | ||
| if err != nil { | ||
| if errors.Is(err, domain.ErrNotFound) { | ||
| return nil, err // TODO: 404レスポンスを返すべき | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| return api.AnnouncementsV1Detail200JSONResponse{ | ||
| Announcement: toApiAnnouncement(announcement), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The new detail endpoint AnnouncementsV1Detail lacks test coverage. The codebase has test coverage for the list endpoint (TestAnnouncementsV1List), so this new endpoint should also have corresponding tests to verify correct behavior, error handling for non-existent IDs, and proper response formatting.
| Title string `gorm:"not null"` | ||
| URL string `gorm:"not null"` |
There was a problem hiding this comment.
The removal of varchar length constraints from Title and URL fields may have database migration implications. The previous constraints were type:varchar(500) for Title and type:varchar(1000) for URL. When GORM AutoMigrate runs, it may not automatically adjust existing column types in the database. Consider adding a database migration to ensure the schema is updated properly, or verify that the database already has compatible column types that can handle the change.
やったこと
確認したこと