[draft] init payments #10573
[draft] init payments #10573
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
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 |
commit: |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
This shouldn't be called auth_plans. It should live in one of the existing files. In tis case the projects.php file because each project will need to have these collections.
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('planId'), |
There was a problem hiding this comment.
No need for a planId, we should always rely on the built-in _uid (aka "$id").
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('stripePriceId'), |
There was a problem hiding this comment.
We should never tie Appwrite to a specific vendor. This is how vendor locking gets created. Instead it should be providerType, providerId or something like that. You should check if we have a similar pattern in another collection and follow it for consistency. Consistency is a huge part in creating great DX. People want to leverage knowledge they gained from using something like Appwrite Messaging and feel like Appwrite Payments works the same way.
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('stripeMeterId'), |
There was a problem hiding this comment.
Same for all mentions of Stripe in the source code.
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('interval'), |
There was a problem hiding this comment.
What unit is being used here? Let's add a comment
There was a problem hiding this comment.
Multiple intervals are supported (daily, weekly, monthly, yearly)
| 'filters' => ['json'], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('usageCap'), |
There was a problem hiding this comment.
does this include the base plan price or not?
| ], | ||
| ], | ||
| ], | ||
| 'auth_usage_events' => [ |
There was a problem hiding this comment.
Is this for measuring usage? We might better use our own usage or rely on the provider's solution all together.
There was a problem hiding this comment.
It's crucial to use both - we enforce usage caps.
Stripe's ingestion is asynchronous and does not reflect immediately, so, someone can manage to spam their way out of usage caps.
If we use our own meters, we will need to handle billing the user at the end of cycle, not a headache worth having.
It's in DB for instant updates to apps, and also sent to Stripe for billing to occur.
| 'filters' => ['datetime'], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('authSubscriptionsEnabled'), |
There was a problem hiding this comment.
Same issues with vendor locking and stripe, Imagine tomorrow a new product called UltraPayments comes in to the picture and is better than Stripe in every aspect, we need to be able to switch or support both,
| 'filters' => ['datetime'], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('authSubscriptionsEnabled'), |
There was a problem hiding this comment.
It might be easier here to follow what we do for auth and just have a single json attribute for payments containing all the configs we need.
| 'filters' => ['datetime'], | ||
| ], | ||
| [ | ||
| '$id' => ID::custom('planId'), |
There was a problem hiding this comment.
Stripe vendor locking here again. Also, we don't want to couple plans with specific users, we need to fine a solution to be able to use them for both users or teams. I would avoid changing teams and users collections and rely one the new collections to create the link, this link is probably a subscriptions collection.
There was a problem hiding this comment.
This should rely on auth, but you should avoid trying to extend /v1/account as much ass possible. It’s better for us to treat this as a totally new service. v1/payments
For v1/payments - we should imitate how new services in Appwrite are built. Look at Functions or Sites for inspiration we use a class based format to define routes unlike what we do in old controllers. We also have a test classes per route and not per service. You can use something like cursor with proper example to the agent to easily convert between the two.
We should find a way to attach a plan or payment to either a team or a user. Teams are crucial for multi-tenant saas products.
| 'optional' => false, | ||
| 'icon' => '', | ||
| ], | ||
| 'authPlans' => [ |
There was a problem hiding this comment.
This is not a service in Appwrite. As mentioned on the PR comment, we should create a new v1/payments service to centralize all these APIs.
| return $response->noContent(); | ||
| }); | ||
|
|
||
| App::get('/v1/account/subscription') |
There was a problem hiding this comment.
This is not a REST compatible API. In rest entities should be plurals.
No description provided.