Add revenue split to affiliate codes v2 #4672
Conversation
fetchfern
left a comment
fetchfern
left a comment
There was a problem hiding this comment.
Good direction, would just address the comments and, overall, to strengthen the approach:
- 1<->1<->1 relationship between
charges,users_subscriptions_affiliations_payouts,payouts_values(one charge/payout value per row in usap, maybeUNIQUE (payout_value_id)?) - Handle refunded charges:
- Separate step which goes through refund charges which have a parent charge which have a usap row which have a payout_value that's not yet available and delete the usap row/payout_value
- In your current query,
JOINthe charge with its refund charges and handle the refund amount (don't forget refund charges have a many-to-1 relationship with their parent charge, since multiple refunds can be issued for 1 charge)
| continue; | ||
| } | ||
|
|
||
| let affiliate_cut = net * revenue_split; |
There was a problem hiding this comment.
charges.net includes tax amount, you'll probably want to take taxes off the cut (so (charges.net - charges.tax_amount) * revenue_split)
| let now = Utc::now(); | ||
| let start: DateTime<Utc> = DateTime::from_naive_utc_and_offset( | ||
| (now - Duration::days(1)) | ||
| .date_naive() | ||
| .and_hms_nano_opt(0, 0, 0, 0) | ||
| .unwrap_or_default(), | ||
| Utc, | ||
| ); |
There was a problem hiding this comment.
The start time (created value of the payouts_values row) should be based on the charge's last attempt time rather than the current time
| payout_info.amount += affiliate_cut; | ||
| payout_info | ||
| .charge_subscription_ids | ||
| .push((row.charge_id, row.subscription_id)); |
There was a problem hiding this comment.
I think it would make more sense to have a single payouts_values row for each unique charge? Then it's easier to handle refunds (which aren't handled here—what happens if the charge gets processed here, is added to payouts_values but is later refunded before it is available to withdraw for the affiliate?)
There was a problem hiding this comment.
I went back and forth between batching the payout value rows and inserting them individually. The payouts queue code batches project views/downloads, but I suppose since each payout value is linked to a single concrete thing here (a payout) it's better to do separately here
| INNER JOIN users_subscriptions_affiliations usa | ||
| ON c.subscription_id = usa.subscription_id | ||
| AND c.subscription_id IS NOT NULL | ||
| AND usa.deactivated_at IS NULL |
There was a problem hiding this comment.
Should be checking if c.last_attempt < usa.deactivated_at
There was a problem hiding this comment.
Replaced with
INNER JOIN users_subscriptions_affiliations usa
ON c.subscription_id = usa.subscription_id
AND c.subscription_id IS NOT NULL
AND (usa.deactivated_at IS NULL OR c.last_attempt < usa.deactivated_at)
| pub struct DBUsersSubscriptionsAffiliations { | ||
| pub subscription_id: DBUserSubscriptionId, | ||
| pub affiliate_code: DBAffiliateCodeId, | ||
| pub deactivated_at: Option<DateTime<Utc>>, |
There was a problem hiding this comment.
You should be setting deactivated_at when a subscription is cancelled in the subscription cancelled route handler
There was a problem hiding this comment.
This is done in routes/internal/billing.rs - see calls to DBUsersSubscriptionsAffiliations::deactivate. But I'm not sure if I've covered all of the places where this is set. I'd like a review to see if there are any paths I've missed for this.
There was a problem hiding this comment.
Oh i think i was looking at an outdated diff rip
There was a problem hiding this comment.
Should deactivate it here as well
code/apps/labrinth/src/queue/billing.rs
Line 792 in e38e8de
Supercedes #4549