fix: address verified review findings (labels-milestones-time-1)

Signed-off-by: bircni <bircni@icloud.com>

internal/services/errors.go

@@ -120,6 +120,8 @@ func (e *ConflictError) Unwrap() error { return e.Err }
 type AmbiguousError struct {
 	Kind, Key, Scope string
 	IDs              []int64
+	// Hint replaces the advice to pass an ID where IDs do not select the resource.
+	Hint string
 }
 
 func (e *AmbiguousError) Error() string {
@@ -131,7 +133,7 @@ func (e *AmbiguousError) Error() string {
 	if e.Scope != "" {
 		text += " in " + e.Scope
 	}
-	return text + " (IDs " + strings.Join(ids, ", ") + "); pass an ID"
+	return text + " (IDs " + strings.Join(ids, ", ") + "); " + cmp.Or(e.Hint, "pass an ID")
 }
 
 // refusals maps the messages Gitea gives for refused requests onto conflict reasons. Each entry

internal/services/label_selection.go

@@ -57,7 +57,7 @@ func (s Labels) CreateUnique(ctx context.Context, existing []Label, opts LabelOp
 		return Label{}, false, &ConflictError{Reason: ReasonExists, Kind: "label", Key: strconv.Quote(current.Name), Scope: s.Scope()}
 	}
 	opts.Name = nil
-	label, err := s.Edit(ctx, current.ID, opts)
+	label, err := s.Edit(ctx, current.ID, opts.KeepArchived(current))
 	return label, true, err
 }
 
@@ -70,7 +70,7 @@ func selectLabelNames(scope string, names []string, matches map[string][]int64)
 			return nil, NotFound("label", strconv.Quote(name), scope)
 		}
 		if len(candidates) > 1 {
-			return nil, &AmbiguousError{Kind: "label", Key: strconv.Quote(name), Scope: scope, IDs: candidates}
+			return nil, &AmbiguousError{Kind: "label", Key: strconv.Quote(name), Scope: scope, IDs: candidates, Hint: "prefix the name with repo: or org: to choose one"}
 		}
 		if !seen[candidates[0]] {
 			ids = append(ids, candidates[0])

internal/services/label_selection_test.go

@@ -66,7 +66,7 @@ func TestIssueLabelsCombineScopesWithoutSilentlyChoosingDuplicates(t *testing.T)
 	}))
 	service := Labels{Client: client, Repository: "company/project"}
 	_, err := service.ResolveIssueNames(context.Background(), []string{"bug"})
-	require.EqualError(t, err, `label "bug" is ambiguous in company/project (IDs 1, 2); pass an ID`)
+	require.EqualError(t, err, `label "bug" is ambiguous in company/project (IDs 1, 2); prefix the name with repo: or org: to choose one`)
 	ids, err := service.ResolveIssueNames(context.Background(), []string{"org:bug", "repo:bug", "shared", "shared"})
 	require.NoError(t, err)
 	require.Equal(t, []int64{2, 1, 3}, ids)

internal/services/metadata.go

@@ -35,6 +35,15 @@ type LabelOptions struct {
 	Archived    *bool   `json:"is_archived,omitempty"`
 }
 
