feat: Add remaining Projects v2 endpoints#4319
Conversation
2d1843a to
10b097f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4319 +/- ##
==========================================
+ Coverage 97.48% 97.50% +0.01%
==========================================
Files 193 193
Lines 19400 19542 +142
==========================================
+ Hits 18912 19054 +142
Misses 270 270
Partials 218 218 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @JamBalaya56562!
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @JamBalaya56562!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
stevehipwell
left a comment
There was a problem hiding this comment.
I think we need to fix the struct types to not use pointers for required fields. I've commented on the first one.
| // CreateProjectV2DraftItemRequest specifies the parameters to create a draft item in a project. | ||
| type CreateProjectV2DraftItemRequest struct { | ||
| // Title is the title of the draft issue item to create. (Required.) | ||
| Title *string `json:"title,omitempty"` |
There was a problem hiding this comment.
| Title *string `json:"title,omitempty"` | |
| Title string `json:"title"` |
As title is required it shouldn't be a pointer.
There was a problem hiding this comment.
Done — thanks! Made the required request fields non-pointer values (and dropped omitempty): CreateProjectV2DraftItemRequest.Title, CreateProjectV2ViewRequest.Name/Layout (all in the OpenAPI required arrays), plus ProjectV2FieldSingleSelectOption.Name (an option's required display label). I kept AddProjectV2FieldRequest all-pointer since its body is a oneOf where fields are only conditionally required, and left the optional/response fields as-is. e5df91b
stevehipwell
left a comment
There was a problem hiding this comment.
There still appear to be fields which are required in the responses that are being defined as pointers (e.g. many of the view fields).
There was a problem hiding this comment.
Please use cmp.Diff, cmp.Equal, testJSONMarshal to simplify tests.
There was a problem hiding this comment.
Good call — switched the new draft/field/view/view-item tests to cmp.Diff(want, got) against a fully-populated want (which now also checks every decoded field) in 365ebfb.
|
@stevehipwell thanks. I've kept the response types pointer-based on purpose, to stay consistent with go-github's convention: response structs use |
PR google#4315 (google#4301) and google#4318 (google#2113) were merged; PR google#4316 (google#3220) was closed unmerged after maintainers decided the rate-limit sleep callback belongs in the external gofri/go-github-ratelimit middleware. google#4319 (google#3715, projects v2) remains open and under review.
|
@JamBalaya56562 I think the current required pattern is to only use pointer types for optional fields. See #4202 (closed) where the consensus from the maintainer/reviewers was that required response body fields should be non-pointer types. |
|
|
||
| // CreateProjectV2DraftItemRequest specifies the parameters to create a draft item in a project. | ||
| type CreateProjectV2DraftItemRequest struct { | ||
| // Title is the title of the draft issue item to create. (Required.) |
There was a problem hiding this comment.
| // Title is the title of the draft issue item to create. (Required.) | |
| // Title is the title of the draft issue item to create. |
| type CreateProjectV2DraftItemRequest struct { | ||
| // Title is the title of the draft issue item to create. (Required.) | ||
| Title string `json:"title"` | ||
| // Body is the body content of the draft issue item to create. (Optional.) |
There was a problem hiding this comment.
| // Body is the body content of the draft issue item to create. (Optional.) | |
| // Body is the body content of the draft issue item to create. |
|
|
||
| // ProjectV2FieldSingleSelectOption represents an option to create for a single_select project field. | ||
| type ProjectV2FieldSingleSelectOption struct { | ||
| // Name is the display name of the option. (Required.) |
There was a problem hiding this comment.
| // Name is the display name of the option. (Required.) | |
| // Name is the display name of the option. |
| return err | ||
| } | ||
| if len(tuple) != 2 { | ||
| return fmt.Errorf("ProjectV2ViewSortBy: expected a [field_id, direction] tuple, got %v elements", len(tuple)) |
There was a problem hiding this comment.
| return fmt.Errorf("ProjectV2ViewSortBy: expected a [field_id, direction] tuple, got %v elements", len(tuple)) | |
| return fmt.Errorf("expected a [field_id, direction] tuple, got %v elements", len(tuple)) |
| // The OpenAPI schema allows the field_id to be a string as well. | ||
| var str string | ||
| if err2 := json.Unmarshal(tuple[0], &str); err2 != nil { | ||
| return fmt.Errorf("ProjectV2ViewSortBy: invalid field_id: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("ProjectV2ViewSortBy: invalid field_id: %w", err) | |
| return fmt.Errorf("invalid field_id: %w", err2) |
There was a problem hiding this comment.
Done in bcc07a6 — also switched to wrap err2 (the actual string-parse error) here, thanks for catching that.
| } | ||
| fieldID, err = strconv.ParseInt(str, 10, 64) | ||
| if err != nil { | ||
| return fmt.Errorf("ProjectV2ViewSortBy: invalid field_id %q: %w", str, err) |
There was a problem hiding this comment.
| return fmt.Errorf("ProjectV2ViewSortBy: invalid field_id %q: %w", str, err) | |
| return fmt.Errorf("invalid field_id %q: %w", str, err) |
|
|
||
| var direction string | ||
| if err := json.Unmarshal(tuple[1], &direction); err != nil { | ||
| return fmt.Errorf("ProjectV2ViewSortBy: invalid direction: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("ProjectV2ViewSortBy: invalid direction: %w", err) | |
| return fmt.Errorf("invalid direction: %w", err) |
| type CreateProjectV2ViewRequest struct { | ||
| // Name is the view's display name. (Required.) | ||
| Name string `json:"name"` | ||
| // Layout is the view's layout. One of: table, board, roadmap. (Required.) |
There was a problem hiding this comment.
| // Layout is the view's layout. One of: table, board, roadmap. (Required.) | |
| // Layout is the view's layout. One of: table, board, roadmap. |
|
|
||
| // CreateProjectV2ViewRequest specifies the parameters to create a project view. | ||
| type CreateProjectV2ViewRequest struct { | ||
| // Name is the view's display name. (Required.) |
There was a problem hiding this comment.
| // Name is the view's display name. (Required.) | |
| // Name is the view's display name. |
|
Thanks @stevehipwell, and apologies for pushing back earlier — you're right. I checked #4202 and the consensus there is indeed that required response fields should be non-pointer (the merged CONTRIBUTING.md hasn't caught up yet, which is what threw me off). I've made |
| // object, so it has custom JSON (un)marshaling. | ||
| type ProjectV2ViewSortBy struct { | ||
| // FieldID is the ID of the field to sort by. | ||
| FieldID *int64 |
There was a problem hiding this comment.
Maybe it makes sense:
| FieldID *int64 | |
| FieldID *int64 `json:"-"` |
There was a problem hiding this comment.
Done — added json:"-" to FieldID and Direction since they're handled by the custom Marshal/UnmarshalJSON. 362c31a
| ID int64 `json:"id"` | ||
| Number int `json:"number"` | ||
| Name string `json:"name"` | ||
| Layout string `json:"layout"` | ||
| NodeID string `json:"node_id"` | ||
| ProjectURL string `json:"project_url"` | ||
| HTMLURL string `json:"html_url"` | ||
| Creator *User `json:"creator,omitempty"` | ||
| Filter *string `json:"filter,omitempty"` |
There was a problem hiding this comment.
Why are these fields without comments and the rest with?
There was a problem hiding this comment.
Good point — I removed the per-field comments to make ProjectV2View consistent (matching the sibling ProjectV2, which has none). Happy to add a comment to every field instead if you'd prefer. 362c31a
| } | ||
|
|
||
| // ProjectV2View represents a view in a project. | ||
| type ProjectV2View struct { |
There was a problem hiding this comment.
"required": [
"id",
"number",
"name",
"layout",
"node_id",
"project_url",
"html_url",
"creator",
"created_at",
"updated_at",
"visible_fields",
"sort_by",
"group_by",
"vertical_group_by"
]These fields are required we do not use omitempty and pointer with them.
Still some required field use omitempty like group_by, vertical_group_by etc. (As said in comment)
There was a problem hiding this comment.
Done — in ProjectV2View the remaining required fields (creator, created_at, updated_at, visible_fields, sort_by, group_by, vertical_group_by) are now non-pointer values without omitempty, matching the convention that only optional fields are pointers. The only field left as a pointer is filter, which is optional. Accessors were regenerated accordingly. 10bea9e
(The branch was also rebased onto the latest master to clear the merge conflict, hence the force-push.)
Add the eight Projects v2 REST endpoints that were present in the OpenAPI
description but not yet implemented, completing the ProjectsService surface:
- Create draft item (organization and user owned projects)
- Add field (organization and user owned projects)
- Create view (organization and user owned projects)
- List items for a project view (organization and user owned projects)
Drafts and view items reuse ProjectV2Item, fields reuse ProjectV2Field, and
a new ProjectV2View type plus request types for the create/add operations
are added. Request bodies are passed by value with a Request suffix per the
paramcheck convention.
Note the user-owned draft and view endpoints identify the user by the numeric
user_id in the path (/user/{user_id}/... and /users/{user_id}/...), mirroring
the upstream REST API, unlike the username-based paths used by the other user
endpoints.
Fixes google#3715
Add subtests for the non-array, unsupported and non-numeric string field_id, and non-string direction branches of ProjectV2ViewSortBy.UnmarshalJSON, bringing the method (and MarshalJSON) to 100% coverage.
Per review, required request-body fields should not be pointers. The OpenAPI required arrays mark title (draft create) and name/layout (view create) as required, and a single-select option's name is its required display label, so make those non-pointer values without omitempty. AddProjectV2FieldRequest stays all-pointer because its fields are required only conditionally (the body is a oneOf), as do the optional fields and the response types.
Replace the hand-written field-by-field assertions in the new draft, field, view and view-item method tests with a single cmp.Diff against a fully-populated want struct, which also verifies every decoded field.
Per review (the consensus from google#4202 that required response fields should be non-pointer), make ProjectV2View's required scalar fields non-pointer: id, number, name, layout, node_id, project_url and html_url. Optional (filter), nested struct (creator), timestamp and slice fields keep their existing pointer/slice forms.
Per review, drop the redundant "ProjectV2ViewSortBy: " prefix from the UnmarshalJSON error messages (and wrap err2, the actual string-parse error, in that branch), and remove the "(Required.)" notes from the CreateProjectV2ViewRequest Name and Layout field comments.
Per review, drop the inconsistent per-field comments from ProjectV2View so the struct matches the sibling ProjectV2 (no per-field comments), and add an explicit json:"-" tag to ProjectV2ViewSortBy's FieldID and Direction fields, which are (de)serialized through custom MarshalJSON / UnmarshalJSON rather than struct tags.
Per review, remove the parenthetical (Required.)/(Optional.) annotations from CreateProjectV2DraftItemRequest.Title/Body and the single-select option Name comments; the value/pointer type already conveys this. The conditional notes on AddProjectV2FieldRequest (required for single_select / iteration) are kept since they describe the one-of constraint.
Per the OpenAPI schema, creator, created_at, updated_at, visible_fields, sort_by, group_by and vertical_group_by are required on the project view response, so they should be non-pointer values without omitempty (matching the convention that only optional fields are pointers). The scalar fields were already converted; this does the same for the object, timestamp and slice fields. Filter stays a pointer as it is optional. Accessors are regenerated accordingly.
2aee530 to
10bea9e
Compare
Fixes #3715
Summary
ProjectsServicealready covers projects (get/list), fields (get/list), and items (full CRUD). Comparing the current code againstopenapi_operations.yaml, eight Projects v2 REST operations were present in the spec but not yet implemented. This PR adds them, completing the Projects v2 REST surface:CreateOrganizationProjectDraftItemCreateUserProjectDraftItemAddOrganizationProjectFieldAddUserProjectFieldCreateOrganizationProjectViewCreateUserProjectViewListOrganizationProjectViewItemsListUserProjectViewItemsNotes
ProjectV2Item; fields returnProjectV2Field; view items use the existingListProjectItemsOptions(before/after cursor pagination). New types:ProjectV2ViewplusCreateProjectV2DraftItemRequest,AddProjectV2FieldRequest,CreateProjectV2ViewRequest(and thesingle_select/iterationfield helper types). Request bodies are passed by value with aRequestsuffix, per theparamcheckconvention.user_idin the path (POST /user/{user_id}/projectsV2/{project_number}/draftsandPOST /users/{user_id}/projectsV2/{project_number}/views), unlike theusername-based paths used by the other user endpoints. These methods therefore take auserID int64, consistent withUsers.GetByID.openapi_operations.yaml(the operations were already present); generated accessors/iterators were regenerated viascript/generate.sh.Testing
Added success + error-path tests for all eight methods (including a single_select field, an iteration field, and view-item pagination).
go test -race ./github/,go vet,go generate ./... --check, and golangci-lint all pass.