fix: address review findings (pagination, case-insensitive bool, docs)
- ListReviews: full pagination (loops until partial page returned) - envOrDefaultBool: case-insensitive, whitespace-trimmed - action.yml: document accepted boolean values - Tests: verify pagination across multiple pages
This commit is contained in:
@@ -67,7 +67,7 @@ inputs:
|
|||||||
required: false
|
required: false
|
||||||
default: 'false'
|
default: 'false'
|
||||||
update-existing:
|
update-existing:
|
||||||
description: 'Delete previous review from same bot before posting (default true)'
|
description: 'Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no (default true)'
|
||||||
required: false
|
required: false
|
||||||
default: 'true'
|
default: 'true'
|
||||||
|
|
||||||
|
|||||||
@@ -359,7 +359,7 @@ func envOrDefaultInt(key string, defaultVal int) int {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func envOrDefaultBool(key string, defaultVal bool) bool {
|
func envOrDefaultBool(key string, defaultVal bool) bool {
|
||||||
v := os.Getenv(key)
|
v := strings.TrimSpace(strings.ToLower(os.Getenv(key)))
|
||||||
if v == "" {
|
if v == "" {
|
||||||
return defaultVal
|
return defaultVal
|
||||||
}
|
}
|
||||||
|
|||||||
+18
-7
@@ -297,21 +297,32 @@ func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// ListReviews returns all reviews on a pull request.
|
// ListReviews returns all reviews on a pull request.
|
||||||
|
// Paginates through all pages to ensure no reviews are missed.
|
||||||
func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) {
|
func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) {
|
||||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews",
|
const pageSize = 50
|
||||||
|
var all []Review
|
||||||
|
for page := 1; ; page++ {
|
||||||
|
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews?limit=%d&page=%d",
|
||||||
c.baseURL,
|
c.baseURL,
|
||||||
url.PathEscape(owner),
|
url.PathEscape(owner),
|
||||||
url.PathEscape(repo),
|
url.PathEscape(repo),
|
||||||
number)
|
number,
|
||||||
|
pageSize,
|
||||||
|
page)
|
||||||
body, err := c.doGet(ctx, reqURL)
|
body, err := c.doGet(ctx, reqURL)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("list reviews: %w", err)
|
return nil, fmt.Errorf("list reviews (page %d): %w", page, err)
|
||||||
}
|
}
|
||||||
var reviews []Review
|
var batch []Review
|
||||||
if err := json.Unmarshal(body, &reviews); err != nil {
|
if err := json.Unmarshal(body, &batch); err != nil {
|
||||||
return nil, fmt.Errorf("parse reviews: %w", err)
|
return nil, fmt.Errorf("parse reviews (page %d): %w", page, err)
|
||||||
}
|
}
|
||||||
return reviews, nil
|
all = append(all, batch...)
|
||||||
|
if len(batch) < pageSize {
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return all, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// DeleteReview deletes a review by ID. The token must belong to the review author.
|
// DeleteReview deletes a review by ID. The token must belong to the review author.
|
||||||
|
|||||||
+45
-2
@@ -357,11 +357,17 @@ func TestGetAuthenticatedUser_EmptyLogin(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestListReviews(t *testing.T) {
|
func TestListReviews(t *testing.T) {
|
||||||
|
pageCount := 0
|
||||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews" {
|
if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews" {
|
||||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||||
}
|
}
|
||||||
|
if r.URL.Query().Get("limit") != "50" {
|
||||||
|
t.Errorf("expected limit=50, got %s", r.URL.Query().Get("limit"))
|
||||||
|
}
|
||||||
|
pageCount++
|
||||||
w.Header().Set("Content-Type", "application/json")
|
w.Header().Set("Content-Type", "application/json")
|
||||||
|
// Return 2 results (less than page size) to signal end
|
||||||
w.Write([]byte(`[{"id":10,"user":{"login":"bot-a"},"state":"APPROVED","stale":false},{"id":11,"user":{"login":"bot-b"},"state":"REQUEST_CHANGES","stale":true}]`))
|
w.Write([]byte(`[{"id":10,"user":{"login":"bot-a"},"state":"APPROVED","stale":false},{"id":11,"user":{"login":"bot-b"},"state":"REQUEST_CHANGES","stale":true}]`))
|
||||||
}))
|
}))
|
||||||
defer server.Close()
|
defer server.Close()
|
||||||
@@ -377,8 +383,45 @@ func TestListReviews(t *testing.T) {
|
|||||||
if reviews[0].User.Login != "bot-a" {
|
if reviews[0].User.Login != "bot-a" {
|
||||||
t.Errorf("expected bot-a, got %s", reviews[0].User.Login)
|
t.Errorf("expected bot-a, got %s", reviews[0].User.Login)
|
||||||
}
|
}
|
||||||
if reviews[1].ID != 11 {
|
if pageCount != 1 {
|
||||||
t.Errorf("expected id 11, got %d", reviews[1].ID)
|
t.Errorf("expected 1 page fetch (results < page size), got %d", pageCount)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestListReviews_Pagination(t *testing.T) {
|
||||||
|
pageCount := 0
|
||||||
|
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
pageCount++
|
||||||
|
page := r.URL.Query().Get("page")
|
||||||
|
w.Header().Set("Content-Type", "application/json")
|
||||||
|
if page == "1" {
|
||||||
|
// Return exactly 50 items to trigger next page fetch
|
||||||
|
items := "["
|
||||||
|
for i := 0; i < 50; i++ {
|
||||||
|
if i > 0 {
|
||||||
|
items += ","
|
||||||
|
}
|
||||||
|
items += fmt.Sprintf(`{"id":%d,"user":{"login":"bot"},"state":"APPROVED","stale":false}`, i+1)
|
||||||
|
}
|
||||||
|
items += "]"
|
||||||
|
w.Write([]byte(items))
|
||||||
|
} else {
|
||||||
|
// Page 2: return fewer than 50 to signal end
|
||||||
|
w.Write([]byte(`[{"id":51,"user":{"login":"bot"},"state":"APPROVED","stale":false}]`))
|
||||||
|
}
|
||||||
|
}))
|
||||||
|
defer server.Close()
|
||||||
|
|
||||||
|
client := NewClient(server.URL, "test-token")
|
||||||
|
reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(reviews) != 51 {
|
||||||
|
t.Fatalf("expected 51 reviews across 2 pages, got %d", len(reviews))
|
||||||
|
}
|
||||||
|
if pageCount != 2 {
|
||||||
|
t.Errorf("expected 2 page fetches, got %d", pageCount)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user