+// KeepArchived returns opts with the archived state of current unless opts sets one: Gitea
+// unarchives a label whose edit leaves is_archived out.
+func (opts LabelOptions) KeepArchived(current Label) LabelOptions {
+	if opts.Archived == nil {
+		opts.Archived = &current.Archived
+	}
+	return opts
+}
+
 // Labels implements explicitly scoped label operations.
 type Labels struct {
 	Client                   *transport.Client
@@ -84,8 +93,14 @@ func (s Labels) Create(ctx context.Context, opts LabelOptions) (Label, error) {
 	if s.Organization != "" && opts.Archived != nil && *opts.Archived {
 		// TODO(sdk): CreateOrgLabelOption.IsArchived
 		var result Label
-		err := sendJSON(ctx, s.Client, http.MethodPost, s.orgEndpoint(), opts, &result)
-		return result, err
+		if err := sendJSON(ctx, s.Client, http.MethodPost, s.orgEndpoint(), opts, &result); err != nil {
+			return result, err
+		}
+		if !result.Archived {
+			// Gitea ignores is_archived when it creates an organization label.
+			return s.Edit(ctx, result.ID, LabelOptions{Archived: opts.Archived})
+		}
+		return result, nil
 	}
 	name, color, description := deref(opts.Name), deref(opts.Color), deref(opts.Description)
 	exclusive, archived := deref(opts.Exclusive), deref(opts.Archived)

internal/services/metadata_test.go

@@ -75,3 +75,22 @@ func TestMilestoneRejectsInvalidDeadlineBeforeRequests(t *testing.T) {
 	_, err = service.Edit(context.Background(), 1, MilestoneOptions{Deadline: &invalid})
 	require.ErrorContains(t, err, "invalid milestone deadline")
 }
+
+func TestOrganizationArchivedLabelCreateFollowsUpWithPatch(t *testing.T) {
+	var requests []string
+	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		requests = append(requests, r.Method+" "+r.URL.Path+" "+string(body))
+		// Gitea ignores is_archived on create and honors it on edit.
+		archived := "false"
+		if r.Method == http.MethodPatch {
+			archived = "true"
+		}
+		_, _ = w.Write([]byte(`{"id":5,"name":"bug","is_archived":` + archived + `}`))
+	}))
+	name, archived := "bug", true
+	label, err := (Labels{Client: client, Organization: "company"}).Create(context.Background(), LabelOptions{Name: &name, Archived: &archived})
+	require.NoError(t, err)
+	require.True(t, label.Archived)
+	require.Equal(t, []string{`POST /api/v1/orgs/company/labels {"name":"bug","is_archived":true}`, `PATCH /api/v1/orgs/company/labels/5 {"is_archived":true}`}, requests)
+}

internal/services/milestone_selection.go

@@ -11,8 +11,11 @@ import (
 )
 
 // All returns the open and closed milestones of the repository.
-func (s Milestones) All(ctx context.Context) ([]Milestone, error) {
-	return listAll(func(page int) ([]Milestone, error) { return s.List(ctx, "all", 100, page) })
+func (s Milestones) All(ctx context.Context) ([]Milestone, error) { return s.AllIn(ctx, "all") }
+
+// AllIn returns every milestone in state: open, closed or all.
+func (s Milestones) AllIn(ctx context.Context, state string) ([]Milestone, error) {
+	return listAll(func(page int) ([]Milestone, error) { return s.List(ctx, state, 100, page) })
 }
 
 // Find returns the milestones that raws address among the open and closed milestones, each by ID
@@ -51,10 +54,6 @@ func (s Milestones) FilterIDs(ctx context.Context, values []string) ([]string, e
 	if err != nil {
 		return nil, err
 	}
-	titles := make(map[string]int, len(items))
-	for _, item := range items {
-		titles[item.Title]++
-	}
 	finder := NameOrID[Milestone]{Kind: "milestone", Scope: s.Repository, ID: func(m Milestone) int64 { return m.ID }, Name: func(m Milestone) string { return m.Title }}
 	result := make([]string, len(values))
 	for i, raw := range values {
@@ -62,15 +61,33 @@ func (s Milestones) FilterIDs(ctx context.Context, values []string) ([]string, e
 		if err != nil {
 			return nil, err
 		}
-		id := strconv.FormatInt(item.ID, 10)
-		switch {
-		case titles[item.Title] == 1 && !strings.Contains(item.Title, ","):
-			result[i] = item.Title
-		case titles[id] == 0:
-			result[i] = id
-		default:
-			return nil, fmt.Errorf("milestone %q (ID %s) cannot filter issues: another milestone of %s is titled %q", item.Title, id, s.Repository, id)
+		if result[i], err = s.filterValue(items, item); err != nil {
+			return nil, err
 		}
 	}
 	return result, nil
 }
+
+// FilterValue is the value that filters an issue listing by item, as FilterIDs resolves it.
+func (s Milestones) FilterValue(ctx context.Context, item Milestone) (string, error) {
+	items, err := s.All(ctx)
+	if err != nil {
+		return "", err
+	}
+	return s.filterValue(items, item)
+}
+
+func (s Milestones) filterValue(items []Milestone, item Milestone) (string, error) {
+	titles := make(map[string]int, len(items))
+	for _, other := range items {
+		titles[other.Title]++
+	}
+	id := strconv.FormatInt(item.ID, 10)
+	switch {
+	case titles[item.Title] == 1 && !strings.Contains(item.Title, ","):
+		return item.Title, nil
+	case titles[id] == 0:
+		return id, nil
+	}
+	return "", fmt.Errorf("milestone %q (ID %s) cannot filter issues: another milestone of %s is titled %q", item.Title, id, s.Repository, id)
+}