Feature/subscription subscribed any support#1806
Feature/subscription subscribed any support#1806thebetar wants to merge 2 commits intolaravel:16.xfrom
Conversation
…pe instead of latest
f7c3f78 to
7abfaff
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for checking if any subscription of a given type is valid, not just the most recent one. This is useful for platforms where users can have multiple active subscriptions of the same type (e.g., an organization with multiple subscription tiers for different device counts).
Key changes:
- Added an optional
$anyparameter to thesubscribed()method inManagesSubscriptionstrait - When
$anyistrue, the method checks all subscriptions of the given type instead of just the first (most recent) one - Added comprehensive test coverage for the new functionality with multiple subscription scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Concerns/ManagesSubscriptions.php | Added optional $any parameter to subscribed() method to enable checking if any subscription of a given type is valid |
| tests/Feature/SubscriptionsTest.php | Added test cases for the new $any parameter functionality, covering both cases where newest/oldest subscriptions are active/inactive; also includes minor code style updates (spacing around operators) |
Comments suppressed due to low confidence (1)
tests/Feature/SubscriptionsTest.php:1035
- The test should also cover the scenario where the
$anyparameter is used with a specific$priceto verify that it correctly checks if any subscription of the given type has that specific price. Currently, all tests with$any=truepassnullfor the price parameter.
$user = $this->createCustomer('test_subscribed_check_with_multiple_subscriptions');
// Case 1 & 2: Newest Active, Oldest Inactive
$subscriptionOld = $user->newSubscription('main', static::$priceId)->create('pm_card_visa');
$subscriptionOld->cancelNow();
$subscriptionNew = $user->newSubscription('main', static::$priceId)->create('pm_card_visa');
$user->refresh();
// 1. Two subscriptions do not use any, newest active, oldest inactive, should result in true
$this->assertTrue($user->subscribed('main'));
// 2. Two subscriptions do use any, newest active, oldest inactive, should result in true
$this->assertTrue($user->subscribed('main', null, true));
// Case 3 & 4: Newest Inactive, Oldest Active
$user = $this->createCustomer('test_subscribed_check_with_multiple_subscriptions_2');
$subscriptionOld = $user->newSubscription('main', static::$priceId)->create('pm_card_visa');
$subscriptionNew = $user->newSubscription('main', static::$priceId)->create('pm_card_visa');
$subscriptionNew->cancelNow();
$user->refresh();
// 3. Two subscriptions do not use any, Newest is inactive, oldest is active, should result in false
$this->assertFalse($user->subscribed('main'));
// 4. Tow subscriptions does use any, Newest is inactive, oldest is active, should result in true
$this->assertTrue($user->subscribed('main', null, true));
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| @@ -135,15 +135,25 @@ | |||
| * @param string|null $price | |||
There was a problem hiding this comment.
The docblock is missing documentation for the new $any parameter. It should include a line like @param bool $any with a description explaining that when true, it checks if any subscription of the given type is valid, not just the most recent one.
| * @param string|null $price | |
| * @param string|null $price | |
| * @param bool $any When true, checks if any subscription of the given type is valid, not just the most recent one. |
| // 3. Two subscriptions do not use any, Newest is inactive, oldest is active, should result in false | ||
| $this->assertFalse($user->subscribed('main')); | ||
|
|
||
| // 4. Tow subscriptions does use any, Newest is inactive, oldest is active, should result in true |
There was a problem hiding this comment.
Typo in comment: "Tow" should be "Two" and "does" should be "do" to maintain grammatical consistency with the previous comments.
| // 4. Tow subscriptions does use any, Newest is inactive, oldest is active, should result in true | |
| // 4. Two subscriptions do use any, Newest is inactive, oldest is active, should result in true |
| * @return bool | ||
| */ | ||
| public function subscribed(string $type = 'default', ?string $price = null): bool | ||
| public function subscribed(string $type = 'default', ?string $price = null, $any = false): bool |
There was a problem hiding this comment.
The parameter name $any is ambiguous. Consider a more descriptive name like $checkAny or $anyValid to clarify its purpose of checking if any subscription (not just the most recent) is valid.
| public function subscribed(string $type = 'default', ?string $price = null, $any = false): bool | ||
| { | ||
| $subscription = $this->subscription($type); | ||
|
|
||
| if (! $subscription || ! $subscription->valid()) { | ||
| return false; | ||
| if (! $any) { | ||
| $subscription = $this->subscription($type); | ||
|
|
||
| if (! $subscription || ! $subscription->valid()) { | ||
| return false; | ||
| } | ||
|
|
||
| return ! $price || $subscription->hasPrice($price); | ||
| } | ||
|
|
||
| return ! $price || $subscription->hasPrice($price); | ||
| return (bool) $this->subscriptions->where('type', $type)->first(function (Subscription $subscription) use ($price) { | ||
| if (! $subscription->valid()) { | ||
| return false; | ||
| } | ||
|
|
||
| return ! $price || $subscription->hasPrice($price); | ||
| }); | ||
| } |
There was a problem hiding this comment.
[nitpick] Consider whether the $any parameter is the best API design choice. The existing onPrice() and onProduct() methods already check all subscriptions (not just the first). The naming and behavior could be confusing since subscribed('main', $price) checks the first subscription while subscribed('main', $price, true) checks any subscription - this is inconsistent with how onPrice($price) works, which always checks all subscriptions regardless. Consider documenting this behavior difference clearly or reconsidering the API design.
| * @return bool | ||
| */ | ||
| public function subscribed(string $type = 'default', ?string $price = null): bool | ||
| public function subscribed(string $type = 'default', ?string $price = null, $any = false): bool |
There was a problem hiding this comment.
The parameter $any should have a type hint for consistency with the codebase. It should be bool $any = false instead of just $any = false.
| public function subscribed(string $type = 'default', ?string $price = null, $any = false): bool | |
| public function subscribed(string $type = 'default', ?string $price = null, bool $any = false): bool |
7abfaff to
e9f6280
Compare
|
I would probably just define your own method on the same model that uses this trait. 👍 |
This pull request is related to the issue I have opened here: #1805
To summarise I found out that when working with multiple subscription of the same type the
subscribedfunctions within the trainManagesSubscriptionsdoes not work as intended. It might be that I am the only one or the one of a small group of people using it like this so I have created an extra parameter to solve this. The default behaviour should stay the same while also solving the issue for users experiencing the same challenge.I think this issue only plays for platforms that allow for a user entity to have multiple of the same subscriptions for instance when an organisation has multiple subscriptions where there are scales of a subscription allows 10 devices to be connected and the more expensive 50. If the user requires only 20 devices it might be that two basic subscriptions are taken.