-
Notifications
You must be signed in to change notification settings - Fork 5
Sweetmantech/myc 3463 api apiartistspro lib for getting all account id from stripe #174
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
Sweetmantech/myc 3463 api apiartistspro lib for getting all account id from stripe #174
Conversation
…ubscriptions associated with Pro accounts
WalkthroughThe PR refactors email-fetching logic across multiple layers to support parallel queries and unified subscriber-enterprise email retrieval. It introduces a new module for fetching subscriber account emails, updates the Stripe subscription handler to support fetching all subscriptions, and modifies the main artists handler to merge both email sources before returning results. Changes
Sequence DiagramsequenceDiagram
participant Handler as getArtistsProHandler
participant Enterprise as getAllEnterpriseAccounts
participant Subscribers as getSubscriberAccountEmails
participant Stripe as Stripe API
participant DB as Database
Handler->>+Enterprise: fetch enterprise emails
Handler->>+Subscribers: fetch subscriber emails
rect rgb(230, 245, 255)
Note over Enterprise: Parallel queries per domain
Enterprise->>+DB: getAccountEmails({queryEmail: `@domain1`})
Enterprise->>+DB: getAccountEmails({queryEmail: `@domain2`})
DB-->>Enterprise: emails
DB-->>Enterprise: emails
Enterprise->>Enterprise: flatten results
end
rect rgb(230, 245, 255)
Note over Subscribers: Extract account IDs from subscriptions
Subscribers->>+Stripe: getActiveSubscriptions()
Stripe-->>Subscribers: subscriptions[]
Subscribers->>+DB: getAccountEmails({account_ids: [...]})
DB-->>Subscribers: subscriber emails
end
Enterprise-->>-Handler: enterprise emails[]
Subscribers-->>-Handler: subscriber emails[]
Handler->>Handler: merge both arrays
Handler-->>Handler: return merged artists
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/stripe/getActiveSubscriptions.ts (1)
6-11: Handle Stripe API pagination to avoid missing subscriptions beyond the 100-item limit.The hardcoded
limit: 100means only the first 100 active subscriptions are retrieved. If your platform has more than 100 active subscriptions, the overflow will be silently ignored, leading to incomplete results ingetSubscriberAccountEmailsand ultimately missing artists in the Pro API response.Apply this diff to implement pagination:
export const getActiveSubscriptions = async (accountId?: string) => { try { + const allSubscriptions: Stripe.Subscription[] = []; + let hasMore = true; + let startingAfter: string | undefined; + + while (hasMore) { - const subscriptions = await stripeClient.subscriptions.list({ - limit: 100, - current_period_end: { - gt: parseInt(Number(Date.now() / 1000).toFixed(0), 10), - }, - }); + const subscriptions = await stripeClient.subscriptions.list({ + limit: 100, + current_period_end: { + gt: parseInt(Number(Date.now() / 1000).toFixed(0), 10), + }, + ...(startingAfter && { starting_after: startingAfter }), + }); + + allSubscriptions.push(...subscriptions.data); + hasMore = subscriptions.has_more; + if (hasMore && subscriptions.data.length > 0) { + startingAfter = subscriptions.data[subscriptions.data.length - 1].id; + } + } // Return all active subscriptions if no accountId provided if (!accountId) { - return subscriptions?.data || []; + return allSubscriptions; } // Filter by accountId if provided - const activeSubscriptions = subscriptions?.data?.filter( + const activeSubscriptions = allSubscriptions.filter( (subscription: Stripe.Subscription) => subscription.metadata?.accountId === accountId ); return activeSubscriptions || [];
🧹 Nitpick comments (2)
lib/enterprise/getAllEnterpriseAccounts.ts (1)
12-20: Good parallelization, consider adding resilience for partial failures.The refactoring from sequential to parallel queries improves performance. However,
Promise.all()will fail the entire operation if any single domain query fails. Consider whether partial results would be acceptable in case of individual query failures.If partial results are acceptable, consider using
Promise.allSettled()instead:- const emailArrays = await Promise.all(emailPromises); + const results = await Promise.allSettled(emailPromises); + const emailArrays = results + .filter((result): result is PromiseFulfilledResult<AccountEmail[]> => result.status === 'fulfilled') + .map(result => result.value); + + // Log any failures + results.forEach((result, index) => { + if (result.status === 'rejected') { + console.error(`Failed to fetch emails for domain ${Array.from(ENTERPRISE_DOMAINS)[index]}:`, result.reason); + } + });controllers/ArtistsProController/getArtistsProHandler.ts (1)
19-23: Consider deduplicating account emails when merging enterprise and subscriber lists.If an enterprise domain account also has an active subscription, it will appear twice in the
artistsarray. Depending on your use case, this may or may not be desired behavior.If deduplication is needed, apply this diff:
- const artists = [...allEnterpriseEmails, ...subscriptionAccountEmails]; + // Merge and deduplicate by account_id + const artistsMap = new Map(); + [...allEnterpriseEmails, ...subscriptionAccountEmails].forEach(email => { + artistsMap.set(email.account_id, email); + }); + const artists = Array.from(artistsMap.values());Alternatively, deduplicate by a combination of
account_idand
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
controllers/ArtistsProController/getArtistsProHandler.ts(1 hunks)controllers/SubscriptionsController.ts(1 hunks)lib/enterprise/getAllEnterpriseAccounts.ts(1 hunks)lib/stripe/getActiveSubscriptions.ts(2 hunks)lib/stripe/getSubscriberAccountEmails.ts(1 hunks)lib/supabase/account_emails/getAccountEmails.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
lib/enterprise/getAllEnterpriseAccounts.ts (2)
lib/consts.ts (1)
ENTERPRISE_DOMAINS(24-31)lib/supabase/account_emails/getAccountEmails.ts (1)
getAccountEmails(14-36)
lib/stripe/getSubscriberAccountEmails.ts (3)
types/database.types.ts (1)
Tables(3609-3636)lib/stripe/getActiveSubscriptions.ts (1)
getActiveSubscriptions(4-28)lib/supabase/account_emails/getAccountEmails.ts (1)
getAccountEmails(14-36)
controllers/SubscriptionsController.ts (1)
lib/supabase/account_emails/getAccountEmails.ts (1)
getAccountEmails(14-36)
controllers/ArtistsProController/getArtistsProHandler.ts (2)
lib/enterprise/getAllEnterpriseAccounts.ts (1)
getAllEnterpriseAccounts(11-21)lib/stripe/getSubscriberAccountEmails.ts (1)
getSubscriberAccountEmails(13-31)
🔇 Additional comments (7)
lib/supabase/account_emails/getAccountEmails.ts (2)
20-22: LGTM!The filtering logic correctly uses
.in()for array-based filtering and properly checks for non-empty arrays before applying the filter.
6-9: All call sites have been successfully updated for the breaking API change.Verification confirms that the transition from
account_id?: stringtoaccount_ids?: string[]has been consistently applied across allgetAccountEmails()call sites:lib/stripe/getSubscriberAccountEmails.ts,controllers/SubscriptionsController.ts, andlib/enterprise/getAllEnterpriseAccounts.tsall use the correct new API. The remainingaccount_idreferences in the codebase are database type definitions and unrelated object properties, not function parameters.controllers/SubscriptionsController.ts (1)
28-28: LGTM!The call correctly adapts to the new API signature by wrapping the single
accountIdin an array.lib/stripe/getActiveSubscriptions.ts (1)
4-16: LGTM!The optional parameter pattern is clean and allows the function to serve dual purposes: fetching all active subscriptions or filtering by a specific account.
controllers/ArtistsProController/getArtistsProHandler.ts (1)
14-17: LGTM!Parallel fetching of enterprise and subscriber emails improves performance compared to sequential execution.
lib/stripe/getSubscriberAccountEmails.ts (2)
1-31: LGTM! Clean implementation with proper type safety.The function correctly extracts account IDs from subscription metadata, filters out undefined values using a type predicate, and efficiently handles the empty case with an early return.
17-23: Good use of type predicate for filtering.The filter correctly narrows the type from
(string | undefined)[]tostring[]using a type predicate, ensuring type safety for the subsequent API call.
Summary by CodeRabbit