repo details page #1888
repo details page #1888
Conversation
|
Warning Rate limit exceeded@motatoes has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces several backend changes. A new API endpoint ( Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Router
participant API as GetJobsForRepoApi
participant DB as Database
C->>R: GET /api/repos/:repo_id/jobs
R->>API: Route request to GetJobsForRepoApi
API->>DB: Query organization using context (orgID, source)
DB-->>API: Return organization details
API->>DB: Fetch repository details using repo_id
DB-->>API: Return repository details
API->>DB: Retrieve job details via GetJobsByRepoName
DB-->>API: Return job query results
API->>C: 200 OK with jobs JSON
sequenceDiagram
participant C as Client
participant R as Router
participant API as GetDashboardStatusApi
participant DB as Database
C->>R: GET /dashboard/status (assumed endpoint)
R->>API: Route request to GetDashboardStatusApi
API->>DB: Query organization using context (orgID, source)
DB-->>API: Return organization or error
API->>C: 200 OK with dashboard JSON (or 404 if not found)
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds repository details functionality and dashboard statistics, introducing a new JobQueryResult struct and API endpoints for querying job data.
- Added
JobQueryResultstruct inbackend/models/queries.gofor structured job query responses - Implemented
GetJobsForRepoApiendpoint inbackend/controllers/jobs.goto retrieve jobs for specific repositories - Added dashboard statistics functions in
backend/models/storage_dashboard.gofor counting repos and jobs - Created
GetDashboardStatusApiinbackend/controllers/dashboard.gobut it returns an empty response - Missing organization ID filter in job count functions could cause incorrect statistics across all organizations
💡 (4/5) You can add custom instructions or style guidelines for the bot here!
6 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
backend/controllers/jobs.go
Outdated
| if errors.Is(err, gorm.ErrRecordNotFound) { | ||
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | ||
| } | ||
| return |
There was a problem hiding this comment.
logic: Missing return statement after sending error response. If organization is not found, the function should return after sending the error response.
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| } | |
| return | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| return | |
| } | |
| return |
backend/controllers/jobs.go
Outdated
| log.Printf("could not fetch repo details") | ||
| c.String(http.StatusInternalServerError, "Unknown error occurred while fetching jobs from database") |
There was a problem hiding this comment.
style: Log message doesn't include the actual error which makes debugging difficult
| log.Printf("could not fetch repo details") | |
| c.String(http.StatusInternalServerError, "Unknown error occurred while fetching jobs from database") | |
| log.Printf("could not fetch repo details: %v", err) | |
| c.String(http.StatusInternalServerError, "Unknown error occurred while fetching jobs from database") |
backend/controllers/dashboard.go
Outdated
| if err != nil { | ||
| if errors.Is(err, gorm.ErrRecordNotFound) { | ||
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
logic: Missing proper error handling for non-ErrRecordNotFound errors. Should return an appropriate HTTP status code and error message.
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| } | |
| return | |
| } | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| return | |
| } | |
| c.String(http.StatusInternalServerError, "Error retrieving organisation") | |
| return | |
| } |
backend/models/storage_dashboard.go
Outdated
| result := db.GormDB.Model(DiggerJob{}). | ||
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | ||
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | ||
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | ||
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | ||
| Count(&count) |
There was a problem hiding this comment.
logic: Missing WHERE clause to filter by the provided organization ID. This will count jobs for all organizations, not just the specified one.
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | |
| Count(&count) | |
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ? AND organisations.id = ?", thirtyDaysAgo, orgID). | |
| Count(&count) |
backend/models/storage_dashboard.go
Outdated
| result := db.GormDB.Model(DiggerJob{}). | ||
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | ||
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | ||
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | ||
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | ||
| Where("digger_jobs.status = ?", scheduler.DiggerJobSucceeded). |
There was a problem hiding this comment.
logic: Missing WHERE clause to filter by the provided organization ID. This will count successful jobs for all organizations, not just the specified one.
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | |
| Where("digger_jobs.status = ?", scheduler.DiggerJobSucceeded). | |
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ? AND organisations.id = ?", thirtyDaysAgo, orgID). | |
| Where("digger_jobs.status = ?", scheduler.DiggerJobSucceeded). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
backend/controllers/dashboard.go (1)
12-28: Return meaningful dashboard data instead of empty responseThe function correctly retrieves the organization but returns an empty response object. If this is intended as a placeholder for future dashboard statistics, consider adding a comment indicating this is a work in progress.
Additionally, the error handling only addresses record not found cases but doesn't handle other potential database errors. Consider adding proper logging for other error scenarios:
func GetDashboardStatusApi(c *gin.Context) { organisationId := c.GetString(middleware.ORGANISATION_ID_KEY) organisationSource := c.GetString(middleware.ORGANISATION_SOURCE_KEY) var org models.Organisation err := models.DB.GormDB.Where("external_id = ? AND external_source = ?", organisationId, organisationSource).First(&org).Error if err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) + } else { + log.Printf("Error finding organisation %s: %v", organisationId, err) + c.String(http.StatusInternalServerError, "Error retrieving organization data") } return } response := make(map[string]interface{}) + // TODO: Add dashboard statistics data to the response c.JSON(http.StatusOK, response) }backend/models/storage.go (1)
927-946: Add pagination to prevent large result setsThe query retrieves all jobs for a repository without pagination, which could potentially return large datasets for repositories with many jobs. Consider adding pagination parameters to limit the result set size.
func (db *Database) GetJobsByRepoName(orgId uint, repoFullName string) ([]JobQueryResult, error) { var results []JobQueryResult + // Default pagination values + limit := 100 + offset := 0 query := ` SELECT j.id, j.created_at, j.updated_at, j.deleted_at, j.digger_job_id, j.status, j.workflow_run_url, j.workflow_file, j.terraform_output, db.pr_number, db.repo_full_name, db.branch_name FROM digger_jobs j, digger_batches db, organisations o, github_app_installation_links l WHERE o.id = l.organisation_id AND l.github_installation_id = db.github_installation_id AND db.id = j.batch_id AND o.id = ? AND db.repo_full_name = ? ORDER BY j.created_at + LIMIT ? OFFSET ? ` - err := db.GormDB.Raw(query, orgId, repoFullName).Scan(&results).Error + err := db.GormDB.Raw(query, orgId, repoFullName, limit, offset).Scan(&results).Error return results, err }Additionally, consider adding error logging to provide more context when errors occur.
backend/controllers/jobs.go (4)
20-23: Fix error message to match parameter nameThe error message mentions "repo_full_name" but the parameter being checked is actually "repo_id".
- log.Printf("missing parameter: repo_full_name") - c.String(http.StatusBadRequest, "missing parameter: repo_full_name") + log.Printf("missing parameter: repo_id") + c.String(http.StatusBadRequest, "missing parameter: repo_id")
35-40: Improve error logging and handlingThe error message is generic and doesn't include the actual error details, making debugging more difficult. Also, consider handling "not found" errors differently than other errors.
repo, err := models.DB.GetRepoById(org.ID, repoId) if err != nil { - log.Printf("could not fetch repo details") + log.Printf("could not fetch repo details: %v", err) + if errors.Is(err, gorm.ErrRecordNotFound) { + c.String(http.StatusNotFound, "Repository not found") + return + } c.String(http.StatusInternalServerError, "Unknown error occurred while fetching jobs from database") return }
42-47: Improve error logging with detailsSimilar to the previous comment, include the actual error details in the log message to facilitate debugging.
jobsRes, err := models.DB.GetJobsByRepoName(org.ID, repo.RepoFullName) if err != nil { - log.Printf("could not fetch job details") + log.Printf("could not fetch job details: %v", err) c.String(http.StatusInternalServerError, "Unknown error occurred while fetching jobs from database") return }
49-58: Fix error log message and consider handling empty statusThe error message incorrectly states "could not convert status to string" when it's actually converting from string to int. Also, consider handling the case where status might be empty.
// update the values of "status" accordingly for i, j := range jobsRes { + if j.Status == "" { + log.Printf("empty status for job id: %v", j.ID) + jobsRes[i].Status = "unknown" + continue + } statusInt, err := strconv.Atoi(j.Status) if err != nil { - log.Printf("could not convert status to string: job id: %v status: %v", j.ID, j.Status) + log.Printf("could not convert status to int: job id: %v status: %v", j.ID, j.Status) continue } statusI := orchestrator_scheduler.DiggerJobStatus(statusInt) jobsRes[i].Status = statusI.ToString() }backend/models/storage_dashboard.go (2)
1-7: Suggestion: Add unit tests for new dashboard functionsThe new dashboard query functions are important for statistics but lack unit tests to verify their behavior, especially considering the complex JOINs and filtering logic.
I recommend adding unit tests to verify:
- The correct time filtering (30 days)
- Organization filtering
- Status filtering for successful jobs
- Error handling
This is especially important given the identified issues with missing organization filters.
11-11: Refactor: Extract duplicate time calculation to helper functionThe same
thirtyDaysAgocalculation is duplicated in all three functions. This could be extracted to a helper function or method to improve maintainability.+ func getThirtyDaysAgo() time.Time { + return time.Now().AddDate(0, 0, -30) + } func (db *Database) GetRepoCount(orgID uint) (int64, error) { var count int64 - thirtyDaysAgo := time.Now().AddDate(0, 0, -30) + thirtyDaysAgo := getThirtyDaysAgo() // ... }Also applies to: 27-27, 46-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/bootstrap/main.go(1 hunks)backend/controllers/dashboard.go(1 hunks)backend/controllers/jobs.go(1 hunks)backend/models/queries.go(1 hunks)backend/models/storage.go(1 hunks)backend/models/storage_dashboard.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (3)
backend/models/queries.go (1)
5-18: LGTM! Well-structured data model.The
JobQueryResultstruct is well-defined with appropriate GORM tags for database column mapping and includes all necessary fields to represent job query results. The struct provides a good foundation for querying and returning job-related data.backend/bootstrap/main.go (1)
223-223: LGTM! New endpoint follows established patterns.The new endpoint for retrieving repository jobs follows the existing API structure and naming conventions. This is a clean addition that complements the dashboard statistics functionality.
backend/models/storage_dashboard.go (1)
8-22: Approved: Repository count function implementationThe GetRepoCount function correctly filters by organization ID and creation date, with proper error handling.
backend/controllers/jobs.go
Outdated
| if err != nil { | ||
| if errors.Is(err, gorm.ErrRecordNotFound) { | ||
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Add missing return statement after error response
When an error is encountered finding the organization, the function sets the status code and error message but there's no explicit return statement. This could lead to the function continuing execution unexpectedly.
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
c.String(http.StatusNotFound, "Could not find organisation: "+organisationId)
+ return
}
+ log.Printf("error finding organisation: %v", err)
+ c.String(http.StatusInternalServerError, "Error finding organisation")
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| } | |
| return | |
| } | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) | |
| return | |
| } | |
| log.Printf("error finding organisation: %v", err) | |
| c.String(http.StatusInternalServerError, "Error finding organisation") | |
| return | |
| } |
backend/models/storage_dashboard.go
Outdated
| func (db *Database) GetJobsCountThisMonth(orgID uint) (int64, error) { | ||
| var count int64 | ||
|
|
||
| thirtyDaysAgo := time.Now().AddDate(0, 0, -30) | ||
|
|
||
| result := db.GormDB.Model(DiggerJob{}). | ||
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | ||
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | ||
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | ||
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | ||
| Count(&count) | ||
|
|
||
| if result.Error != nil { | ||
| return 0, result.Error | ||
| } | ||
|
|
||
| return count, nil | ||
| } |
There was a problem hiding this comment.
Missing organization filter in jobs count query
The GetJobsCountThisMonth function doesn't filter by the provided orgID parameter in its WHERE clause, which seems inconsistent with the function's purpose. This would return counts for all organizations instead of just the requested one.
result := db.GormDB.Model(DiggerJob{}).
Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id").
Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id").
Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id").
Where("digger_jobs.created_at >= ?", thirtyDaysAgo).
+ Where("organisations.id = ?", orgID).
Count(&count)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (db *Database) GetJobsCountThisMonth(orgID uint) (int64, error) { | |
| var count int64 | |
| thirtyDaysAgo := time.Now().AddDate(0, 0, -30) | |
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | |
| Count(&count) | |
| if result.Error != nil { | |
| return 0, result.Error | |
| } | |
| return count, nil | |
| } | |
| func (db *Database) GetJobsCountThisMonth(orgID uint) (int64, error) { | |
| var count int64 | |
| thirtyDaysAgo := time.Now().AddDate(0, 0, -30) | |
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | |
| Where("organisations.id = ?", orgID). | |
| Count(&count) | |
| if result.Error != nil { | |
| return 0, result.Error | |
| } | |
| return count, nil | |
| } |
backend/models/storage_dashboard.go
Outdated
| func (db *Database) GetSuccessfulJobsCountThisMonth(orgID uint) (int64, error) { | ||
| var count int64 | ||
|
|
||
| thirtyDaysAgo := time.Now().AddDate(0, 0, -30) | ||
|
|
||
| result := db.GormDB.Model(DiggerJob{}). | ||
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | ||
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | ||
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | ||
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | ||
| Where("digger_jobs.status = ?", scheduler.DiggerJobSucceeded). | ||
| Count(&count) | ||
|
|
||
| if result.Error != nil { | ||
| return 0, result.Error | ||
| } | ||
|
|
||
| return count, nil | ||
| } |
There was a problem hiding this comment.
Missing organization filter in successful jobs count query
Similar to the previous function, this query is missing the organization filter. Also, consider combining the WHERE clauses for better readability.
result := db.GormDB.Model(DiggerJob{}).
Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id").
Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id").
Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id").
- Where("digger_jobs.created_at >= ?", thirtyDaysAgo).
- Where("digger_jobs.status = ?", scheduler.DiggerJobSucceeded).
+ Where("digger_jobs.created_at >= ? AND digger_jobs.status = ? AND organisations.id = ?",
+ thirtyDaysAgo, scheduler.DiggerJobSucceeded, orgID).
Count(&count)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (db *Database) GetSuccessfulJobsCountThisMonth(orgID uint) (int64, error) { | |
| var count int64 | |
| thirtyDaysAgo := time.Now().AddDate(0, 0, -30) | |
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ?", thirtyDaysAgo). | |
| Where("digger_jobs.status = ?", scheduler.DiggerJobSucceeded). | |
| Count(&count) | |
| if result.Error != nil { | |
| return 0, result.Error | |
| } | |
| return count, nil | |
| } | |
| func (db *Database) GetSuccessfulJobsCountThisMonth(orgID uint) (int64, error) { | |
| var count int64 | |
| thirtyDaysAgo := time.Now().AddDate(0, 0, -30) | |
| result := db.GormDB.Model(DiggerJob{}). | |
| Joins("JOIN digger_batches ON digger_jobs.batch_id = digger_batches.id"). | |
| Joins("JOIN github_app_installation_links ON digger_batches.github_installation_id = github_app_installation_links.github_installation_id"). | |
| Joins("JOIN organisations ON github_app_installation_links.organisation_id = organisations.id"). | |
| Where("digger_jobs.created_at >= ? AND digger_jobs.status = ? AND organisations.id = ?", thirtyDaysAgo, scheduler.DiggerJobSucceeded, orgID). | |
| Count(&count) | |
| if result.Error != nil { | |
| return 0, result.Error | |
| } | |
| return count, nil | |
| } |
also added some helper queries for dashboard stats
Summary by CodeRabbit