Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for new Event Meters #1698

Merged
merged 21 commits into from
Aug 13, 2024
Merged

Add support for new Event Meters #1698

merged 21 commits into from
Aug 13, 2024

Conversation

Bark-fa
Copy link
Contributor

@Bark-fa Bark-fa commented Jul 30, 2024

Hello! This PR adds support for the new event meters by stripe for tracking usage in usage-based subscriptions, this is my first time contributing to cashier, and I've made every effort to maintain the style and naming conventions, and everything is fully typed, please let me know if I missed something either in the code itself or the tests.

Thank you so much for your time and effort.

@Bark-fa Bark-fa changed the title Add support for new Event Meter Add support for new Event Meters Jul 30, 2024
@Bark-fa
Copy link
Contributor Author

Bark-fa commented Jul 30, 2024

I see the CI has failed but for some reason, I cannot see why, it shows a generic "blocked" message, is it some geo-restriction?

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Jul 30, 2024

I managed to access it with a VPN, all style issues have been fixed

@driesvints
Copy link
Member

Can you remove all of your idea settings from this PR? Thanks!

@driesvints driesvints marked this pull request as draft August 1, 2024 13:11
@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 1, 2024

Ah yes, sorry about that I thought they'd be in the .gitignore!

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 1, 2024

done @driesvints

@Bark-fa Bark-fa marked this pull request as ready for review August 1, 2024 14:08
@driesvints driesvints self-assigned this Aug 1, 2024
@driesvints driesvints marked this pull request as draft August 1, 2024 15:17
@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 1, 2024

@driesvints Do you need me to delete them from the previous commit history too?

@driesvints
Copy link
Member

@Bark-fa no that's okay. This PR will get squashed anyway.

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 7, 2024

Hello @driesvints, I'm reaching out because I noticed the PR is still in draft, I know you must be very busy, so if the PR needs any more work or if I missed anything in the docs please let me know and I'll gladly take it off your hands, I've got plenty of time this week.

Thanks!

src/Subscription.php Outdated Show resolved Hide resolved
src/Subscription.php Outdated Show resolved Hide resolved
src/Subscription.php Outdated Show resolved Hide resolved
src/SubscriptionItem.php Outdated Show resolved Hide resolved
src/SubscriptionItem.php Outdated Show resolved Hide resolved
src/SubscriptionItem.php Outdated Show resolved Hide resolved
@driesvints
Copy link
Member

@Bark-fa sorry, needed a bit to have a thorough look here. I completely refactored all of this to be on a new trait on the billable. It's not needed for usage based billing to live closely to a subscription or subscription item. I've revised the API's and simplified things.

The downside of this is that the written tests aren't applicable anymore. Could you rewrite those into a new UsageBasedBillingTest.php file? Then I believe this PR will be ready for review.

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 8, 2024

Done, I refactored the new tests into UsageBasedBillingTest.php and removed them from the old file.

Let me know if anything else comes up, thank you so much!

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 8, 2024

And regarding the new trait, I agree it's a better approach, I didn't allow myself the freedom to create new files though so I just wrote the functions where the old (now deprecated) code was 😅

@driesvints driesvints marked this pull request as ready for review August 9, 2024 07:56
@driesvints
Copy link
Member

Done. Thanks a bunch for this one @Bark-fa. Would also highly appreciate a PR to the docs!

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 9, 2024

Hello @driesvints, you're very much welcome, I just submitted a PR for the docs

@taylorotwell taylorotwell merged commit e93c299 into laravel:15.x Aug 13, 2024
7 checks passed
@driesvints
Copy link
Member

@Bark-fa did you try these tests? Because they're failing atm.

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 19, 2024

Hello @driesvints, what tests specifically? I tried all the tests I wrote, and the old ones as well, and all ran with success, which ones are failing?

@driesvints
Copy link
Member

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 19, 2024

I just checked, I think the problem was caused by this line:
$startTime = $options['start_time'] ?? $this->created_at->timestamp;

This used to be in the Subscription class, so the logic was that if the user did not provide a start time, we'd set it to the date the subscription was created, but when it was moved to a trait, $this references the User model, this should fix it:
$startTime = $options['start_time'] ?? 1;

I pushed it to my fork

@driesvints
Copy link
Member

Can you send in a PR? Thanks!

@Bark-fa
Copy link
Contributor Author

Bark-fa commented Aug 19, 2024

Of course, on it

driesvints added a commit that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants