Merge 0e6b925ee994d67d5b0eb081663778c6a1c0196f into c2e23d3301b1be2b2ad667184030087f92ad2470

See the FIXME comment in code. Otherwise, if a repo's issue unit is
disabled, then the PRs can't be edited anymore.

By the way, make the permission log output look slightly better.

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: metiftikci <metiftikci@hotmail.com>

models/perm/access/repo_permission.go

@@ -152,7 +152,7 @@ func (p *Permission) ReadableUnitTypes() []unit.Type {
 }
 
 func (p *Permission) LogString() string {
-	format := "<Permission AccessMode=%s, %d Units, %d UnitsMode(s): [ "
+	format := "<Permission AccessMode=%s, %d Units, %d UnitsMode(s): ["
 	args := []any{p.AccessMode.ToString(), len(p.units), len(p.unitsMode)}
 
 	for i, u := range p.units {
@@ -164,14 +164,16 @@ func (p *Permission) LogString() string {
 				config = err.Error()
 			}
 		}
-		format += "\nUnits[%d]: ID: %d RepoID: %d Type: %s Config: %s"
+		format += "\n\tunits[%d]: ID=%d RepoID=%d Type=%s Config=%s"
 		args = append(args, i, u.ID, u.RepoID, u.Type.LogString(), config)
 	}
 	for key, value := range p.unitsMode {
-		format += "\nUnitMode[%-v]: %-v"
+		format += "\n\tunitsMode[%-v]: %-v"
 		args = append(args, key.LogString(), value.LogString())
 	}
-	format += " ]>"
+	format += "\n\teveryoneAccessMode: %-v"
+	args = append(args, p.everyoneAccessMode)
+	format += "\n\t]>"
 	return fmt.Sprintf(format, args...)
 }
 

modules/log/logger_impl.go

@@ -5,6 +5,7 @@ package log
 
 import (
 	"context"
+	"reflect"
 	"runtime"
 	"strings"
 	"sync"
@@ -175,6 +176,20 @@ func (l *LoggerImpl) IsEnabled() bool {
 	return l.level.Load() < int32(FATAL) && len(l.eventWriters) > 0
 }
 
+func asLogStringer(v any) LogStringer {
+	if s, ok := v.(LogStringer); ok {
+		return s
+	} else if a := reflect.ValueOf(v); a.Kind() == reflect.Struct {
+		// in case the receiver is a pointer, but the value is a struct
+		vp := reflect.New(a.Type())
+		vp.Elem().Set(a)
+		if s, ok := vp.Interface().(LogStringer); ok {
+			return s
+		}
+	}
+	return nil
+}
+
 // Log prepares the log event, if the level matches, the event will be sent to the writers
 func (l *LoggerImpl) Log(skip int, level Level, format string, logArgs ...any) {
 	if Level(l.level.Load()) > level {
@@ -207,11 +222,11 @@ func (l *LoggerImpl) Log(skip int, level Level, format string, logArgs ...any) {
 	// handle LogStringer values
 	for i, v := range msgArgs {
 		if cv, ok := v.(*ColoredValue); ok {
-			if s, ok := cv.v.(LogStringer); ok {
-				cv.v = logStringFormatter{v: s}
+			if ls := asLogStringer(cv.v); ls != nil {
+				cv.v = logStringFormatter{v: ls}
 			}
-		} else if s, ok := v.(LogStringer); ok {
-			msgArgs[i] = logStringFormatter{v: s}
+		} else if ls := asLogStringer(v); ls != nil {
+			msgArgs[i] = logStringFormatter{v: ls}
 		}
 	}
 

modules/log/logger_test.go

@@ -116,6 +116,14 @@ func (t testLogString) LogString() string {
 	return "log-string"
 }
 
+type testLogStringPtrReceiver struct {
+	Field string
+}
+
+func (t *testLogStringPtrReceiver) LogString() string {
+	return "log-string-ptr-receiver"
+}
+
 func TestLoggerLogString(t *testing.T) {
 	logger := NewLoggerWithWriters(context.Background(), "test")
 
@@ -124,9 +132,13 @@ func TestLoggerLogString(t *testing.T) {
 	logger.AddWriters(w1)
 
 	logger.Info("%s %s %#v %v", testLogString{}, &testLogString{}, testLogString{Field: "detail"}, NewColoredValue(testLogString{}, FgRed))
+	logger.Info("%s %s %#v %v", testLogStringPtrReceiver{}, &testLogStringPtrReceiver{}, testLogStringPtrReceiver{Field: "detail"}, NewColoredValue(testLogStringPtrReceiver{}, FgRed))
 	logger.Close()
 
-	assert.Equal(t, []string{"log-string log-string log.testLogString{Field:\"detail\"} \x1b[31mlog-string\x1b[0m\n"}, w1.GetLogs())
+	assert.Equal(t, []string{
+		"log-string log-string log.testLogString{Field:\"detail\"} \x1b[31mlog-string\x1b[0m\n",
+		"log-string-ptr-receiver log-string-ptr-receiver &log.testLogStringPtrReceiver{Field:\"detail\"} \x1b[31mlog-string-ptr-receiver\x1b[0m\n",
+	}, w1.GetLogs())
 }
 
 func TestLoggerExpressionFilter(t *testing.T) {

options/locale/locale_en-US.ini

@@ -908,6 +908,7 @@ token_name = Token Name
 generate_token = Generate Token
 generate_token_success = Your new token has been generated. Copy it now as it will not be shown again.
 generate_token_name_duplicate = <strong>%s</strong> has been used as an application name already. Please use a new one.
+generate_token_name_required = Token Name is required.
 delete_token = Delete
 access_token_deletion = Delete Access Token
 access_token_deletion_cancel_action = Cancel

routers/web/web.go

@@ -1196,6 +1196,10 @@ func registerRoutes(m *web.Router) {
 			})
 		})
 	}
+	// FIXME: many "pulls" requests are sent to "issues" endpoints correctly, so the issue endpoints have to tolerate pull request permissions at the moment
+	m.Group("/{username}/{reponame}/{type:issues}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignment, context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests))
+	m.Group("/{username}/{reponame}/{type:pulls}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignment, reqUnitPullsReader)
+
 	m.Group("/{username}/{reponame}", func() {
 		m.Get("/comments/{id}/attachments", repo.GetCommentAttachments)
 		m.Get("/labels", repo.RetrieveLabelsForList, repo.Labels)
@@ -1203,9 +1207,6 @@ func registerRoutes(m *web.Router) {
 		m.Get("/milestone/{id}", context.RepoRef(), repo.MilestoneIssuesAndPulls)
 		m.Get("/issues/suggestions", repo.IssueSuggestions)
 	}, optSignIn, context.RepoAssignment, reqRepoIssuesOrPullsReader) // issue/pull attachments, labels, milestones
-
-	m.Group("/{username}/{reponame}/{type:issues}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignment, reqUnitIssuesReader)
-	m.Group("/{username}/{reponame}/{type:pulls}", addIssuesPullsViewRoutes, optSignIn, context.RepoAssignment, reqUnitPullsReader)
 	// end "/{username}/{reponame}": view milestone, label, issue, pull, etc
 
 	m.Group("/{username}/{reponame}/{type:issues}", func() {
@@ -1224,7 +1225,7 @@ func registerRoutes(m *web.Router) {
 			m.Get("/search", repo.SearchRepoIssuesJSON)
 		}, reqUnitIssuesReader)
 
-		addIssuesPullsRoutes := func() {
+		addIssuesPullsUpdateRoutes := func() {
 			// for "/{username}/{reponame}/issues" or "/{username}/{reponame}/pulls"
 			m.Group("/{index}", func() {
 				m.Post("/title", repo.UpdateIssueTitle)
@@ -1267,8 +1268,9 @@ func registerRoutes(m *web.Router) {
 			m.Delete("/unpin/{index}", reqRepoAdmin, repo.IssueUnpin)
 			m.Post("/move_pin", reqRepoAdmin, repo.IssuePinMove)
 		}
-		m.Group("/{type:issues}", addIssuesPullsRoutes, reqUnitIssuesReader, context.RepoMustNotBeArchived())
-		m.Group("/{type:pulls}", addIssuesPullsRoutes, reqUnitPullsReader, context.RepoMustNotBeArchived())
+		// FIXME: many "pulls" requests are sent to "issues" endpoints incorrectly, so the issue endpoints have to tolerate pull request permissions at the moment
+		m.Group("/{type:issues}", addIssuesPullsUpdateRoutes, context.RequireUnitReader(unit.TypeIssues, unit.TypePullRequests), context.RepoMustNotBeArchived())
+		m.Group("/{type:pulls}", addIssuesPullsUpdateRoutes, reqUnitPullsReader, context.RepoMustNotBeArchived())
 
 		m.Group("/comments/{id}", func() {
 			m.Post("", repo.UpdateCommentContent)
@@ -1292,7 +1294,7 @@ func registerRoutes(m *web.Router) {
 			m.Post("/delete", repo.DeleteMilestone)
 		}, reqRepoIssuesOrPullsWriter, context.RepoRef())
 
-		// FIXME: need to move these routes to the proper place
+		// FIXME: many "pulls" requests are sent to "issues" endpoints incorrectly, need to move these routes to the proper place
 		m.Group("/issues", func() {
 			m.Post("/request_review", repo.UpdatePullReviewRequest)
 			m.Post("/dismiss_review", reqRepoAdmin, web.Bind(forms.DismissReviewForm{}), repo.DismissReview)

templates/user/settings/applications.tmpl

@@ -55,10 +55,12 @@
 			</h5>
 			<form id="scoped-access-form" class="ui form ignore-dirty" action="{{.Link}}" method="post">
 				{{.CsrfTokenHtml}}
-				<div class="field {{if .Err_Name}}error{{end}}">
-					<label for="name">{{ctx.Locale.Tr "settings.token_name"}}</label>
-					<input id="name" name="name" value="{{.name}}" autofocus required maxlength="255">
-				</div>
+					<div class="field">
+						<label for="name">{{ctx.Locale.Tr "settings.token_name"}}</label>
+						<div id="scoped-access-token-name">
+							<input id="name" name="name" value="{{.name}}" autofocus required maxlength="255">
+						</div>
+					</div>
 				<div class="field">
 					<label>{{ctx.Locale.Tr "settings.repo_and_org_access"}}</label>
 					<label class="tw-cursor-pointer">
@@ -90,6 +92,9 @@
 					{{ctx.Locale.Tr "settings.generate_token"}}
 				</button>
 			</form>{{/* Fomantic ".ui.form .warning.message" is hidden by default, so put the warning message out of the form*/}}
+			<div id="token-name-warning" class="ui warning message center tw-hidden">
+				{{ctx.Locale.Tr "settings.generate_token_name_required"}}
+			</div>
 			<div id="scoped-access-warning" class="ui warning message center tw-hidden">
 				{{ctx.Locale.Tr "settings.at_least_one_permission"}}
 			</div>

web_src/js/components/ScopedAccessTokenForm.vue

@@ -38,20 +38,31 @@ onUnmounted(() => {
 function onClickSubmit(e: Event) {
   e.preventDefault();
 
+  const nameInput = document.querySelector<HTMLInputElement>('#name');
+  const nameWarning = document.querySelector('#token-name-warning');
+  if (!nameInput?.value.trim()) {
+    showElem(nameWarning);
+    return;
+  }
+  hideElem(nameWarning);
+
   const warningEl = document.querySelector('#scoped-access-warning');
-  // check that at least one scope has been selected
+  let hasSelectedScope = false;
+
   for (const el of document.querySelectorAll<HTMLInputElement>('.access-token-select')) {
     if (el.value) {
-      // Hide the error if it was visible from previous attempt.
-      hideElem(warningEl);
-      // Submit the form.
-      document.querySelector<HTMLFormElement>('#scoped-access-form').submit();
-      // Don't show the warning.
-      return;
+      hasSelectedScope = true;
+      break;
     }
   }
-  // no scopes selected, show validation error
-  showElem(warningEl);
+
+  if (!hasSelectedScope) {
+    showElem(warningEl);
+    return;
+  }
+
+  hideElem(warningEl);
+  document.querySelector<HTMLFormElement>('#scoped-access-form')?.submit();
 }
 </script>
 

web_src/js/features/scoped-access-token.ts

@@ -4,9 +4,9 @@ export async function initScopedAccessTokenCategories() {
   const el = document.querySelector('#scoped-access-token-selector');
   if (!el) return;
 
-  const {default: ScopedAccessTokenSelector} = await import(/* webpackChunkName: "scoped-access-token-selector" */'../components/ScopedAccessTokenSelector.vue');
+  const {default: ScopedAccessTokenForm} = await import(/* webpackChunkName: "scoped-access-token-form" */'../components/ScopedAccessTokenForm.vue');
   try {
-    const View = createApp(ScopedAccessTokenSelector, {
+    const View = createApp(ScopedAccessTokenForm, {
       isAdmin: JSON.parse(el.getAttribute('data-is-admin')),
       noAccessLabel: el.getAttribute('data-no-access-label'),
       readLabel: el.getAttribute('data-read-label'),
@@ -14,7 +14,7 @@ export async function initScopedAccessTokenCategories() {
     });
     View.mount(el);
   } catch (err) {
-    console.error('ScopedAccessTokenSelector failed to load', err);
+    console.error('ScopedAccessTokenForm failed to load', err);
     el.textContent = el.getAttribute('data-locale-component-failed-to-load');
   }
 }