Implement badge service #33
Conversation
…y service (in future this will be a cron job that will be scheduled to run daily)
…andling while fetching repositoryDetails
…ndled in frontend
…y separating logic for ProcessEachContribution
|
/review |
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #7c201c
Actionable Suggestions - 16
-
internal/app/auth/service.go - 1
- Stale user data after account recovery · Line 86-92
-
internal/config/bigquery.go - 1
- Missing resource cleanup method · Line 9-11
-
internal/repository/repository.go - 1
- SQL parameter type mismatch · Line 57-57
-
internal/app/cronJob/dailyJob.go - 1
- Unhandled error in cron job execution · Line 31-31
-
internal/db/migrate.go - 1
- Incorrect parameter name for create operation · Line 208-219
-
internal/app/cronJob/init.go - 1
- Incomplete error handling for job scheduling · Line 35-35
-
internal/repository/user.go - 2
- Critical time calculation error · Line 187-187
- Incorrect error return on success · Line 195-195
-
internal/app/user/service.go - 1
- Missing transaction extraction error handling · Line 129-129
-
internal/app/transaction/domain.go - 2
- Duplicate struct definition causing type conflicts · Line 5-15
- Duplicate struct definition causing type conflicts · Line 17-28
-
internal/db/migrations/1751028730_add-gh-event-id.up.sql - 1
- Potential data integrity issue with default value · Line 1-1
-
internal/app/badge/service.go - 1
- Missing return after successful badge creation · Line 38-39
-
internal/pkg/utils/helper.go - 1
- Invalid month validation logic · Line 73-73
-
internal/app/contribution/handler.go - 2
- Wrong error mapping for year validation · Line 67-67
- Wrong error mapping for month validation · Line 67-67
Additional Suggestions - 1
-
internal/pkg/apperrors/errors.go - 1
-
Fix typo in error message · Line 43-43There is a typo in the error message for `ErrContributionCreationFailed`. The word "contrbitution" should be "contribution". This typo will appear in error responses returned to clients, affecting user experience and potentially causing confusion.
Code suggestion
@@ -43,1 +43,1 @@ - ErrContributionCreationFailed = errors.New("failed to create contrbitution") + ErrContributionCreationFailed = errors.New("failed to create contribution")
-
Review Details
-
Files reviewed - 59 · Commit Range:
3834565..347eb31- .gitignore
- cmd/main.go
- go.mod
- go.sum
- internal/app/auth/service.go
- internal/app/badge/domain.go
- internal/app/badge/handler.go
- internal/app/badge/service.go
- internal/app/bigquery/domain.go
- internal/app/bigquery/service.go
- internal/app/contribution/domain.go
- internal/app/contribution/handler.go
- internal/app/contribution/service.go
- internal/app/cronJob/cleanupJob.go
- internal/app/cronJob/cronjob.go
- internal/app/cronJob/dailyJob.go
- internal/app/cronJob/init.go
- internal/app/dependencies.go
- internal/app/github/domain.go
- internal/app/github/service.go
- internal/app/goal/domain.go
- internal/app/goal/handler.go
- internal/app/goal/service.go
- internal/app/repository/domain.go
- internal/app/repository/handler.go
- internal/app/repository/service.go
- internal/app/router.go
- internal/app/transaction/domain.go
- internal/app/transaction/service.go
- internal/app/user/domain.go
- internal/app/user/handler.go
- internal/app/user/service.go
- internal/config/app.go
- internal/config/bigquery.go
- internal/db/migrate.go
- internal/db/migrations/1748862201_init.up.sql
- internal/db/migrations/1750328591_add_column_contributors_url.down.sql
- internal/db/migrations/1750328591_add_column_contributors_url.up.sql
- internal/db/migrations/1751016438_allow-null-contribution-id.down.sql
- internal/db/migrations/1751016438_allow-null-contribution-id.up.sql
- internal/db/migrations/1751028730_add-gh-event-id.down.sql
- internal/db/migrations/1751028730_add-gh-event-id.up.sql
- internal/db/migrations/1751266661_set-not-null-contributors-url.down.sql
- internal/db/migrations/1751266661_set-not-null-contributors-url.up.sql
- internal/db/migrations/1751268286_set-not-null-gh-event-id.down.sql
- internal/db/migrations/1751268286_set-not-null-gh-event-id.up.sql
- internal/db/migrations/1752476063_create-index-users-current-balance.down.sql
- internal/db/migrations/1752476063_create-index-users-current-balance.up.sql
- internal/pkg/apperrors/errors.go
- internal/pkg/middleware/middleware.go
- internal/pkg/utils/helper.go
- internal/repository/badge.go
- internal/repository/base.go
- internal/repository/contribution.go
- internal/repository/domain.go
- internal/repository/goal.go
- internal/repository/repository.go
- internal/repository/transaction.go
- internal/repository/user.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- GOVULNCHECK (Security Vulnerability) - ✖︎ Failed
- OWASP (Security Vulnerability) - ✔︎ Successful
- SNYK (Security Vulnerability) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| if userData.IsDeleted { | ||
| err = s.userService.RecoverAccountInGracePeriod(ctx, userData.Id) | ||
| if err != nil { | ||
| slog.Error("error in recovering account in grace period during login", "error", err) | ||
| return "", apperrors.ErrInternalServer | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical data inconsistency issue: After successfully recovering a deleted account, the code continues using stale userData that still has IsDeleted=true. The JWT token is generated using userInfo.IsAdmin from GitHub, but other user data remains stale. This creates inconsistent user state and potential security issues. Fix: Refresh userData after successful recovery or update the local userData.IsDeleted field to false.
Code suggestion
Check the AI-generated fix before applying
| if userData.IsDeleted { | |
| err = s.userService.RecoverAccountInGracePeriod(ctx, userData.Id) | |
| if err != nil { | |
| slog.Error("error in recovering account in grace period during login", "error", err) | |
| return "", apperrors.ErrInternalServer | |
| } | |
| } | |
| if userData.IsDeleted { | |
| err = s.userService.RecoverAccountInGracePeriod(ctx, userData.Id) | |
| if err != nil { | |
| slog.Error("error in recovering account in grace period during login", "error", err) | |
| return "", apperrors.ErrInternalServer | |
| } | |
| userData.IsDeleted = false | |
| } |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| type Bigquery struct { | ||
| Client *bigquery.Client | ||
| } |
There was a problem hiding this comment.
The Bigquery struct lacks a Close() method to properly close the underlying BigQuery client. This will cause resource leaks as BigQuery clients maintain connections that should be closed when no longer needed. Add a Close() method that calls Client.Close().
Code suggestion
Check the AI-generated fix before applying
| type Bigquery struct { | |
| Client *bigquery.Client | |
| } | |
| type Bigquery struct { | |
| Client *bigquery.Client | |
| } | |
| func (b *Bigquery) Close() error { | |
| return b.Client.Close() | |
| } |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
|
||
| fetchUsersContributedReposQuery = `SELECT * from repositories where id in (SELECT repository_id from contributions where user_id=$1);` | ||
|
|
||
| fetchUserContributionsInRepoQuery = `SELECT * from contributions where repository_id=$1 and user_id=$2;` |
There was a problem hiding this comment.
SQL parameter mismatch in FetchUserContributionsInRepo. The query expects repository_id as $1 but receives repoGithubId (GitHub repo ID). This will cause incorrect query results or database errors. Fix: Change query to use github_repo_id or first resolve the GitHub ID to internal repository ID.
Code suggestion
Check the AI-generated fix before applying
| fetchUserContributionsInRepoQuery = `SELECT * from contributions where repository_id=$1 and user_id=$2;` | |
| fetchUserContributionsInRepoQuery = `SELECT c.* FROM contributions c JOIN repositories r ON c.repository_id = r.id WHERE r.github_repo_id = $1 AND c.user_id = $2;` |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| } | ||
|
|
||
| func (d *DailyJob) run(ctx context.Context) { | ||
| d.contributionService.ProcessFetchedContributions(ctx) |
There was a problem hiding this comment.
The ProcessFetchedContributions method returns an error that is not being handled. This will cause silent failures in the cron job execution, making it impossible to detect when the daily contribution processing fails. Add error handling with logging to ensure failures are visible.
Code suggestion
Check the AI-generated fix before applying
| d.contributionService.ProcessFetchedContributions(ctx) | |
| if err := d.contributionService.ProcessFetchedContributions(ctx); err != nil { | |
| slog.Error("failed to process fetched contributions", "error", err) | |
| } |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| action := os.Args[1] | ||
| var steps string | ||
| if len(os.Args) > 2 { | ||
| steps = os.Args[2] | ||
| } | ||
| switch action { | ||
| case "up": | ||
| migration.MigrationsUp(os.Args[2]) | ||
| migration.MigrationsUp(steps) | ||
| case "down": | ||
| migration.MigrationsDown(os.Args[2]) | ||
| migration.MigrationsDown(steps) | ||
| case "create": | ||
| migration.CreateMigrationFile(os.Args[2]) | ||
| migration.CreateMigrationFile(steps) |
There was a problem hiding this comment.
Critical bug: CreateMigrationFile is called with steps variable which should contain a filename, not step count. This will cause the create operation to fail when users try to create migration files with numeric names or fail validation. Change the variable name to arg or handle create case separately with proper filename parameter.
Code suggestion
Check the AI-generated fix before applying
| action := os.Args[1] | |
| var steps string | |
| if len(os.Args) > 2 { | |
| steps = os.Args[2] | |
| } | |
| switch action { | |
| case "up": | |
| migration.MigrationsUp(os.Args[2]) | |
| migration.MigrationsUp(steps) | |
| case "down": | |
| migration.MigrationsDown(os.Args[2]) | |
| migration.MigrationsDown(steps) | |
| case "create": | |
| migration.CreateMigrationFile(os.Args[2]) | |
| migration.CreateMigrationFile(steps) | |
| action := os.Args[1] | |
| var arg string | |
| if len(os.Args) > 2 { | |
| arg = os.Args[2] | |
| } | |
| switch action { | |
| case "up": | |
| migration.MigrationsUp(arg) | |
| case "down": | |
| migration.MigrationsDown(arg) | |
| case "create": | |
| migration.CreateMigrationFile(arg) |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @@ -0,0 +1 @@ | |||
| ALTER TABLE contributions ADD COLUMN github_event_id VARCHAR(255) DEFAULT ''; | |||
There was a problem hiding this comment.
The migration adds github_event_id column with DEFAULT '' (empty string), but a subsequent migration sets this column to NOT NULL. If existing records exist when this migration runs, they will get empty string values, which may cause duplicate detection issues in GetContributionByGithubEventId method. Consider using DEFAULT NULL instead or ensure proper data migration.
Code suggestion
Check the AI-generated fix before applying
| ALTER TABLE contributions ADD COLUMN github_event_id VARCHAR(255) DEFAULT ''; | |
| ALTER TABLE contributions ADD COLUMN github_event_id VARCHAR(255) DEFAULT NULL; |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return Badge{}, err | ||
| } |
There was a problem hiding this comment.
Critical logic error: After successfully creating a badge when sql.ErrNoRows is encountered, the code continues to execute lines 37-38 which log an error and return an error, making successful badge creation appear as a failure. Add return Badge(badge), nil after line 35 to fix this.
Code suggestion
Check the AI-generated fix before applying
| return Badge{}, err | |
| } | |
| return Badge{}, err | |
| } | |
| return Badge(badge), nil | |
| } |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return 0, err | ||
| } | ||
|
|
||
| if month < 0 || month > 12 { |
There was a problem hiding this comment.
The month validation logic incorrectly allows month 0, which is invalid. Valid months should be 1-12, not 0-12. Change month < 0 to month < 1 to fix the validation logic.
Code suggestion
Check the AI-generated fix before applying
| if month < 0 || month > 12 { | |
| if month < 1 || month > 12 { |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| month, err := utils.ValidateMonthQueryParam(monthVal) | ||
| if err != nil { | ||
| slog.Error("error converting month value to integer", "error", err) | ||
| status, errorMessage := apperrors.MapError(apperrors.ErrContextValue) |
There was a problem hiding this comment.
Incorrect error mapping for year validation failure. The code maps apperrors.ErrContextValue instead of the actual validation error err returned by utils.ValidateYearQueryParam(). This will return misleading error messages to clients when year validation fails. Change to apperrors.MapError(err).
Code suggestion
Check the AI-generated fix before applying
| status, errorMessage := apperrors.MapError(apperrors.ErrContextValue) | |
| status, errorMessage := apperrors.MapError(err) |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| month, err := utils.ValidateMonthQueryParam(monthVal) | ||
| if err != nil { | ||
| slog.Error("error converting month value to integer", "error", err) | ||
| status, errorMessage := apperrors.MapError(apperrors.ErrContextValue) |
There was a problem hiding this comment.
Incorrect error mapping for month validation failure. The code maps apperrors.ErrContextValue instead of the actual validation error err returned by utils.ValidateMonthQueryParam(). This will return misleading error messages to clients when month validation fails. Change to apperrors.MapError(err).
Code suggestion
Check the AI-generated fix before applying
| status, errorMessage := apperrors.MapError(apperrors.ErrContextValue) | |
| status, errorMessage := apperrors.MapError(err) |
Code Review Run #7c201c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Summary by Bito
This pull request implements a new badge service with supporting infrastructure for assigning badges when users achieve targets. It enhances the contribution processing pipeline by integrating BigQuery for daily contribution retrieval and adds cron jobs for scheduled tasks. The changes also include significant updates to repository layer implementations and database migration scripts to improve system reliability.