Implement Subscriptions #364

alexstewartja · 2025-03-08T16:54:40Z

Core Changes

Created Subscription related models and API client
Integrated subscription client actions into the main Client class
Added functional Subscription tests

Auxiliary Changes

Added SettlementStatus class
Added a convenience const to Env class for BitPay's datetime format
Updated dependencies (symfony/console)

+ Created subscription related models + Integrated subscription actions into the main Client class + Added a convenience const to Env class for BitPay's datetime format + Added functional Subscription tests + Updated dependencies

bobbrodie · 2025-03-31T11:46:47Z

Thank you for the submission! We'll take a look and get this scheduled.

bobbrodie · 2025-05-28T01:12:48Z

src/BitPaySDK/Model/Subscription/BillData.php

I believe we can remove this file in favor of src/BitPaySDK/Model/Bill/Bill.php.

@alexstewartja I've taken a deeper look and I see that Bill and BillData won't be 1:1, however there's a lot of overlap. It could be worth considering extracting certain types to reduce duplication.

For example, they both have:

currency

email

items (which are Bill/Item)

number

name

address1

address2

city

state

zip

country

cc

phone

dueDate

passProcessingFee

We could think about extracting these into an class such as CoreBill and then extending it for Bill and using it as a type in Subscription for $billData. I'm not sure that it's worth it, but wanted to see what folks think.

If that won't work, it's OK, just thinking it through and sharing.

I agree, I'm all for composition and adherence to DRY principles. We should definitely abstract away those repetitive fields. I'll put your suggestion into action later today.

I agree with @bobbrodie, there is no need for another class. All missing fields can be added to Bill\Bill or you can create a class that extends it to eliminate duplicates.

bobbrodie · 2025-05-28T01:14:12Z

src/BitPaySDK/Model/Subscription/Item.php

If we can use src/BitPaySDK/Model/Bill/Bill.php, we'll carry the reference to src/BitPaySDK/Model/Bill/Item.php.

@alexstewartja to clarify the above, Subscription/Item carries the same properties as Bill/Item, and fundamentally represent the same thing, so even if we don't extract core bill properties and extend, I think we can still consider using Bill/Item instead of Subscription/Item.

I support using Bill\Item instead of a separate class.

bobbrodie · 2025-05-28T01:14:40Z

src/BitPaySDK/Model/Subscription/Subscription.php

+{
+    protected ?string $id = null;
+    protected ?string $status = null;
+    protected BillData $billData;


Let's consider src/BitPaySDK/Model/Bill/Bill.php

I get that they appear identical, but if this package is to be a pure abstraction over the API endpoints, then we have to account for the subtle differences in the structure/shape of Subscriptions vs Bills.

For example, the Create a Bill response data is unwrapped and relatively one-dimensional. On the other hand the Create a Subscription response data is wrapped in the billData field, one level deep.

Right, I'm sorry for not clarifying this a bit, @alexstewartja. This comment was for line 22. Currently, $billData is typed to BillData. We could keep $billData of course, but maybe have it typed to a new type with just the core bill fields that are shared between Bill and Subscription. I think there are ~15 common fields.

Since a subscription is just a recurring bill, so BillData represents a Bill, albeit with fewer properties.

Yes, thanks for the clarification. I'll create a base/core model then inherit/extend from it to implement the specifics of Subscriptions

swlodarski-sumoheavy · 2025-05-30T11:20:49Z

src/BitPaySDK/Model/Subscription/BillData.php

I agree with @bobbrodie, there is no need for another class. All missing fields can be added to Bill\Bill or you can create a class that extends it to eliminate duplicates.

swlodarski-sumoheavy · 2025-05-30T11:22:13Z

src/BitPaySDK/Model/Subscription/BillData.php

+        $this->number = $number;
+        $this->currency = $currency ?: Currency::USD;
+        $this->email = $email;
+        $this->setEmailBill();


Is it a good idea to force emailBill in the constructor? Shouldn't this be controlled by the developer?

swlodarski-sumoheavy · 2025-05-30T11:24:57Z

src/BitPaySDK/Model/Subscription/Item.php

I support using Bill\Item instead of a separate class.

swlodarski-sumoheavy · 2025-05-30T11:27:41Z

src/BitPaySDK/Client.php

@@ -576,7 +578,7 @@ public function getBill(string $billId, string $facade = Facade::MERCHANT, bool
     *
     * @see https://developer.bitpay.com/reference/retrieve-bills-by-status Retrieve Bills by Status
     *
-     * @param string|null The status to filter the bills.
+     * @param $status string|null The status to filter the bills.


The variable name should be after the type.

swlodarski-sumoheavy · 2025-05-30T11:28:09Z

src/BitPaySDK/Client.php

+     *
+     * @see https://developer.bitpay.com/reference/retrieve-a-subscription Retrieve a Subscription
+     *
+     * @param $subscriptionId string The ID of the subscription to retrieve.


Same as above

swlodarski-sumoheavy · 2025-05-30T11:28:33Z

src/BitPaySDK/Client.php

+     *
+     * @see https://developer.bitpay.com/reference/retrieve-subscriptions-by-status Retrieve Subscriptions by Status
+     *
+     * @param $status string|null The status on which to filter the subscriptions.


Incorrect order of type and var name

alexstewartja added 5 commits March 8, 2025 11:48

Implement Subscriptions

299fe76

+ Created subscription related models + Integrated subscription actions into the main Client class + Added a convenience const to Env class for BitPay's datetime format + Added functional Subscription tests + Updated dependencies

Implement Subscriptions

9962c88

+ Created subscription related models + Integrated subscription actions into the main Client class + Added a convenience const to Env class for BitPay's datetime format + Added functional Subscription tests + Updated dependencies

Merge remote-tracking branch 'origin/subscriptions' into subscriptions

52252e6

Fix PHP CodeSniffer action failure

24fb49c

Fix PHP CodeSniffer errors/warnings

808e3f9

alexstewartja marked this pull request as ready for review March 10, 2025 11:41

alexstewartja added 2 commits March 10, 2025 08:23

Added SettlementStatus convenience constants

77fa8e6

Fix PHP CodeSniffer warnings

399e0d4

bobbrodie added this to SDKs & Plugins Mar 31, 2025

bobbrodie added this to the v9.3.0 milestone Mar 31, 2025

bobbrodie moved this to Todo in SDKs & Plugins Apr 1, 2025

bobbrodie moved this from Todo to Under Review in SDKs & Plugins Apr 1, 2025

bobbrodie changed the base branch from 9.2.x to 9.3.x May 28, 2025 00:54

bobbrodie requested review from swlodarski-sumoheavy, p-maguire and bobbrodie May 28, 2025 00:56

bobbrodie reviewed May 28, 2025

View reviewed changes

swlodarski-sumoheavy reviewed May 30, 2025

View reviewed changes

Implement Subscriptions #364

Are you sure you want to change the base?

Implement Subscriptions #364

Uh oh!

Conversation

alexstewartja commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Core Changes

Auxiliary Changes

Uh oh!

bobbrodie commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrodie May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbrodie May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexstewartja commented Mar 8, 2025 •

edited

Loading

bobbrodie May 30, 2025 •

edited

Loading

bobbrodie May 30, 2025 •

edited

Loading