feat(integration-github): emit event when pull request is approved#374
Closed
Re-Gouveia-QA wants to merge 13 commits into
Closed
feat(integration-github): emit event when pull request is approved#374Re-Gouveia-QA wants to merge 13 commits into
Re-Gouveia-QA wants to merge 13 commits into
Conversation
Co-authored-by: Sther <72408918+stherzada@users.noreply.github.com> Signed-off-by: Daniel Reis <danielhe4rt@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds onboarding and squads bounded-context docs/config, updates repo dependency mapping and root package requirements, and introduces a new Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 3
🧹 Nitpick comments (4)
composer.json (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent version constraints for new packages.
he4rt/onboardingandhe4rt/squadsuse*while otherhe4rt/*packages use>=1or^1.0. Align constraints with the existing pattern to prevent accidental major-version upgrades.Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composer.json` at line 32, The new package constraints for he4rt/onboarding and he4rt/squads are inconsistent with the rest of the he4rt/* entries in composer.json. Update the dependency versions in the composer manifest to match the existing pattern used by the other packages (such as >=1 or ^1.0) so the added packages do not allow unrestricted major upgrades.app-modules/onboarding/CONTEXT.md (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifiers to fenced code blocks.
-``` +```textAlso applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app-modules/onboarding/CONTEXT.md` at line 26, The fenced code block in CONTEXT.md is missing a language specifier, so update the affected markdown fences to use the appropriate format label; locate the code block(s) near the onboarding context content and add the specifier consistently to both referenced occurrences.app-modules/squads/CONTEXT.md (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to fenced code block.
-``` +```text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app-modules/squads/CONTEXT.md` at line 50, The fenced code block in CONTEXT.md is missing a language specifier, so update the markdown fence to use an explicit text label. Locate the affected fenced block in the context documentation and change it to a properly annotated code block so the formatter and renderers treat it as plain text.app-modules/integration-github/src/Webhook/ProjectGithubEvent.php (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
intFrominstead of casting a string-coerced value.
(int) $this->stringFrom(...)round-trips through string;intFromalready returns a typed int from the payload.- pr_number: (int) $this->stringFrom($payload, 'pull_request.number'), + pr_number: $this->intFrom($payload, 'pull_request.number') ?? 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app-modules/integration-github/src/Webhook/ProjectGithubEvent.php` at line 117, The pull request number parsing in ProjectGithubEvent is using a string coercion followed by a cast, which is unnecessary and less direct. Update the payload mapping in the event-handling code to use intFrom for pr_number instead of casting the result of stringFrom, keeping the typed integer flow consistent with the other payload accessors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app-modules/integration-github/src/Webhook/ProjectGithubEvent.php`:
- Around line 111-121: The approved pull request event is being dispatched
inside the per-tenant review flow, which causes duplicate
`GithubPullRequestApproved` emissions when `execute()` iterates over multiple
tenants for the same repo. Move the `state === 'approved'` check and the
`event(new GithubPullRequestApproved(...))` call out of
`ProjectGithubEvent::review()` and into `maybeEmitApproved()` so the PR-level
event is emitted once per delivery, before or outside the tenant fan-out loop.
In `@app-modules/integration-github/tests/Feature/GithubWebhookTest.php`:
- Around line 178-187: The current webhook test only covers duplicate delivery
handling for one repository, so it does not exercise the multi-tenant fan-out
path in ProjectGithubEvent::review. Add a new test alongside GithubWebhookTest
that creates two tenant records tracking the same GithubRepository full_name,
then send the same pull_request_review webhook delivery and assert
GithubPullRequestApproved is dispatched exactly once. Use the existing
postGithubWebhook helper and Event::fake/assertDispatchedTimes to verify the
per-tenant deduplication behavior.
In `@app-modules/onboarding/composer.json`:
- Around line 1-27: The composer metadata for the onboarding package is out of
sync with the lock file, so update the dependency state and regenerate
composer.lock to include the new packages. Use the package definition in
composer.json for He4rt\\Onboarding and then run the appropriate Composer update
for the affected packages so he4rt/onboarding and he4rt/squads are reflected in
the lock file.
---
Nitpick comments:
In `@app-modules/integration-github/src/Webhook/ProjectGithubEvent.php`:
- Line 117: The pull request number parsing in ProjectGithubEvent is using a
string coercion followed by a cast, which is unnecessary and less direct. Update
the payload mapping in the event-handling code to use intFrom for pr_number
instead of casting the result of stringFrom, keeping the typed integer flow
consistent with the other payload accessors.
In `@app-modules/onboarding/CONTEXT.md`:
- Line 26: The fenced code block in CONTEXT.md is missing a language specifier,
so update the affected markdown fences to use the appropriate format label;
locate the code block(s) near the onboarding context content and add the
specifier consistently to both referenced occurrences.
In `@app-modules/squads/CONTEXT.md`:
- Line 50: The fenced code block in CONTEXT.md is missing a language specifier,
so update the markdown fence to use an explicit text label. Locate the affected
fenced block in the context documentation and change it to a properly annotated
code block so the formatter and renderers treat it as plain text.
In `@composer.json`:
- Line 32: The new package constraints for he4rt/onboarding and he4rt/squads are
inconsistent with the rest of the he4rt/* entries in composer.json. Update the
dependency versions in the composer manifest to match the existing pattern used
by the other packages (such as >=1 or ^1.0) so the added packages do not allow
unrestricted major upgrades.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3e9a0da5-0326-46d0-9091-f10cf8fda1d5
📒 Files selected for processing (29)
CONTEXT-MAP.mdapp-modules/integration-github/src/Events/GithubPullRequestApproved.phpapp-modules/integration-github/src/Webhook/ProjectGithubEvent.phpapp-modules/integration-github/tests/Feature/GithubWebhookTest.phpapp-modules/onboarding/CONTEXT.mdapp-modules/onboarding/composer.jsonapp-modules/onboarding/database/factories/.gitkeepapp-modules/onboarding/database/migrations/.gitkeepapp-modules/onboarding/database/seeders/.gitkeepapp-modules/onboarding/docs/adr/0001-onboarding-polimorfico-por-tipo.mdapp-modules/onboarding/docs/adr/0002-sinal-de-pr-aprovado-via-evento-de-dominio.mdapp-modules/onboarding/phpstan.ignore.neonapp-modules/onboarding/phpstan.neonapp-modules/onboarding/src/Providers/OnboardingServiceProvider.phpapp-modules/onboarding/tests/Feature/.gitkeepapp-modules/onboarding/tests/Unit/.gitkeepapp-modules/squads/CONTEXT.mdapp-modules/squads/composer.jsonapp-modules/squads/database/factories/.gitkeepapp-modules/squads/database/migrations/.gitkeepapp-modules/squads/database/seeders/.gitkeepapp-modules/squads/docs/adr/0001-governanca-como-registro.mdapp-modules/squads/docs/adr/0002-modelo-de-dados.mdapp-modules/squads/phpstan.ignore.neonapp-modules/squads/phpstan.neonapp-modules/squads/src/Providers/SquadsServiceProvider.phpapp-modules/squads/tests/Feature/.gitkeepapp-modules/squads/tests/Unit/.gitkeepcomposer.json
Comment on lines
+111
to
+121
| $state = $this->stringFrom($review, 'state'); | ||
| if ($state === 'approved') { | ||
| event( | ||
| new GithubPullRequestApproved( | ||
| author_login: $this->stringFrom($payload, 'pull_request.user.login', 'ghost'), | ||
| repo: $repo, | ||
| pr_number: (int) $this->stringFrom($payload, 'pull_request.number'), | ||
| approved_at: $submittedAt, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Event emitted inside the per-tenant fan-out loop — duplicate emissions when multiple tenants track the same repo.
execute() calls review() once per tenant in tenantsTracking($repo). GithubPullRequestApproved carries no tenant_id and is emitted unconditionally on state === 'approved', so a single approval on a repo tracked by N tenants dispatches N events. Downstream resolves the PR author and awards coins/xp, so the author is rewarded N times. This bypasses the dedup guard RecordContribution uses (wasRecentlyCreated). Tests pass only because they create a single repository/tenant.
Emit this PR-level event once per delivery, outside the tenant loop.
Proposed direction
public function execute(string $event, array $payload): void
{
$repo = Str::lower($this->stringFrom($payload, 'repository.full_name'));
if ($repo === '') {
return;
}
+
+ if ($event === 'pull_request_review') {
+ $this->maybeEmitApproved($repo, $payload);
+ }
foreach ($this->tenantsTracking($repo) as $tenantId) {
match ($event) {Move the approved-state check + event(...) into maybeEmitApproved() and drop it from review().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/integration-github/src/Webhook/ProjectGithubEvent.php` around
lines 111 - 121, The approved pull request event is being dispatched inside the
per-tenant review flow, which causes duplicate `GithubPullRequestApproved`
emissions when `execute()` iterates over multiple tenants for the same repo.
Move the `state === 'approved'` check and the `event(new
GithubPullRequestApproved(...))` call out of `ProjectGithubEvent::review()` and
into `maybeEmitApproved()` so the PR-level event is emitted once per delivery,
before or outside the tenant fan-out loop.
Comment on lines
+178
to
+187
| it('nao duplica GithubPullRequestApproved em reentrega do mesmo delivery id', function (): void { | ||
| Event::fake([GithubPullRequestApproved::class]); | ||
| GithubRepository::factory()->create(['full_name' => 'he4rt/heartdevs.com']); | ||
| $delivery = Str::uuid()->toString(); | ||
|
|
||
| postGithubWebhook('pull_request_review', prReviewWebhookPayload(), delivery: $delivery)->assertSuccessful(); | ||
| postGithubWebhook('pull_request_review', prReviewWebhookPayload(), delivery: $delivery)->assertSuccessful(); | ||
|
|
||
| Event::assertDispatchedTimes(GithubPullRequestApproved::class, 1); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing coverage: multi-tenant fan-out.
All tests create a single repository, so the per-tenant duplicate-emission path (see ProjectGithubEvent::review) is never exercised. Add a test that registers two tenants tracking the same full_name and asserts GithubPullRequestApproved dispatches exactly once.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/integration-github/tests/Feature/GithubWebhookTest.php` around
lines 178 - 187, The current webhook test only covers duplicate delivery
handling for one repository, so it does not exercise the multi-tenant fan-out
path in ProjectGithubEvent::review. Add a new test alongside GithubWebhookTest
that creates two tenant records tracking the same GithubRepository full_name,
then send the same pull_request_review webhook delivery and assert
GithubPullRequestApproved is dispatched exactly once. Use the existing
postGithubWebhook helper and Event::fake/assertDispatchedTimes to verify the
per-tenant deduplication behavior.
Comment on lines
+1
to
+27
| { | ||
| "name": "he4rt/onboarding", | ||
| "description": "", | ||
| "type": "library", | ||
| "version": "1.0.0", | ||
| "license": "proprietary", | ||
| "autoload": { | ||
| "psr-4": { | ||
| "He4rt\\Onboarding\\": "src/", | ||
| "He4rt\\Onboarding\\Database\\Factories\\": "database/factories/", | ||
| "He4rt\\Onboarding\\Database\\Seeders\\": "database/seeders/" | ||
| } | ||
| }, | ||
| "autoload-dev": { | ||
| "psr-4": { | ||
| "He4rt\\Onboarding\\Tests\\": "tests/" | ||
| } | ||
| }, | ||
| "minimum-stability": "stable", | ||
| "extra": { | ||
| "laravel": { | ||
| "providers": [ | ||
| "He4rt\\Onboarding\\Providers\\OnboardingServiceProvider" | ||
| ] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Regenerate composer.lock to include new packages.
The CI warning indicates the lock file is stale. Run composer update to pick up he4rt/onboarding and he4rt/squads.
🧰 Tools
🪛 GitHub Actions: Continuous Integration / Setup PHP
[warning] 1-1: Composer install warning: The lock file is not up to date with the latest changes in composer.json. Required packages are missing from composer.lock (he4rt/onboarding, he4rt/squads).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app-modules/onboarding/composer.json` around lines 1 - 27, The composer
metadata for the onboarding package is out of sync with the lock file, so update
the dependency state and regenerate composer.lock to include the new packages.
Use the package definition in composer.json for He4rt\\Onboarding and then run
the appropriate Composer update for the affected packages so he4rt/onboarding
and he4rt/squads are reflected in the lock file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contexto
Closes #345