feat: Add sort order column to categories #151
Conversation
Summary of ChangesHello @gocanto, 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 introduces a new sorting mechanism for categories by adding a dedicated 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Repo
participant DB
Note over Handler,Repo: Sort-aware retrieval (primary: sort ASC, secondary: name ASC)
Client->>Handler: GET /categories
Handler->>Repo: GetAll(order="sort ASC, name ASC")
Repo->>DB: SELECT ... ORDER BY sort ASC, name ASC
DB-->>Repo: rows(sorted)
Repo-->>Handler: []Category{..., Sort:int}
Handler-->>Client: 200 OK { categories: [{slug,name,sort}, ...] }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 sort column for categories to allow for custom ordering. The changes are well-implemented across the database migration, GORM models, repository queries, and API responses. The new functionality is also covered by tests. My review includes a few suggestions to improve maintainability and robustness: one regarding a potential conflict between the GORM model and the database migration for an index, another about the logic for updating the sort value which prevents setting it to zero, and a final one to enhance test coverage for the secondary sort order.
gocanto
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
database/model.go (1)
112-112: Remove the index tag to avoid conflict with the migration.The
index:idx_categories_sorttag will instruct GORM to create a single-column index onsort, but the migration000004_categories_sort.up.sqlcreates a composite indexidx_categories_sorton(sort, name). This naming conflict can cause issues ifAutoMigrateis used. Remove the index tag and rely exclusively on migrations for schema management.Apply this diff:
- Sort int `gorm:"type:int;not null;index:idx_categories_sort"` + Sort int `gorm:"type:int;not null"`database/repository/categories_postgres_test.go (1)
27-66: Good coverage of primary and secondary ordering.Both tests assert Sort ASC and Name ASC tie-breaks; inserting in reverse ensures the ORDER BY is effective.
Also applies to: 68-107
🧹 Nitpick comments (2)
database/attrs.go (1)
24-24: Addition looks correct; confirm domain rules for Sort.Type aligns with the model. If negative values should be disallowed, add a DB CHECK or validate at write-time to prevent invalid sort orders.
handler/categories_test.go (1)
42-42: Good assertion of Sort in payload; consider adding an ordering case.Add a second category (same Sort, different Name) and assert handler output is ordered by Sort then Name to mirror repository behavior.
Also applies to: 79-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
database/attrs.go(1 hunks)database/infra/migrations/000004_categories_sort.down.sql(1 hunks)database/infra/migrations/000004_categories_sort.up.sql(1 hunks)database/model.go(1 hunks)database/repository/categories.go(4 hunks)database/repository/categories_postgres_test.go(2 hunks)database/seeder/importer/runner_test.go(1 hunks)database/seeder/seeds/categories.go(1 hunks)handler/categories.go(1 hunks)handler/categories_test.go(2 hunks)handler/payload/categories.go(2 hunks)handler/payload/categories_test.go(1 hunks)handler/payload/posts_response_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
database/seeder/seeds/categories.go (1)
database/model.go (1)
Category(106-119)
handler/payload/categories_test.go (1)
handler/payload/categories.go (1)
GetCategoriesResponse(13-27)
database/repository/categories_postgres_test.go (2)
database/model.go (1)
Category(106-119)database/repository/categories.go (1)
Categories(13-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (1.25.3)
🔇 Additional comments (14)
database/infra/migrations/000004_categories_sort.down.sql (1)
1-3: LGTM!The rollback migration correctly drops the index before the column, and both statements use
IF EXISTSguards for safe, idempotent rollback.handler/categories.go (1)
44-44: LGTM!The Sort field is correctly populated from the database model to the API response payload.
database/repository/categories.go (4)
23-23: LGTM!The ordering by
sort asc, name asccorrectly implements the intended category ordering behavior.
57-57: LGTM!Consistent ordering applied to GetAll, matching the behavior in Get().
108-108: LGTM!Sort value correctly propagated from seed data to the new category record.
145-146: LGTM!The Sort field is now updated unconditionally in ExistOrUpdate, correctly allowing Sort to be synchronized with seed data (including zero values if provided).
Note: The past review comment about preventing Sort=0 updates appears outdated, as the current code has no such condition.
handler/payload/categories.go (2)
10-10: LGTM!Sort field correctly added to CategoryResponse with proper JSON tag.
22-22: LGTM!Sort field correctly populated in the response mapping function.
handler/payload/posts_response_test.go (2)
28-28: LGTM!Test fixture correctly updated to include the new Sort field.
54-54: LGTM!The test assertion correctly verifies that the Sort field is present and properly populated in the response.
database/seeder/importer/runner_test.go (1)
503-504: LGTM!The test INSERT statement correctly includes the new sort column with an appropriate test value.
database/seeder/seeds/categories.go (1)
29-36: LGTM!The seeder correctly generates deterministic sort values (1, 2, 3, ..., 10) for the categories, aligning with the PR objectives.
handler/payload/categories_test.go (1)
16-16: LGTM — payload includes Sort and assertion matches.Also applies to: 22-22
database/infra/migrations/000004_categories_sort.up.sql (1)
1-7: Align GORM tag to composite index definition or drop and rely on migrations.The migration is correct—
CREATE INDEX IF NOT EXISTS idx_categories_sort ON categories (sort, name)properly creates the composite index. However, the GORM tag onSort(line 112 indatabase/model.go) does not reflect this:
- Current:
Sort intwithgorm:"...index:idx_categories_sort"(single-column tag)- Expected: both
SortandNameshould referenceindex:idx_categories_sortwith priorities, OR the tag should be dropped entirelySince production code does not use
AutoMigrate(migration execution is SQL-only viadatabase/infra/migrations/), the mismatch is a code maintenance issue rather than a production bug. However, it creates future risk if AutoMigrate is introduced.Choose one:
Fix GORM tag to document the composite index:
- Add
index:idx_categories_sort,priority:1to Name field- Adjust Sort field to
index:idx_categories_sort,priority:2Drop GORM tag and rely on migrations (recommended if migrations are the source of truth):
- Remove
index:idx_categories_sortfrom Sort field- Add comment noting the index is defined in
000004_categories_sort.up.sqlThe optional suggestions (CONCURRENTLY, CHECK constraint) remain valid if needed for your use case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
database/repository/testhelpers_test.go (1)
116-116: Consider making the sort value configurable.Hardcoding
Sort: 1means all categories created by this helper will have identical sort values. This limits the ability to test sort ordering behavior when multiple categories are involved, as they'll all fall back to ordering by name.Consider one of these approaches:
- Add a
sort intparameter to allow callers to specify the sort value- Use a counter/incrementing value for deterministic but varied sort values
- Document that this helper creates categories with the same sort value
Example with a parameter:
-func seedCategory(t *testing.T, conn *database.Connection, slug, name string) database.Category { +func seedCategory(t *testing.T, conn *database.Connection, slug, name string, sort int) database.Category { t.Helper() category := database.Category{ UUID: uuid.NewString(), Slug: slug, Name: name, - Sort: 1, + Sort: sort, }handler/categories_test.go (1)
100-110: Validation is correct; consider adding test case for different Sort values.The assertions properly verify the Sort field and the alphabetical ordering when Sort values are equal. However, this test doesn't cover the primary sorting behavior when categories have different Sort values.
Consider adding a complementary test case to validate primary Sort ordering:
func TestCategoriesHandlerIndex_SortOrdering(t *testing.T) { conn, author := handlertests.NewTestDB(t) published := time.Now() post := database.Post{ UUID: uuid.NewString(), AuthorID: author.ID, Slug: "hello", Title: "Hello", Excerpt: "Ex", Content: "Body", PublishedAt: &published, } if err := conn.Sql().Create(&post).Error; err != nil { t.Fatalf("create post: %v", err) } catLow := database.Category{ UUID: uuid.NewString(), Name: "Zebra", Slug: "zebra", Description: "desc", Sort: 5, } if err := conn.Sql().Create(&catLow).Error; err != nil { t.Fatalf("create category: %v", err) } catHigh := database.Category{ UUID: uuid.NewString(), Name: "Apple", Slug: "apple", Description: "desc", Sort: 20, } if err := conn.Sql().Create(&catHigh).Error; err != nil { t.Fatalf("create category: %v", err) } // Link both to post for _, catID := range []uint64{catLow.ID, catHigh.ID} { link := database.PostCategory{PostID: post.ID, CategoryID: catID} if err := conn.Sql().Create(&link).Error; err != nil { t.Fatalf("create link: %v", err) } } h := NewCategoriesHandler(&repository.Categories{DB: conn}) req := httptest.NewRequest("GET", "/categories", nil) rec := httptest.NewRecorder() if err := h.Index(rec, req); err != nil { t.Fatalf("index err: %v", err) } var resp pagination.Pagination[payload.CategoryResponse] if err := json.NewDecoder(rec.Body).Decode(&resp); err != nil { t.Fatalf("decode: %v", err) } if len(resp.Data) != 2 { t.Fatalf("unexpected data length: %+v", resp.Data) } // Zebra (Sort 5) should come before Apple (Sort 20) if resp.Data[0].Name != "Zebra" || resp.Data[0].Sort != 5 { t.Fatalf("unexpected first category: %+v", resp.Data[0]) } if resp.Data[1].Name != "Apple" || resp.Data[1].Sort != 20 { t.Fatalf("unexpected second category: %+v", resp.Data[1]) } }This would validate that categories with lower Sort values appear first, regardless of alphabetical name ordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
database/model.go(1 hunks)database/repository/repository_test.go(2 hunks)database/repository/testhelpers_test.go(1 hunks)handler/categories_test.go(2 hunks)metal/cli/posts/handler_test.go(1 hunks)metal/cli/seo/testhelpers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- database/model.go
🧰 Additional context used
🧬 Code graph analysis (1)
handler/categories_test.go (1)
database/model.go (2)
Category(106-119)PostCategory(121-126)
🔇 Additional comments (5)
metal/cli/posts/handler_test.go (1)
49-49: LGTM! Test fixture properly updated.The addition of
Sort: 1to the test Category aligns with the new schema and maintains test data consistency.metal/cli/seo/testhelpers_test.go (1)
134-134: LGTM! Correctly initializes the new Sort field.The addition of
Sort: 1properly initializes the new field and is consistent with the minimal-parameter pattern used in other seed helpers in this file.handler/categories_test.go (1)
37-77: LGTM! Good test setup for secondary ordering.The test correctly creates two categories with identical Sort values (10) but different names, validating that alphabetical ordering by Name applies when Sort values are equal. Creating Beta before Alpha and expecting Alpha first in results confirms the ordering is query-driven rather than insertion-order-dependent.
database/repository/repository_test.go (2)
141-168: LGTM! Appropriate test fixture update.The addition of
Sort: 1correctly updates the test fixture to accommodate the new Sort field. The test remains focused on its original purpose (testing FindBy functionality), and the Sort value is deterministic and appropriate for a test fixture.
170-252: LGTM! Consistent fixture update.The addition of
Sort: 1to the category fixture is consistent with the change on line 152 and appropriately updates the test to work with the new Sort field. The test correctly maintains its focus on post creation and retrieval functionality.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
database/repository/categories_postgres_test.go (1)
109-150: Consider adding a test for updating Sort to a non-zero value.This test correctly verifies that passing Sort=0 in the update attributes preserves the existing Sort value while updating other fields. However, there's no corresponding test that verifies actually updating Sort to a different non-zero value (e.g., from 25 to 30). Adding such a test would provide more complete coverage of the update behavior.
Example test to add:
func TestCategoriesExistOrUpdateUpdatesSortWhenNonZero(t *testing.T) { conn := newPostgresConnection(t, &database.Category{}) repo := repository.Categories{DB: conn} original := database.Category{ UUID: uuid.NewString(), Name: "Original", Slug: "original", Sort: 25, } if err := conn.Sql().Create(&original).Error; err != nil { t.Fatalf("create original: %v", err) } existed, err := repo.ExistOrUpdate(database.CategoriesAttrs{ Slug: "original", Name: "Renamed", Sort: 30, }) if err != nil { t.Fatalf("exist or update: %v", err) } if !existed { t.Fatalf("expected category to exist") } var updated database.Category if err := conn.Sql().Where("id = ?", original.ID).First(&updated).Error; err != nil { t.Fatalf("reload category: %v", err) } if updated.Sort != 30 { t.Fatalf("expected sort to be updated to 30, got %d", updated.Sort) } if updated.Name != "Renamed" { t.Fatalf("expected name to be updated, got %s", updated.Name) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
database/repository/categories.go(3 hunks)database/repository/categories_postgres_test.go(2 hunks)database/repository/posts_postgres_test.go(5 hunks)database/repository/testhelpers_test.go(1 hunks)handler/categories_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- database/repository/categories.go
🧰 Additional context used
🧬 Code graph analysis (3)
database/repository/categories_postgres_test.go (3)
database/model.go (1)
Category(106-119)database/repository/categories.go (1)
Categories(13-15)database/attrs.go (1)
CategoriesAttrs(19-25)
database/repository/testhelpers_test.go (1)
database/model.go (1)
Category(106-119)
handler/categories_test.go (5)
database/model.go (3)
Category(106-119)PostCategory(121-126)Post(83-104)handler/tests/db.go (1)
NewTestDB(18-88)handler/categories.go (1)
NewCategoriesHandler(20-24)database/repository/categories.go (1)
Categories(13-15)handler/payload/categories.go (1)
CategoryResponse(5-11)
🔇 Additional comments (7)
handler/categories_test.go (2)
37-111: LGTM! Test correctly verifies Sort field in responses and secondary Name ordering.The updated test now creates two categories with identical Sort values (10) and verifies they are returned in alphabetical order by Name (Alpha before Beta). The assertions correctly check the Sort field in the response payload.
113-192: LGTM! Test correctly verifies primary Sort ordering.This test creates categories with names in reverse alphabetical order (Zebra before Apple) but assigns them Sort values that should reverse the display order (5 before 20). This ensures the ordering is based on Sort and not Name, providing good coverage of the primary sorting behavior.
database/repository/posts_postgres_test.go (1)
24-24: LGTM! seedCategory calls correctly updated with sort parameter.All seedCategory calls have been updated to include the new sort parameter with appropriate test values (1 or 2). These changes align with the updated helper function signature.
Also applies to: 92-92, 141-141, 199-200, 243-243
database/repository/testhelpers_test.go (1)
109-124: LGTM! Helper function correctly updated to support Sort field.The seedCategory helper function signature and implementation have been properly updated to accept and set the Sort parameter on the Category struct.
database/repository/categories_postgres_test.go (3)
14-14: LGTM! Test updated with sort parameter.The seedCategory call has been correctly updated to include the sort parameter.
27-66: LGTM! Test correctly verifies primary Sort ordering.This test creates categories in reverse Sort order (High with Sort=20 first, then Low with Sort=10) and verifies they are returned in ascending Sort order. This ensures the ordering is based on the Sort field and not insertion order.
68-107: Excellent! This test addresses the previous review comment.This test verifies the secondary sort criterion (Name) when Sort values are equal. By creating Bravo before Alpha (both with Sort=10), it confirms that the results are ordered alphabetically by Name when Sort values match, directly addressing the concern raised in the previous review.
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Testing
Summary by CodeRabbit
New Features
Migrations
Seeder
Tests