[4.x] Create pending tenants with pending_since, improve --with-pending#1458
Conversation
Pasing --with-pending, --with-pending=1 or --with-pending=true now makes tenant commands include pending tenants regardless of the include_in_queries config, and passing --with-pending=false, --with-pending=0 or --with-pending=foo makes the commands exclude pending tenants.
Cover that --with-pending can now accept values, and that the option takes precedence over include_in_queries config. Also simplify/improve existing assertions.
Instead of creating pending tenants without pending_since, letting job pipelines (e.g. TenantCreated) run, and only after that, setting the tenant's pending_since, set it while creating. This allows checking tenant's pending status accurately e.g. in the CreateDatabase job pipeline.
Now that tenants are created with pending_since right away, the migrate and seed database jobs need to include pending tenants for creating pending tenants to work fully. The jobs can still exclude pending tenants if $includePending is set to false. Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references. 📝 WalkthroughWalkthrough
ChangesPending tenant behavior
Sequence DiagramsequenceDiagram
participant CLI
participant JobPipeline
participant TenantModel
participant MigrateDatabase
participant SeedDatabase
participant Artisan
participant DB
CLI->>JobPipeline: dispatch tenant lifecycle (may include --with-pending)
JobPipeline->>TenantModel: create tenant (attributes include pending_since when pending)
TenantModel-->>JobPipeline: creating/created events fire if tenant->pending()
JobPipeline->>MigrateDatabase: enqueue/run (forward --with-pending = MigrateDatabase::$includePending ?? config)
MigrateDatabase->>Artisan: call tenants:migrate with --with-pending
Artisan->>DB: run migrations for tenant DB
JobPipeline->>SeedDatabase: enqueue/run (forward --with-pending = SeedDatabase::$includePending ?? config)
SeedDatabase->>Artisan: call tenants:seed with --with-pending
Artisan->>DB: run seeders for tenant DB
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1458 +/- ##
============================================
+ Coverage 85.90% 86.04% +0.14%
- Complexity 1170 1182 +12
============================================
Files 185 185
Lines 3412 3447 +35
============================================
+ Hits 2931 2966 +35
Misses 481 481 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/PendingTenantsTest.php (1)
195-239: 🧹 Nitpick | 🔵 TrivialConsider adding test coverage for invalid
--with-pendingvalues.The current tests cover truthy (
--with-pending,=true,=1) and falsy (=false,=0) values. Per the PR description, "invalid values treated as false" viafilter_var. Consider adding a test case with an invalid value like--with-pending=invalidto document this behavior.📝 Suggested test addition
test('commands exclude pending tenants when invalid --with-pending value is passed', function () { config(['tenancy.pending.include_in_queries' => true]); $pendingTenant = Tenant::createPending(); $normalTenant = Tenant::create(); // Invalid values are treated as false per filter_var behavior $command = pest()->artisan("tenants:run 'bar testing testing@test.test password foo' --with-pending=invalid"); $command->expectsOutputToContain("Tenant: {$normalTenant->getTenantKey()}"); $command->doesntExpectOutputToContain("Tenant: {$pendingTenant->getTenantKey()}"); $command->assertSuccessful(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/PendingTenantsTest.php` around lines 195 - 239, Add a new test in PendingTenantsTest.php that verifies invalid --with-pending values are treated as false: create one pending and one normal Tenant, set config(['tenancy.pending.include_in_queries' => true]), run the tenants:run command with the flag "--with-pending=invalid" (use pest()->artisan as in the existing tests), and assert that the normal tenant appears in output while the pending tenant does not; name the test similar to test('commands exclude pending tenants when invalid --with-pending value is passed') to match existing test style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Concerns/HasPending.php`:
- Around line 52-60: The CreatingPendingTenant event is being dispatched before
the model is persisted, so listeners (especially with the GeneratesIds trait)
see a tenant without an ID; move the event dispatch to after the model is saved
so the ID is generated first: create the tenant with static::make(...) and call
$tenant->save(), then call event(new CreatingPendingTenant($tenant));
references: static::make, static::getPendingAttributes, $tenant->save(), and the
CreatingPendingTenant event.
---
Outside diff comments:
In `@tests/PendingTenantsTest.php`:
- Around line 195-239: Add a new test in PendingTenantsTest.php that verifies
invalid --with-pending values are treated as false: create one pending and one
normal Tenant, set config(['tenancy.pending.include_in_queries' => true]), run
the tenants:run command with the flag "--with-pending=invalid" (use
pest()->artisan as in the existing tests), and assert that the normal tenant
appears in output while the pending tenant does not; name the test similar to
test('commands exclude pending tenants when invalid --with-pending value is
passed') to match existing test style.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73bb6188-dcf8-449c-82e3-b0795c3952ca
📒 Files selected for processing (5)
src/Concerns/HasTenantOptions.phpsrc/Database/Concerns/HasPending.phpsrc/Jobs/MigrateDatabase.phpsrc/Jobs/SeedDatabase.phptests/PendingTenantsTest.php
Include --with-pending=foo as the fasly option in the falsy --with-pending test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Concerns/HasPending.php`:
- Around line 64-68: The createPending() method currently uses array_merge([...
'pending_since' => now()->timestamp], static::getPendingAttributes($attributes),
$attributes) which allows callers to override 'pending_since' (e.g., passing
null) and produce a non-pending tenant; modify createPending() so the enforced
'pending_since' cannot be overwritten—either remove 'pending_since' from the
incoming $attributes before merging, or perform the merge so the timestamp is
applied last (or use array union + to prefer the existing timestamp), ensuring
createPending() always sets a non-null pending_since and thus always creates a
pending tenant; update any references to getPendingAttributes(...) if needed to
avoid reintroducing the override.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 208319f4-ed48-45b1-a11f-9a4cf977ffb7
📒 Files selected for processing (2)
src/Database/Concerns/HasPending.phptests/PendingTenantsTest.php
Reverting the HasPending changes (in createPending(), create tenant without pending_since, and only after that, update the tenant to give it pending_since) makes the test fail.
…reation Ensures that createPending always creates tenants with pending_since set to the current timestamp.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Database/Concerns/HasPending.php (1)
32-35:⚠️ Potential issue | 🟠 Major
CreatingPendingTenantnow fires before the tenant is persisted.Line 32 moves this public event into Eloquent’s
creatingphase, so listeners now run before the tenant row exists in storage. That is a behavioral break for any listener that queries the tenant, depends on DB-populated state, or expects the record to be readable by key. Please either keep this dispatch on the post-save path or document the timing change as a breaking change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Concerns/HasPending.php` around lines 32 - 35, The CreatingPendingTenant event is now dispatched during Eloquent’s creating phase (static::creating) which runs before the tenant row is persisted; to restore previous behavior move the dispatch into the post-save lifecycle (e.g., static::created or static::saved) so listeners see a persisted record, i.e. replace the static::creating(...) handler with a static::created(...) or static::saved(...) handler that checks $tenant->pending() and fires event(new CreatingPendingTenant($tenant)); if this timing change is intentional instead update documentation/changelog to mark it as a breaking change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Concerns/HasPending.php`:
- Around line 32-35: The CreatingPendingTenant event is now dispatched during
Eloquent’s creating phase (static::creating) which runs before the tenant row is
persisted; to restore previous behavior move the dispatch into the post-save
lifecycle (e.g., static::created or static::saved) so listeners see a persisted
record, i.e. replace the static::creating(...) handler with a
static::created(...) or static::saved(...) handler that checks
$tenant->pending() and fires event(new CreatingPendingTenant($tenant)); if this
timing change is intentional instead update documentation/changelog to mark it
as a breaking change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e6c14c2-cc7b-4baf-b8a6-44fa8e7f29db
📒 Files selected for processing (2)
src/Database/Concerns/HasPending.phptests/PendingTenantsTest.php
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Concerns/HasTenantOptions.php`:
- Line 21: Remove or resolve the inline TODO comment on the InputOption
declaration for '--with-pending' in HasTenantOptions.php: either delete the "//
todo@pending mention in docs" comment, replace it with a reference to the
documentation page or issue tracker (e.g., "see docs:
<doc-path-or-issue-number>"), or open and reference a new issue tracking the
docs update so the reminder is not left in source; update the comment text to
include the issue/PR number or documentation path next to the
InputOption('with-pending', ...) declaration.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d952e8d-aafd-440e-93c9-215d054af511
📒 Files selected for processing (5)
src/Concerns/HasTenantOptions.phpsrc/Database/Concerns/HasPending.phpsrc/Jobs/MigrateDatabase.phpsrc/Jobs/SeedDatabase.phptests/PendingTenantsTest.php
Resolved in archtechx/tenancy-docs#5
There was a problem hiding this comment.
Pull request overview
This PR fixes pending-tenant lifecycle timing by setting pending_since at creation time (so pending status is observable during early creation pipelines) and enhances tenant commands/jobs so migrations/seeders can reliably include or exclude pending tenants via an improved --with-pending option.
Changes:
- Set
pending_sinceduringcreatePending()and dispatch pending-tenant lifecycle events from Eloquentcreating/createdhooks. - Extend
--with-pendingto accept boolean-like values and make it overridetenancy.pending.include_in_querieswhen explicitly provided. - Ensure
MigrateDatabase/SeedDatabasejobs pass--with-pendingbased on a configurable static flag; expand test coverage for these behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/PendingTenantsTest.php | Adds/updates tests for --with-pending parsing and job-driven migrate/seed behavior for pending tenants. |
| src/Jobs/SeedDatabase.php | Adds SeedDatabase::$includePending and forwards it to tenants:seed --with-pending=.... |
| src/Jobs/MigrateDatabase.php | Adds MigrateDatabase::$includePending and forwards it to tenants:migrate --with-pending=.... |
| src/Database/Concerns/HasPending.php | Sets pending_since during creation and dispatches pending lifecycle events via model hooks. |
| src/Concerns/HasTenantOptions.php | Changes --with-pending to accept values and adjusts query inclusion logic accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| Artisan::call('tenants:migrate', [ | ||
| '--tenants' => [$this->tenant->getTenantKey()], | ||
| '--with-pending' => static::$includePending, |
There was a problem hiding this comment.
The docblock for includePending says:
/**
* Should pending tenants be included while migrating,
* regardless of the tenancy.pending.include_in_queries config value.
*/But there are three possible states - force include in pending, force exclude from pending, and default to config. This docblock sounds like it's just about whether a forced inclusion should be done, but if the value of the static property is set to false, '--with-pending' => false will be passed resulting in forced exclusion when the user would likely expect just defaulting to the config.
There was a problem hiding this comment.
To change this to default to the config we could just add ?: config('tenancy.pending.include_in_queries') but I guess there might be use cases for specifically excluding.
So maybe making this nullable, adding a ?? default, and expanding the docblock to mention that behavior would be ideal. Would also more closely match the behavior expected in this PR's tests.
| Event::listen(TenantCreated::class, JobPipeline::make([ | ||
| CreateDatabase::class, | ||
| PendingTenantJob::class, | ||
| ])->send(function (TenantCreated $event) { |
There was a problem hiding this comment.
The PR description talks about:
This is a problem if someone wants to e.g. add a job to the DatabaseCreated job pipeline
But the added test uses a TenantCreated pipeline. Those two might behave differently so it might be worth seeing if this regression test acts as intended.
There was a problem hiding this comment.
Trying this test on master - I get false with both TenantCreated and DatabaseCreated.
On this branch it passes with both this code as well as
Event::listen(TenantCreated::class, JobPipeline::make([
CreateDatabase::class,
])->send(function (TenantCreated $event) {
return $event->tenant;
})->toListener());
Event::listen(DatabaseCreated::class, JobPipeline::make([
PendingTenantJob::class,
])->send(function (DatabaseCreated $event) {
return $event->tenant;
})->toListener());So we can probably merge it like this since it's simpler. Just keeping this review here to note the difference in tests and PR desc.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/Jobs/MigrateDatabase.php`:
- Around line 20-27: The docblock in MigrateDatabase contains contradictory
phrasing about including pending tenants; update the comment on the relevant
property/method in the MigrateDatabase class so it clearly states behavior for
each value (true = include pending tenants, false = exclude pending tenants,
null = fall back to tenancy.pending.include_in_queries config) and remove the
phrase "regardless of the tenancy.pending.include_in_queries config value" so
the null/default behavior is unambiguous.
In `@src/Jobs/SeedDatabase.php`:
- Around line 20-27: Update the docblock above the pending-tenants flag in the
SeedDatabase class to remove the contradictory phrase "regardless of the
tenancy.pending.include_in_queries config value" and instead state that the flag
overrides that config when set to true or false and that when null the flag
defaults to the tenancy.pending.include_in_queries configuration; keep the
three-case explanation (true = include, false = exclude, null = use config) and
ensure the description clearly references the SeedDatabase pending-tenants flag
so readers know which property is affected.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: edd4f056-4bdb-4b15-a9b9-37faddb921c7
📒 Files selected for processing (3)
src/Jobs/MigrateDatabase.phpsrc/Jobs/SeedDatabase.phptests/PendingTenantsTest.php
Pending tenants aren't getting their
pending_sinceset until they're created completely (e.g. their DB was created, migrated and seeded -- first, the tenant is created fully, and only after that, the tenant is updated to havepending_since). This is a problem if someone wants to e.g. add a job to theDatabaseCreatedjob pipeline that would check$this->tenant->pending(). Since at the point ofDatabaseCreated, the tenant'spending_sinceisn't set yet,$this->tenant->pending()returnsfalse, even for tenants created usingcreatePending(). So instead of letting the pending tenant get fully created, and only after that, setting itspending_since(usingupdate()), we now setpending_sinceincreate().CreatingPendingTenantis now dispatched from thestatic::creatinghook, andPendingTenantCreatedis dispatched fromstatic::createdfor consistency.Setting
pending_sinceright increate()made theMigrateDatabaseandSeedDatabasejobs exclude the pending tenants during their creation if thetenancy.pending.include_in_queriesconfig was set tofalse-- in that case, these jobs would never migrate or seed the databases of pending tenants. So these jobs now pass--with-pendingto their underlying commands, with the value set in their$includePendingstatic property (trueby default). This overrides thetenancy.pending.include_in_queriesconfig -- unless the$includePendingproperties are set tofalse, these jobs will always include pending tenants.The
--with-pendingtenant command option originally worked just to opt-in for including pending tenants in the command. Now,--with-pendingcan accept values (true/1orfalse/0), so e.g.tenants:run foowith--with-pending/--with-pending=true/--with-pending=1includes pending tenantstenants:run foowith--with-pending=false/--with-pending=0excludes pending tenants (also--with-pending=foobar-- invalid input, consideredfalse)Passing
--with-pendingmakes the command bypass thetenancy.pending.include_in_queriesconfig (so e.g. iftenancy.pending.include_in_queriesis set totrue, and--with-pending=falseis passed to a command, the command will exclude pending tenants). When--with-pendingis not passed, the command will include or exclude pending tenants based on thetenancy.pending.include_in_queriesconfig.In tenancy-docs, I opened a PR with a note about the
--with-pendingand job changes (https://github.com/archtechx/tenancy-docs/pull/5).Summary by CodeRabbit