From a1d3a85ab8287f8276fab99caaf1666c764809eb Mon Sep 17 00:00:00 2001 From: wangyue789 Date: Fri, 5 Jun 2026 14:56:02 +0800 Subject: [PATCH] fix(health): use effective list filters --- shortcuts/health/api.go | 53 +++++++++++------ shortcuts/health/api_test.go | 108 ++++++++++++++++++++++++++++++++++ shortcuts/health/db.go | 111 +++++++++++++++++++++++++++++++---- shortcuts/health/db_test.go | 62 +++++++++++++++++++ 4 files changed, 304 insertions(+), 30 deletions(-) create mode 100644 shortcuts/health/api_test.go create mode 100644 shortcuts/health/db_test.go diff --git a/shortcuts/health/api.go b/shortcuts/health/api.go index 2d9f81b..5d55372 100644 --- a/shortcuts/health/api.go +++ b/shortcuts/health/api.go @@ -9,36 +9,57 @@ import ( ) // v1RepoPath constructs the v1 API path for a repository. -// Issue operations use the v1 API (/v1/{owner}/{repo}) to match shortcuts/issue conventions, -// while PR operations use the v2 API via ctx.RepoPath() (/{owner}/{repo}/pulls) to match shortcuts/pr conventions. - func v1RepoPath(owner, repo string) string { return fmt.Sprintf("/v1/%s/%s", owner, repo) } +func normalizePRListStatus(state string) string { + switch state { + case "open", "opened": + return "0" + case "merged": + return "1" + case "closed": + return "2" + default: + return "" + } +} + +func normalizeIssueListCategory(state string) string { + switch state { + case "open", "opened": + return "opened" + case "closed": + return "closed" + default: + return "all" + } +} + func fetchPRListPage(ctx *common.RuntimeContext, state string, page, limit int) ([]interface{}, error) { q := url.Values{} q.Set("page", fmt.Sprintf("%d", page)) q.Set("limit", fmt.Sprintf("%d", limit)) - if state != "" { - q.Set("state", state) + if status := normalizePRListStatus(state); status != "" { + q.Set("status", status) } - env, err := ctx.CallAPIWithQuery("GET", ctx.RepoPath()+"/pulls", q) + env, err := ctx.CallAPIWithQuery("GET", v1RepoPath(ctx.Owner, ctx.Repo)+"/pulls", q) if err != nil { - fmt.Fprintf(os.Stderr, " CLI error: pr +list state=%s page=%d: %v\n", state, page, err) + fmt.Fprintf(os.Stderr, " CLI error: pr list state=%s page=%d: %v\n", state, page, err) return nil, err } if !env.OK { - err := fmt.Errorf("API error: pr +list state=%s page=%d", state, page) + err := fmt.Errorf("API error: pr list state=%s page=%d", state, page) fmt.Fprintf(os.Stderr, " %v\n", err) return nil, err } data, ok := env.Data.(map[string]interface{}) if !ok { - return nil, fmt.Errorf("unexpected response type for pr +list") + return nil, fmt.Errorf("unexpected response type for pr list") } - issues, _ := data["issues"].([]interface{}) - return issues, nil + pulls, _ := data["pulls"].([]interface{}) + return pulls, nil } // fetchPRDetail retrieves full PR detail to extract merged_at timestamp. @@ -86,22 +107,20 @@ func fetchIssueListPage(ctx *common.RuntimeContext, owner, repo, state string, p q := url.Values{} q.Set("page", fmt.Sprintf("%d", page)) q.Set("limit", fmt.Sprintf("%d", limit)) - if state != "" { - q.Set("state", state) - } + q.Set("category", normalizeIssueListCategory(state)) env, err := ctx.CallAPIWithQuery("GET", v1RepoPath(owner, repo)+"/issues", q) if err != nil { - fmt.Fprintf(os.Stderr, " CLI error: issue +list state=%s page=%d: %v\n", state, page, err) + fmt.Fprintf(os.Stderr, " CLI error: issue list state=%s page=%d: %v\n", state, page, err) return nil, err } if !env.OK { - err := fmt.Errorf("API error: issue +list state=%s page=%d", state, page) + err := fmt.Errorf("API error: issue list state=%s page=%d", state, page) fmt.Fprintf(os.Stderr, " %v\n", err) return nil, err } data, ok := env.Data.(map[string]interface{}) if !ok { - return nil, fmt.Errorf("unexpected response type for issue +list") + return nil, fmt.Errorf("unexpected response type for issue list") } issues, _ := data["issues"].([]interface{}) return issues, nil diff --git a/shortcuts/health/api_test.go b/shortcuts/health/api_test.go new file mode 100644 index 0000000..6cc6ab8 --- /dev/null +++ b/shortcuts/health/api_test.go @@ -0,0 +1,108 @@ +package health + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gitlink-org/gitlink-cli/internal/client" + "github.com/gitlink-org/gitlink-cli/shortcuts/common" +) + +func TestFetchPRListPageUsesV1StatusFilter(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Fatalf("method=%s, want GET", r.Method) + } + if r.URL.Path != "/v1/owner/repo/pulls.json" { + t.Fatalf("path=%s, want /v1/owner/repo/pulls.json", r.URL.Path) + } + if got := r.URL.Query().Get("status"); got != "1" { + t.Fatalf("status=%q, want 1", got) + } + if got := r.URL.Query().Get("state"); got != "" { + t.Fatalf("state should not be sent, got %q", got) + } + writeHealthJSON(t, w, map[string]interface{}{ + "total_count": 1, + "pulls": []map[string]interface{}{ + {"id": 15414, "index": 109, "status": "merged"}, + }, + }) + })) + defer server.Close() + + pulls, err := fetchPRListPage(healthTestContext(server), "merged", 2, 5) + if err != nil { + t.Fatalf("fetchPRListPage failed: %v", err) + } + if len(pulls) != 1 { + t.Fatalf("len(pulls)=%d, want 1", len(pulls)) + } +} + +func TestFetchIssueListPageUsesCategoryFilter(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Fatalf("method=%s, want GET", r.Method) + } + if r.URL.Path != "/v1/owner/repo/issues.json" { + t.Fatalf("path=%s, want /v1/owner/repo/issues.json", r.URL.Path) + } + if got := r.URL.Query().Get("category"); got != "closed" { + t.Fatalf("category=%q, want closed", got) + } + if got := r.URL.Query().Get("state"); got != "" { + t.Fatalf("state should not be sent, got %q", got) + } + writeHealthJSON(t, w, map[string]interface{}{ + "total_count": 1, + "issues": []map[string]interface{}{ + {"id": 140801, "project_issues_index": 1}, + }, + }) + })) + defer server.Close() + + issues, err := fetchIssueListPage(healthTestContext(server), "owner", "repo", "closed", 1, 20) + if err != nil { + t.Fatalf("fetchIssueListPage failed: %v", err) + } + if len(issues) != 1 { + t.Fatalf("len(issues)=%d, want 1", len(issues)) + } +} + +func TestNormalizeListFilters(t *testing.T) { + prCases := map[string]string{"open": "0", "opened": "0", "merged": "1", "closed": "2", "all": ""} + for input, want := range prCases { + if got := normalizePRListStatus(input); got != want { + t.Fatalf("normalizePRListStatus(%q)=%q, want %q", input, got, want) + } + } + issueCases := map[string]string{"open": "opened", "opened": "opened", "closed": "closed", "all": "all"} + for input, want := range issueCases { + if got := normalizeIssueListCategory(input); got != want { + t.Fatalf("normalizeIssueListCategory(%q)=%q, want %q", input, got, want) + } + } +} + +func healthTestContext(server *httptest.Server) *common.RuntimeContext { + return &common.RuntimeContext{ + Client: &client.Client{HTTP: server.Client(), BaseURL: server.URL}, + Owner: "owner", + Repo: "repo", + Format: "json", + Args: map[string]string{}, + } +} + +func writeHealthJSON(t *testing.T, w http.ResponseWriter, payload interface{}) { + t.Helper() + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(payload); err != nil { + t.Fatalf("write json: %v", err) + } +} diff --git a/shortcuts/health/db.go b/shortcuts/health/db.go index 78ea7e8..0a1926b 100644 --- a/shortcuts/health/db.go +++ b/shortcuts/health/db.go @@ -139,6 +139,97 @@ func extractTagNames(data map[string]interface{}, key string) []string { return names } +func nestedMap(data map[string]interface{}, key string) map[string]interface{} { + obj, _ := data[key].(map[string]interface{}) + return obj +} + +func extractPullNumber(pr map[string]interface{}) int { + for _, key := range []string{"pull_request_number", "index"} { + if v, ok := pr[key].(float64); ok && v > 0 { + return int(v) + } + if v, ok := pr[key].(int); ok && v > 0 { + return v + } + } + return 0 +} + +func extractPullAuthorLogin(pr map[string]interface{}) string { + if login, _ := pr["author_login"].(string); login != "" { + return login + } + if login := extractLogin(pr, "author", ""); login != "" { + return login + } + if issue := nestedMap(pr, "issue"); issue != nil { + return extractLogin(issue, "author", "") + } + return "" +} + +func extractPullAssigneeLogin(pr map[string]interface{}) string { + if login, _ := pr["assign_user_login"].(string); login != "" { + return login + } + if issue := nestedMap(pr, "issue"); issue != nil { + if login := extractLogin(issue, "assign_user", "assign_user_login"); login != "" { + return login + } + } + return "" +} + +func extractPullStatus(pr map[string]interface{}) string { + statusMap := map[int]string{0: "open", 1: "merged", 2: "closed"} + if v, ok := pr["pull_request_status"].(float64); ok { + return statusMap[int(v)] + } + if v, ok := pr["pull_request_status"].(int); ok { + return statusMap[v] + } + for _, key := range []string{"pull_request_staus", "status", "state"} { + if status, _ := pr[key].(string); status != "" { + switch status { + case "open", "opened": + return "open" + case "merged": + return "merged" + case "closed", "close": + return "closed" + } + } + } + return "open" +} + +func extractPullCreateTime(pr map[string]interface{}) string { + for _, key := range []string{"pr_full_time", "created_at", "create_time"} { + if v, _ := pr[key].(string); v != "" { + return v + } + } + if issue := nestedMap(pr, "issue"); issue != nil { + for _, key := range []string{"created_at", "create_time"} { + if v, _ := issue[key].(string); v != "" { + return v + } + } + } + return "" +} + +func extractPullTagNames(pr map[string]interface{}) []string { + if names := extractTagNames(pr, "issue_tags"); len(names) > 0 { + return names + } + if issue := nestedMap(pr, "issue"); issue != nil { + return extractTagNames(issue, "issue_tags") + } + return nil +} + // mergeTimeFromList tries to extract merged_at directly from the list API response // (preferred, avoids an extra API call per PR). func mergeTimeFromList(pr map[string]interface{}) string { @@ -165,28 +256,22 @@ func savePull(db *sql.DB, repoID int, pr map[string]interface{}, mergedAt string return } - prNumber, _ := pr["pull_request_number"].(float64) + prNumber := extractPullNumber(pr) if prNumber == 0 { return } - author, _ := pr["author_login"].(string) - createrID, _ := getOrCreateUser(db, author) + createrID, _ := getOrCreateUser(db, extractPullAuthorLogin(pr)) - statusCode := 0 - if v, ok := pr["pull_request_status"].(float64); ok { - statusCode = int(v) - } - statusMap := map[int]string{0: "open", 1: "merged", 2: "closed"} - status := statusMap[statusCode] + status := extractPullStatus(pr) if status == "" { status = "open" } - createTime, _ := pr["pr_full_time"].(string) + createTime := extractPullCreateTime(pr) var processorID *int - if assignee, _ := pr["assign_user_login"].(string); assignee != "" { + if assignee := extractPullAssigneeLogin(pr); assignee != "" { pid, _ := getOrCreateUser(db, assignee) processorID = &pid } @@ -201,12 +286,12 @@ func savePull(db *sql.DB, repoID int, pr map[string]interface{}, mergedAt string if _, err := db.Exec(`INSERT OR REPLACE INTO pulls (id, repo_id, number, creater_id, status, processor_id, create_time, merged_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, - int(prID), repoID, int(prNumber), createrID, status, processorID, createTime, mergedAtVal); err != nil { + int(prID), repoID, prNumber, createrID, status, processorID, createTime, mergedAtVal); err != nil { fmt.Fprintf(os.Stderr, " DB error: save pull %d: %v\n", int(prID), err) } // Save tags - tagNames := extractTagNames(pr, "issue_tags") + tagNames := extractPullTagNames(pr) var tagIDs []int for _, name := range tagNames { if tid, err := getOrCreateTag(db, repoID, name); err == nil { diff --git a/shortcuts/health/db_test.go b/shortcuts/health/db_test.go new file mode 100644 index 0000000..8cec91c --- /dev/null +++ b/shortcuts/health/db_test.go @@ -0,0 +1,62 @@ +package health + +import ( + "path/filepath" + "testing" +) + +func TestSavePullAcceptsV1ListShape(t *testing.T) { + db, err := openDB(filepath.Join(t.TempDir(), "health.db")) + if err != nil { + t.Fatalf("openDB: %v", err) + } + defer db.Close() + + repoID, err := getOrCreateRepo(db, "repo", "owner") + if err != nil { + t.Fatalf("getOrCreateRepo: %v", err) + } + + savePull(db, repoID, map[string]interface{}{ + "id": float64(15414), + "index": float64(109), + "status": "merged", + "issue": map[string]interface{}{ + "author": map[string]interface{}{"login": "alice"}, + "issue_tags": []interface{}{ + map[string]interface{}{"name": "docs"}, + }, + }, + }, "2026-06-05T12:00:00+08:00") + + var number int + var status string + var author string + var mergedAt string + if err := db.QueryRow(` + SELECT pulls.number, pulls.status, users.user_name, pulls.merged_at + FROM pulls JOIN users ON users.id = pulls.creater_id + WHERE pulls.id = ?`, 15414).Scan(&number, &status, &author, &mergedAt); err != nil { + t.Fatalf("query saved pull: %v", err) + } + if number != 109 { + t.Fatalf("number=%d, want 109", number) + } + if status != "merged" { + t.Fatalf("status=%q, want merged", status) + } + if author != "alice" { + t.Fatalf("author=%q, want alice", author) + } + if mergedAt == "" { + t.Fatal("merged_at was not saved") + } + + var tagCount int + if err := db.QueryRow(`SELECT COUNT(*) FROM pull_tags WHERE pull_id = ?`, 15414).Scan(&tagCount); err != nil { + t.Fatalf("query tags: %v", err) + } + if tagCount != 1 { + t.Fatalf("tagCount=%d, want 1", tagCount) + } +}