Skip to content

Feature/retry badges#36752

Open
bircni wants to merge 3 commits intogo-gitea:mainfrom
bircni:feature/retry-badges
Open

Feature/retry badges#36752
bircni wants to merge 3 commits intogo-gitea:mainfrom
bircni:feature/retry-badges

Conversation

@bircni
Copy link
Contributor

@bircni bircni commented Feb 25, 2026

trying to reenable #31262

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 25, 2026
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations labels Feb 25, 2026
@bircni
Copy link
Contributor Author

bircni commented Feb 25, 2026

Addressed the open review threads with the following changes:

  1. Removed services/user/badge.go wrappers (UpdateBadge, DeleteBadge, GetBadgeUsers) and switched admin badge handlers to direct model calls.
  2. Removed redundant transaction wrapping by dropping the service wrappers and calling user_model.UpdateBadge / user_model.DeleteBadge directly.
  3. Fixed badge edit form semantics: removed invalid disabled usage on parent container, removed non-local, and made slug display readonly.
  4. Removed unnecessary id/for attributes in templates/admin/badge/edit.tmpl where framework defaults are sufficient.
  5. Removed no-op delete class from the badge delete modal.
  6. Moved the add-user form to the top of templates/admin/badge/users.tmpl and removed unused id attributes.
  7. Re-aligned and cleaned nested <div> structure in templates/admin/badge/view.tmpl for readability.
  8. Fixed control flow in DeleteBadgeUser: now returns immediately after flash + redirect on IsErrUserNotExist.
  9. Added dedicated AdminEditBadgeForm and updated route binding for badge edit, so edit validation no longer depends on slug input.

Validation run:

  • make fmt
  • make lint-go (0 issues)
@bircni bircni marked this pull request as ready for review February 25, 2026 18:28
@bircni bircni force-pushed the feature/retry-badges branch from 56aeab8 to 5cbe732 Compare February 26, 2026 18:30
@lunny
Copy link
Member

lunny commented Feb 26, 2026

It's better to have a user name search when inputing like other places.
image

@lunny
Copy link
Member

lunny commented Feb 26, 2026

image Align problem.
@bircni
Copy link
Contributor Author

bircni commented Feb 26, 2026

fixed

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 26, 2026
@lunny lunny added this to the 1.26.0 milestone Feb 26, 2026
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need more improvements

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 27, 2026
@bircni bircni requested a review from wxiaoguang February 27, 2026 20:16
func TestDeleteBadgeUserRedirectEscapesSlug(t *testing.T) {
unittest.PrepareTestEnv(t)
ctx, resp := contexttest.MockContext(t, "POST /-/admin/badges/slug/badge/users/delete")
ctx.SetPathParam("badge_slug", "badge/slug with space")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it can happen

This test seems not useful and unnecessary

Comment on lines +9 to +10
<label for="slug">{{ctx.Locale.Tr "admin.badges.slug"}}</label>
<input autofocus required id="slug" name="slug">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use for / id . Framework will add them

// ErrInvalidGroupTeamMap is returned when a group team mapping is invalid
ErrInvalidGroupTeamMap = "InvalidGroupTeamMap"
// ErrInvalidSlug is returned when a slug is invalid
ErrInvalidSlug = "InvalidSlug"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrInvalidBadgeSlug = "InvalidBadgeSlug"

func addSlugPatternRule() {
binding.AddRule(&binding.Rule{
IsMatch: func(rule string) bool {
return rule == "Slug"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BadgeSlug

Comment on lines +40 to +43
if sortType == "" {
sortType = BadgeSearchDefaultAdminSort
ctx.SetFormString("sort", sortType)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RenderBadgeSearch already does correct fall-back.

These if / SetFormString actually helps nothing

Comment on lines +22 to +27
<div class="menu">
<button class="item" name="sort" value="oldest">{{ctx.Locale.Tr "repo.issues.filter_sort.oldest"}}</button>
<button class="item" name="sort" value="newest">{{ctx.Locale.Tr "repo.issues.filter_sort.latest"}}</button>
<button class="item" name="sort" value="alphabetically">{{ctx.Locale.Tr "repo.issues.label.filter_sort.alphabetically"}}</button>
<button class="item" name="sort" value="reversealphabetically">{{ctx.Locale.Tr "repo.issues.label.filter_sort.reverse_alphabetically"}}</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a bug? No item will be selected even if end user chooses one. I haven't tested.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Feb 28, 2026
Comment on lines +31 to +32
validSlugPattern: regexp.MustCompile(`^[\da-zA-Z][-.\w]*$`),
invalidSlugPattern: regexp.MustCompile(`[-._]{2,}|[-._]$`),
Copy link
Contributor

@wxiaoguang wxiaoguang Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the proper patterns for "badge slug"?

Should it support names like "Awesome!" or "Emoji 💯" ? ( I think no, it only uses image)

If it is not designed to support these patterns, then the test ( https://github.com/go-gitea/gitea/pull/36752/files#r2867004215 ) seems not useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files

4 participants