Conversation
|
Addressed the open review threads with the following changes:
Validation run:
|
56aeab8 to
5cbe732
Compare
|
fixed |
wxiaoguang
left a comment
There was a problem hiding this comment.
Need more improvements
69458f9 to
106d325
Compare
There was a problem hiding this comment.
Pull request overview
Re-enables and wires up admin-side badge management and display by adding UI pages/routes for creating/editing/viewing badges, plus model/validation/migration support to enforce better data integrity.
Changes:
- Add admin badge management routes, handlers, and templates (list/new/view/edit/users) and expose the nav entry.
- Introduce badge slug validation + i18n strings, and update profile badge rendering to support image-less “chip” badges.
- Add model logic/tests for badges, plus a migration to enforce a unique
(user_id, badge_id)constraint.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web_src/css/user.css | Adjust badge layout to flex-wrap and add styling for text “chip” badges. |
| templates/shared/user/profile_big_avatar.tmpl | Render badges as images when available, otherwise as slug chips. |
| templates/admin/navbar.tmpl | Add “Badges” entry and keep Identity & Access menu expanded on badge pages. |
| templates/admin/badge/view.tmpl | New admin badge detail page (badge info + users preview). |
| templates/admin/badge/users.tmpl | New admin page to add/remove users for a badge. |
| templates/admin/badge/new.tmpl | New admin create-badge form. |
| templates/admin/badge/list.tmpl | New admin badge listing with search + sorting. |
| templates/admin/badge/edit.tmpl | New admin edit/delete badge UI. |
| services/forms/admin.go | Add admin badge create/edit form structs + validation hooks. |
| routers/web/web.go | Register admin /badges routes. |
| routers/web/admin/badges.go | Implement admin badge CRUD + user assignment endpoints and badge search rendering. |
| options/locale/locale_en-US.json | Add i18n strings for badge management + slug validation error. |
| modules/web/middleware/binding.go | Map new badge-slug validation error classification to a translated message. |
| modules/validation/helpers_test.go | Add tests for badge slug validation. |
| modules/validation/helpers.go | Add IsValidBadgeSlug and patterns. |
| modules/validation/binding.go | Add BadgeSlug binding rule and error classification. |
| models/user/badge_test.go | Add model-level tests for badges, users, and searching. |
| models/user/badge.go | Add badge search, badge-user listing, uniqueness index, and improved error semantics. |
| models/migrations/v1_26/v326_test.go | Add migration test for deduping and unique index creation. |
| models/migrations/v1_26/v326.go | Add migration to dedupe user_badge and add compound unique index. |
| models/migrations/migrations.go | Register migration #326. |
| models/fixtures/badge.yml | Add badge fixture data for tests. |
Comments suppressed due to low confidence (1)
models/user/badge.go:158
AddUserBadgesinserts intouser_badgewithout checking whether the user already has the badge. With the new unique constraint (unique_user_badge), attempting to assign the same badge twice will return a raw DB unique-violation error (db.Insert doesn't wrap it), which currently bubbles up to handlers as a 500. Consider detecting duplicates (pre-checkuser_badgeexistence or wrap unique-violation intoutil.ErrAlreadyExist) so callers like the admin UI can show a user-friendly message instead of a server error.
for _, badge := range badges {
// hydrate badge and check if it exists
has, err := db.GetEngine(ctx).Where("slug=?", badge.Slug).Get(badge)
if err != nil {
return err
} else if !has {
return util.NewNotExistErrorf("badge does not exist [slug: %s]", badge.Slug)
}
if err := db.Insert(ctx, &UserBadge{
BadgeID: badge.ID,
UserID: u.ID,
}); err != nil {
return err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
The more I think about it the more I think images are not useful here and maybe a tab like bade could be enough |
|
Cannot reproduce the errors on my machine :-( |
da63623 to
525b0bb
Compare
|
This comment was written by Claude on behalf of @silverwind Review
|
|
Fixed |
|
PR title updated to be more accurate. |
|
Updated to main and resolved conflicts |
|
I think this PR's code is good enough. I don't need badges and don't know what the origin requirement is. So the maintainers from the origin PR can help to review: @techknowlogick |
|
reopened - if there is still need for it sure but I want to focus on other stuff to improve currently |


trying to reenable #31262