Conversation
Sync develop into main
…r list (#73) - Define ValorantRole type (DUELIST, INITIATOR, CONTROLLER, SENTINEL) in domain - Add ValorantRoles (custom JSON type) and Description to ContestMember entity - Extend SenderSnapshot and ContestMemberWithUser with new fields - Accept optional request body in POST /api/contests/:id/applications - Validate description max 64 chars and role deduplication in service layer - Include valorant_roles and description in GET /api/contests/:id/members response - Add DB migration 000025 to add valorant_roles JSON and description VARCHAR(64) columns Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the contest application process by allowing users to specify their Valorant in-game roles and provide a short self-introduction. This information is then stored and made accessible through the contest member list API, providing contest leaders with valuable insights into participants' profiles. The changes involve updates across the domain, application, presentation, and infrastructure layers, including new data types, validation rules, API modifications, and database schema updates. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 feature to allow applicants of Valorant contests to specify their roles and a self-introduction. The changes are well-structured, touching the database, domain, application, and presentation layers. My review focuses on improving the validation logic to provide more specific error feedback, enhancing error handling in the controller, and ensuring consistency in error codes and constants.
| // Parse optional request body | ||
| var req dto.RequestParticipateRequest | ||
| // ShouldBindJSON returns error if body exists but is invalid; nil body is OK | ||
| _ = ctx.ShouldBindJSON(&req) |
There was a problem hiding this comment.
The error returned by ctx.ShouldBindJSON(&req) is being ignored. While an empty request body is acceptable, a malformed JSON body will cause ShouldBindJSON to return an error. By ignoring it, the server will proceed as if no body was sent, which is confusing for API clients who provided a body and expected it to be processed. The error should be handled to return a 400 Bad Request for invalid JSON. Note that ShouldBindJSON returns io.EOF for an empty body, which should be explicitly ignored.
| _ = ctx.ShouldBindJSON(&req) | |
| if err := ctx.ShouldBindJSON(&req); err != nil && !errors.Is(err, io.EOF) { | |
| response.JSON(ctx, response.BadRequest("invalid request body: "+err.Error())) | |
| return | |
| } |
| func (s *ContestApplicationService) RequestParticipate(ctx context.Context, contestId, userId int64, req *dto.RequestParticipateRequest) (*dto.DiscordLinkRequiredResponse, error) { | ||
| // Validate request if provided | ||
| if req != nil { | ||
| if len(req.Description) > 64 { |
There was a problem hiding this comment.
The value 64 is a magic number representing the maximum length of the description. This value is also hardcoded in the database migration (VARCHAR(64)). To improve maintainability and ensure consistency, it's best practice to define this as a constant (e.g., domain.MaxDescriptionLength) and reuse it across the application.
| if len(req.ValorantRoles) > 0 { | ||
| roles := domain.ValorantRoles(req.ValorantRoles) | ||
| if !roles.AreValid() { | ||
| return nil, exception.ErrInvalidValorantRole | ||
| } | ||
| } |
There was a problem hiding this comment.
This validation logic returns a generic ErrInvalidValorantRole for any issue with the roles. To provide more specific feedback, the AreValid method in the domain should return a distinct error for duplicate roles (as ErrDuplicateValorantRole has been defined). This requires updating the AreValid method to return an error and then handling that error here.
| if len(req.ValorantRoles) > 0 { | |
| roles := domain.ValorantRoles(req.ValorantRoles) | |
| if !roles.AreValid() { | |
| return nil, exception.ErrInvalidValorantRole | |
| } | |
| } | |
| if len(req.ValorantRoles) > 0 { | |
| roles := domain.ValorantRoles(req.ValorantRoles) | |
| if err := roles.AreValid(); err != nil { | |
| return nil, err | |
| } | |
| } |
| func (vr ValorantRoles) AreValid() bool { | ||
| seen := make(map[ValorantRole]bool) | ||
| for _, role := range vr { | ||
| if !role.IsValid() { | ||
| return false | ||
| } | ||
| if seen[role] { | ||
| return false | ||
| } | ||
| seen[role] = true | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
The AreValid method currently returns a bool, which prevents the caller from distinguishing between an invalid role and a duplicate role. A specific error, ErrDuplicateValorantRole, has been defined but is not used because of this. To provide more specific feedback to the user, this method should be changed to return an error. This will allow the service layer to return the correct error code. Additionally, using map[ValorantRole]struct{} is more idiomatic in Go for implementing a set.
| func (vr ValorantRoles) AreValid() bool { | |
| seen := make(map[ValorantRole]bool) | |
| for _, role := range vr { | |
| if !role.IsValid() { | |
| return false | |
| } | |
| if seen[role] { | |
| return false | |
| } | |
| seen[role] = true | |
| } | |
| return true | |
| } | |
| func (vr ValorantRoles) AreValid() error { | |
| seen := make(map[ValorantRole]struct{}) | |
| for _, role := range vr { | |
| if !role.IsValid() { | |
| return exception.ErrInvalidValorantRole | |
| } | |
| if _, exists := seen[role]; exists { | |
| return exception.ErrDuplicateValorantRole | |
| } | |
| seen[role] = struct{}{} | |
| } | |
| return nil | |
| } |
| ErrInvalidValorantRole = NewBadRequestError("invalid valorant role", "CT035") | ||
| ErrDescriptionTooLong = NewBadRequestError("description must be at most 64 characters", "CT036") | ||
| ErrDuplicateValorantRole = NewBadRequestError("duplicate valorant role", "CT037") |
There was a problem hiding this comment.
There is an inconsistency in the error codes between this file and the PR description. The description states CT035 for ErrDescriptionTooLong and CT036 for ErrInvalidValorantRole, but the codes are swapped here. To maintain consistency and avoid confusion for API consumers, the codes should be corrected to match the documentation.
| ErrInvalidValorantRole = NewBadRequestError("invalid valorant role", "CT035") | |
| ErrDescriptionTooLong = NewBadRequestError("description must be at most 64 characters", "CT036") | |
| ErrDuplicateValorantRole = NewBadRequestError("duplicate valorant role", "CT037") | |
| ErrInvalidValorantRole = NewBadRequestError("invalid valorant role", "CT036") | |
| ErrDescriptionTooLong = NewBadRequestError("description must be at most 64 characters", "CT035") | |
| ErrDuplicateValorantRole = NewBadRequestError("duplicate valorant role", "CT037") |
1 participant
Overview
Closes #73
When applying to a Valorant contest, applicants can now select their in-game role(s) and write a short self-introduction. This information is also exposed in the contest member list API so that contest leaders can review participants' profiles at a glance.
Changes
Domain
ValorantRoletype with 4 constants:DUELIST,INITIATOR,CONTROLLER,SENTINELValorantRolesas a custom JSON-serializable slice type (implementsdriver.Valuer/sql.Scanner) to persist cleanly in MySQL without external librariesValorantRolesandDescriptionfields toContestMemberentityApplication Layer
SenderSnapshot(Redis) andContestMemberWithUser(DB port) with the new fieldsRequestParticipateRequestDTO with optionalvalorant_rolesanddescriptiondescriptionmust not exceed 64 charactersValorantRolevalue must be one of the 4 valid rolesAcceptApplicationnow copies roles and description from the Redis snapshot into the persistedContestMemberPresentation Layer
POST /api/contests/:id/applicationsnow accepts an optional JSON request bodyInfrastructure
GetMembersWithUserByContestquery updated to SELECT the two new columnsContestMemberResponseDTO extended;ToContestMemberResponseupdated accordinglyError Codes
ErrDescriptionTooLongErrInvalidValorantRoleErrDuplicateValorantRoleDatabase Migration
000025_add_valorant_fields_to_contests_membersBoth columns are nullable / default-safe so existing rows are unaffected. A DOWN migration is provided for rollback.
Result
POST /api/contests/:id/applicationsRequest body (optional)
{ "valorant_roles": ["DUELIST", "SENTINEL"], "description": "I can flex any role." }Behavior
game_type != VALORANT→ roles and description are accepted but carry no game-specific meaning (no hard block, keeps the API generic)GET /api/contests/:id/membersResponse (added fields)
{ "user_id": 1, "username": "gamer", "member_type": "NORMAL", "leader_type": "MEMBER", "point": 1200, "valorant_roles": ["DUELIST", "SENTINEL"], "description": "I can flex any role.", ... }