Add GitHub Issue Milestones functionality#13
Conversation
This commit implements comprehensive support for GitHub Issue Milestones, enabling project planning and tracking capabilities. Features added: - Milestone Data class with full GitHub API field support - ManagesMilestones trait with CRUD operations for milestones - ManagesMilestonesInterface contract defining the milestone operations API - Methods to assign/remove issues from milestones - Method to list issues within a milestone - Full test coverage for Milestone data transformations Breaking changes: - Issue::$milestone changed from ?string to ?Milestone object Resolves #9
WalkthroughAdds comprehensive milestone management support to the GitHub Issues library through a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Data/Issue.php (1)
31-51: Avoid undefined-index notices whenmilestoneis missing from payload.
$data['milestone'] ? ... : nullwill emit a notice if the key is absent (common with partial responses/mocks).- milestone: $data['milestone'] ? Milestone::fromArray($data['milestone']) : null, + milestone: (isset($data['milestone']) && is_array($data['milestone'])) + ? Milestone::fromArray($data['milestone']) + : null,
🧹 Nitpick comments (2)
tests/Unit/Data/MilestoneTest.php (1)
77-109: Consider asserting exact date formatting (if output format stability matters).
Right nowdue_onis only asserted as “string”, so a formatting change could slip through unnoticed.src/Data/Milestone.php (1)
46-63:toArray()shape is good; note it won’t necessarily round-trip the exact GitHub timestamp string.
format('c')yields+00:00instead ofZfor UTC inputs (still valid ISO-8601).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Contracts/IssuesServiceInterface.php(1 hunks)src/Contracts/ManagesMilestonesInterface.php(1 hunks)src/Data/Issue.php(3 hunks)src/Data/Milestone.php(1 hunks)src/Services/IssuesService.php(1 hunks)src/Traits/ManagesMilestones.php(1 hunks)tests/Unit/Data/IssueTest.php(6 hunks)tests/Unit/Data/MilestoneTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Data/Milestone.php (1)
src/Data/User.php (1)
User(7-38)
tests/Unit/Data/MilestoneTest.php (2)
src/Data/Milestone.php (5)
Milestone(9-74)fromArray(27-44)toArray(46-63)isOpen(65-68)isClosed(70-73)src/Data/User.php (1)
User(7-38)
src/Services/IssuesService.php (1)
src/Data/Issue.php (1)
Issue(9-86)
src/Data/Issue.php (1)
src/Data/Milestone.php (1)
Milestone(9-74)
tests/Unit/Data/IssueTest.php (3)
src/Data/Issue.php (1)
Issue(9-86)src/Data/Milestone.php (1)
Milestone(9-74)src/Data/User.php (1)
User(7-38)
🔇 Additional comments (9)
src/Data/Issue.php (2)
54-75:toArray()now emits a milestone object shape; ensure callers don’t expect a string.This is consistent with the new model, but any serialization consumers (JSON snapshots, caching, custom mappers) may need updates.
11-29: The?Milestonetype change is properly implemented with no downstream breaking changes. All usages correctly handle the nullableMilestoneinstance: deserialization viaMilestone::fromArray()with null coalescing, serialization via optional chaining (?->toArray()), and tests expectMilestoneinstances.src/Contracts/IssuesServiceInterface.php (1)
7-7: Interface composition looks consistent with new milestone capability.src/Services/IssuesService.php (1)
7-23: Trait wiring is clean; just watch for future trait method name collisions.tests/Unit/Data/MilestoneTest.php (1)
8-45: fromArray coverage (including nested creator) looks solid.tests/Unit/Data/IssueTest.php (2)
35-96: Test updates correctly validateIssue::fromArray()produces aMilestoneobject.
98-150:Issue::toArray()milestone assertions match the new nested shape.src/Traits/ManagesMilestones.php (1)
60-78: Confirm GitHub API semantics for milestone assignment and issue filtering.
update issuepayload: whethermilestoneexpects the milestone number (as implemented) across all API versions you target.list issuesfiltering: whethermilestonemust be a string and whether casting is necessary/desired.src/Contracts/ManagesMilestonesInterface.php (1)
11-34: Contract matches the trait API and keeps return types explicit.
| public static function fromArray(array $data): self | ||
| { | ||
| return new self( | ||
| id: $data['id'], | ||
| number: $data['number'], | ||
| title: $data['title'], | ||
| description: $data['description'] ?? null, | ||
| state: $data['state'], | ||
| openIssues: $data['open_issues'], | ||
| closedIssues: $data['closed_issues'], | ||
| createdAt: new DateTime($data['created_at']), | ||
| updatedAt: new DateTime($data['updated_at']), | ||
| closedAt: $data['closed_at'] ? new DateTime($data['closed_at']) : null, | ||
| dueOn: $data['due_on'] ? new DateTime($data['due_on']) : null, | ||
| htmlUrl: $data['html_url'], | ||
| creator: User::fromArray($data['creator']), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Harden fromArray() against missing closed_at / due_on keys.
- closedAt: $data['closed_at'] ? new DateTime($data['closed_at']) : null,
- dueOn: $data['due_on'] ? new DateTime($data['due_on']) : null,
+ closedAt: (isset($data['closed_at']) && $data['closed_at'] !== null) ? new DateTime($data['closed_at']) : null,
+ dueOn: (isset($data['due_on']) && $data['due_on'] !== null) ? new DateTime($data['due_on']) : null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function fromArray(array $data): self | |
| { | |
| return new self( | |
| id: $data['id'], | |
| number: $data['number'], | |
| title: $data['title'], | |
| description: $data['description'] ?? null, | |
| state: $data['state'], | |
| openIssues: $data['open_issues'], | |
| closedIssues: $data['closed_issues'], | |
| createdAt: new DateTime($data['created_at']), | |
| updatedAt: new DateTime($data['updated_at']), | |
| closedAt: $data['closed_at'] ? new DateTime($data['closed_at']) : null, | |
| dueOn: $data['due_on'] ? new DateTime($data['due_on']) : null, | |
| htmlUrl: $data['html_url'], | |
| creator: User::fromArray($data['creator']), | |
| ); | |
| } | |
| public static function fromArray(array $data): self | |
| { | |
| return new self( | |
| id: $data['id'], | |
| number: $data['number'], | |
| title: $data['title'], | |
| description: $data['description'] ?? null, | |
| state: $data['state'], | |
| openIssues: $data['open_issues'], | |
| closedIssues: $data['closed_issues'], | |
| createdAt: new DateTime($data['created_at']), | |
| updatedAt: new DateTime($data['updated_at']), | |
| closedAt: (isset($data['closed_at']) && $data['closed_at'] !== null) ? new DateTime($data['closed_at']) : null, | |
| dueOn: (isset($data['due_on']) && $data['due_on'] !== null) ? new DateTime($data['due_on']) : null, | |
| htmlUrl: $data['html_url'], | |
| creator: User::fromArray($data['creator']), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/Data/Milestone.php around lines 27 to 44, the fromArray() method assumes
'closed_at' and 'due_on' keys always exist; update it to first check those keys
(use array_key_exists or isset) and that their values are non-empty before
constructing DateTime, otherwise pass null. Replace the current ternary
expressions with checks like: if key exists and not null/empty then new
DateTime(value) else null; keep other fields unchanged.
| public function listMilestones(string $owner, string $repo, array $filters = []): Collection | ||
| { | ||
| $response = $this->connector->send( | ||
| $this->connector->get("/repos/{$owner}/{$repo}/milestones", $filters) | ||
| ); | ||
|
|
||
| return collect($response->json()) | ||
| ->map(fn (array $data) => Milestone::fromArray($data)); | ||
| } | ||
|
|
||
| public function getMilestone(string $owner, string $repo, int $milestoneNumber): Milestone | ||
| { | ||
| $response = $this->connector->send( | ||
| $this->connector->get("/repos/{$owner}/{$repo}/milestones/{$milestoneNumber}") | ||
| ); | ||
|
|
||
| return Milestone::fromArray($response->json()); | ||
| } | ||
|
|
||
| public function createMilestone(string $owner, string $repo, array $data): Milestone | ||
| { | ||
| $response = $this->connector->send( | ||
| $this->connector->post("/repos/{$owner}/{$repo}/milestones", $data) | ||
| ); | ||
|
|
||
| return Milestone::fromArray($response->json()); | ||
| } | ||
|
|
||
| public function updateMilestone(string $owner, string $repo, int $milestoneNumber, array $data): Milestone | ||
| { | ||
| $response = $this->connector->send( | ||
| $this->connector->patch("/repos/{$owner}/{$repo}/milestones/{$milestoneNumber}", $data) | ||
| ); | ||
|
|
||
| return Milestone::fromArray($response->json()); | ||
| } | ||
|
|
||
| public function deleteMilestone(string $owner, string $repo, int $milestoneNumber): void | ||
| { | ||
| $this->connector->send( | ||
| $this->connector->delete("/repos/{$owner}/{$repo}/milestones/{$milestoneNumber}") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.php" | grep -i connector | head -20Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
find . -type f -name "*.php" -path "*/src/*" | head -30Repository: conduit-ui/issue
Length of output: 621
🏁 Script executed:
git ls-files "*.php" | grep -E "(Connector|connector)" | head -20Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
cat -n src/Services/IssuesService.php | head -100Repository: conduit-ui/issue
Length of output: 834
🏁 Script executed:
cat -n src/Traits/ManagesIssues.php | head -150Repository: conduit-ui/issue
Length of output: 2340
🏁 Script executed:
find . -type f -name "composer.json" -o -name "composer.lock" | head -5Repository: conduit-ui/issue
Length of output: 91
🏁 Script executed:
cat composer.json | grep -A 5 -B 5 "GitHubConnector" 2>/dev/null || cat composer.json | head -100Repository: conduit-ui/issue
Length of output: 2747
🏁 Script executed:
find . -path "*/vendor/*" -name "Connector.php" 2>/dev/null | grep -i github | head -5Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
find . -path "*/vendor/conduit-ui/*" -type f -name "*.php" 2>/dev/null | head -20Repository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
rg "class Connector" -A 20 --type phpRepository: conduit-ui/issue
Length of output: 42
🏁 Script executed:
find ./tests -type f -name "*.php" 2>/dev/null | head -10Repository: conduit-ui/issue
Length of output: 228
🏁 Script executed:
rg "listMilestones|getMilestone" -A 10 --type phpRepository: conduit-ui/issue
Length of output: 2820
🌐 Web query:
conduit-ui/connector GitHub repository
💡 Result:
I couldn't find a GitHub repository named exactly "conduit-ui/connector". Related projects you might mean:
- conduit-ui/conduit (Conduit CLI platform) — package info and source reference. [1]
- conduit-ui/github-connector (a GitHub API connector package from conduit-ui on Packagist/Libraries.io). [1][2]
- ConduitPlatform/conduit-ui (a separate Conduit web UI repo / project surfaced by mirrors/analysis tools). [3]
If you want, tell me which one you meant (or paste a link) and I’ll open it and summarize the repo contents.
Sources: [1] Packagist conduit-ui listings; [2] Libraries.io entry for conduit-ui/github-connector; [3] Conduit-UI code listing (mirror/analysis).
🌐 Web query:
Saloon PHP HTTP client URL path parameter encoding
💡 Result:
Short answer: Saloon does not automatically percent‑encode URL path segments for you — you should encode path parameters yourself before returning the endpoint in resolveEndpoint. Use rawurlencode (RFC‑3986) per segment (or a URI library like League\Uri\Encoder::encodePath if you need finer control) and then build the endpoint string. [1][2][3]
Example (concept):
$segment = rawurlencode($value);
return "/items/{$segment}/details";
Sources:
[1] Saloon — Requests / resolveEndpoint (docs). (docs.saloon.dev)
[2] PHP rawurlencode() manual (RFC‑3986 behavior). (php.net)
[3] League\Uri Encoder — encodePath utility. (uri.thephpleague.com)
Citations:
- 1: https://docs.saloon.dev/2/the-basics/requests?utm_source=openai
- 2: https://www.php.net/manual/es/function.rawurlencode.php?utm_source=openai
- 3: https://uri.thephpleague.com/interfaces/7.0/encoder/?utm_source=openai
🌐 Web query:
site:packagist.org conduit-ui/connector
💡 Result:
I couldn't find a Packagist package named "conduit-ui/connector". The closest match is "conduit-ui/conduit" on Packagist. [1]
Sources
[1] conduit-ui/conduit — Packagist.
🏁 Script executed:
cat -n tests/TestCase.phpRepository: conduit-ui/issue
Length of output: 924
🏁 Script executed:
cat -n src/Traits/ManagesMilestones.phpRepository: conduit-ui/issue
Length of output: 3235
Encode $owner and $repo path parameters with rawurlencode().
Saloon (the underlying HTTP client) does not automatically percent-encode URL path segments. If these parameters contain special characters (including /), the endpoint will be malformed. Add rawurlencode() to all methods that interpolate these values:
public function listMilestones(string $owner, string $repo, array $filters = []): Collection
{
+ $owner = rawurlencode($owner);
+ $repo = rawurlencode($repo);
$response = $this->connector->send(
$this->connector->get("/repos/{$owner}/{$repo}/milestones", $filters)
);
@@
public function getMilestone(string $owner, string $repo, int $milestoneNumber): Milestone
{
+ $owner = rawurlencode($owner);
+ $repo = rawurlencode($repo);
$response = $this->connector->send(
$this->connector->get("/repos/{$owner}/{$repo}/milestones/{$milestoneNumber}")
);
@@
public function createMilestone(string $owner, string $repo, array $data): Milestone
{
+ $owner = rawurlencode($owner);
+ $repo = rawurlencode($repo);
$response = $this->connector->send(
$this->connector->post("/repos/{$owner}/{$repo}/milestones", $data)
);
@@
public function updateMilestone(string $owner, string $repo, int $milestoneNumber, array $data): Milestone
{
+ $owner = rawurlencode($owner);
+ $repo = rawurlencode($repo);
$response = $this->connector->send(
$this->connector->patch("/repos/{$owner}/{$repo}/milestones/{$milestoneNumber}", $data)
);
@@
public function deleteMilestone(string $owner, string $repo, int $milestoneNumber): void
{
+ $owner = rawurlencode($owner);
+ $repo = rawurlencode($repo);
$this->connector->send(
$this->connector->delete("/repos/{$owner}/{$repo}/milestones/{$milestoneNumber}")
);
}Note: This same pattern appears in ManagesIssues and other traits and should be fixed consistently across the codebase.
🤖 Prompt for AI Agents
In src/Traits/ManagesMilestones.php around lines 16 to 58, path parameters
$owner and $repo are interpolated directly into URLs and must be
percent-encoded; update every method that builds a repo path (listMilestones,
getMilestone, createMilestone, updateMilestone, deleteMilestone) to wrap $owner
and $repo with rawurlencode() when constructing the endpoint strings so special
characters (including "/") are encoded, and apply the same change to other
traits like ManagesIssues that use the same pattern to ensure consistent, safe
URL construction across the codebase.
|
Features already shipped in #14 (bundled with gate workflow) |
1 participant
Summary
This PR implements comprehensive support for GitHub Issue Milestones, enabling project planning and tracking capabilities as requested in #9.
Features Implemented
Milestone Operations:
listMilestones)getMilestone)createMilestone)updateMilestone)deleteMilestone)Milestone Assignment:
assignIssueToMilestone)removeIssueFromMilestone)listMilestoneIssues)Implementation Details
New Files:
src/Data/Milestone.php- Milestone data class with full GitHub API field supportsrc/Contracts/ManagesMilestonesInterface.php- Interface defining milestone operationssrc/Traits/ManagesMilestones.php- Trait implementing milestone CRUD and issue assignmenttests/Unit/Data/MilestoneTest.php- Comprehensive test coverage for Milestone data transformationsModified Files:
src/Services/IssuesService.php- Added ManagesMilestones traitsrc/Contracts/IssuesServiceInterface.php- Extended ManagesMilestonesInterfacesrc/Data/Issue.php- Changed milestone property from ?string to ?Milestone objecttests/Unit/Data/IssueTest.php- Updated tests for new Milestone object typeBreaking Changes
Issue::$milestoneproperty changed from?stringto?Milestoneobject$issue->milestonereturned a string like "v1.0"$issue->milestone->titlereturns "v1.0", with access to full milestone detailsGitHub API Endpoints Used
GET /repos/{owner}/{repo}/milestones- List milestonesGET /repos/{owner}/{repo}/milestones/{milestone_number}- Get milestonePOST /repos/{owner}/{repo}/milestones- Create milestonePATCH /repos/{owner}/{repo}/milestones/{milestone_number}- Update milestoneDELETE /repos/{owner}/{repo}/milestones/{milestone_number}- Delete milestoneTesting
All tests pass successfully:
Closes #9
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.