fix(membership): address Apple IAP review feedback

Author: Innei

apps/core/src/modules/membership/membership.service.ts

@@ -27,6 +27,11 @@ const isLiveProviderSubscription = (row: MembershipRow): boolean => {
   return row.currentPeriodEnd.getTime() > Date.now()
 }
 
+const hasCurrentEntitlement = (row: MembershipRow): boolean => {
+  const status = effectiveMembershipStatus(row)
+  return status === 'active' || status === 'on_hold'
+}
+
 @Injectable()
 export class MembershipService {
   constructor(
@@ -82,31 +87,19 @@ export class MembershipService {
     )
     if (
       byReader &&
-      isLiveProviderSubscription(byReader) &&
+      hasCurrentEntitlement(byReader) &&
       byReader.provider !== 'apple'
     ) {
       return this.toStatusResult(byReader)
     }
 
     const event = appleActivatedEvent(input.decoded, input.readerId, plan)
-    const applied = await this.applyEvent({
+    await this.applyEvent({
       event,
       rawPayload: input.decoded,
       rawType: 'apple.confirm',
     })
 
-    if (!applied.applied && byReader?.provider === 'apple') {
-      await this.membershipRepository.update(byReader.id, {
-        provider: 'apple',
-        providerCustomerId:
-          input.decoded.appAccountToken ?? input.decoded.originalTransactionId,
-        providerSubscriptionId: input.decoded.originalTransactionId,
-        plan,
-        status: 'active',
-        currentPeriodEnd: new Date(input.decoded.expiresDate),
-      })
-    }
-
     return this.toStatusResult(
       await this.membershipRepository.findByReaderId(input.readerId),
     )
@@ -165,9 +158,10 @@ export class MembershipService {
       )
       if (byReader) {
         const canBindInitialSubscription =
-          byReader.provider === event.provider &&
-          byReader.providerSubscriptionId === null &&
-          event.type === 'activated'
+          event.type === 'activated' &&
+          ((byReader.provider === event.provider &&
+            byReader.providerSubscriptionId === null) ||
+            !hasCurrentEntitlement(byReader))
         if (!canBindInitialSubscription) return false
         existing = byReader
       }

apps/core/src/modules/membership/membership.types.ts

@@ -67,9 +67,15 @@ export interface AppleIapAvailability {
 
 const nonEmpty = (value?: string) => Boolean(value?.trim())
 
+const positiveInteger = (value?: string) => {
+  const parsed = Number(value?.trim())
+  return Number.isSafeInteger(parsed) && parsed > 0
+}
+
 export function resolveAppleIapAvailability(config: {
   enabled?: boolean
   appleBundleId?: string
+  appleAppAppleId?: string
   appleKeyId?: string
   appleIssuerId?: string
   applePrivateKey?: string
@@ -81,6 +87,7 @@ export function resolveAppleIapAvailability(config: {
   const enabled =
     !!config.enabled &&
     nonEmpty(config.appleBundleId) &&
+    positiveInteger(config.appleAppAppleId) &&
     nonEmpty(config.appleKeyId) &&
     nonEmpty(config.appleIssuerId) &&
     nonEmpty(config.applePrivateKey) &&

apps/core/src/modules/membership/providers/apple.provider.ts

@@ -13,6 +13,7 @@ import { APPLE_ROOT_CA_PEMS } from './apple-root-cas'
 import {
   type AppleDecodedTransaction,
   appleNotificationEventType,
+  planFromAppleProductId,
 } from './apple-transaction'
 import type {
   BillingWebhookResult,
@@ -162,13 +163,24 @@ export class AppleProvider implements PaymentProviderAdapter {
       }
     }
 
+    const monthlyProductId = membershipConfig.appleMonthlyProductId?.trim()
+    const yearlyProductId = membershipConfig.appleYearlyProductId?.trim()
+    const plan =
+      decoded.productId && monthlyProductId && yearlyProductId
+        ? (planFromAppleProductId(decoded.productId, {
+            monthlyProductId,
+            yearlyProductId,
+          }) ?? undefined)
+        : undefined
+
     return {
       event: {
         eventId: notificationUUID,
         provider: 'apple',
         type,
         customerId: decoded.appAccountToken ?? decoded.originalTransactionId,
         subscriptionId: decoded.originalTransactionId,
+        plan,
         currentPeriodEnd: new Date(decoded.expiresDate),
         readerId: '',
       },
@@ -228,7 +240,7 @@ export class AppleProvider implements PaymentProviderAdapter {
   private environmentsToTry(appleAppAppleId?: string) {
     const appAppleId = Number(appleAppAppleId)
     const production =
-      Number.isFinite(appAppleId) && appAppleId > 0
+      Number.isSafeInteger(appAppleId) && appAppleId > 0
         ? [
             {
               environment: Environment.PRODUCTION,

apps/core/test/src/modules/membership/apple.provider.spec.ts

@@ -21,4 +21,45 @@ describe('AppleProvider.createCheckout', () => {
       code: AppErrorCode.MEMBERSHIP_PROVIDER_NOT_CONFIGURED,
     })
   })
+
+  it.each([
+    ['DID_RENEW', 'renewed'],
+    ['DID_CHANGE_RENEWAL_PREF', 'plan_changed'],
+  ] as const)(
+    'maps the current Apple product during %s',
+    async (notificationType, expectedType) => {
+      get.mockResolvedValue({
+        appleAppAppleId: '1234567890',
+        appleBundleId: 'dev.yohaku.app',
+        appleMonthlyProductId: 'yohaku.membership.monthly',
+        appleYearlyProductId: 'yohaku.membership.yearly',
+      })
+      const provider = new AppleProvider({ get } as any)
+      vi.spyOn(
+        provider as any,
+        'verifyNotificationWithFallback',
+      ).mockResolvedValue({
+        data: { signedTransactionInfo: 'signed-transaction' },
+        notificationType,
+        notificationUUID: 'notification-1',
+      })
+      vi.spyOn(
+        provider as any,
+        'verifyTransactionWithFallback',
+      ).mockResolvedValue({
+        expiresDate: Date.parse('2026-09-01T00:00:00.000Z'),
+        originalTransactionId: 'original-1',
+        productId: 'yohaku.membership.yearly',
+      })
+
+      await expect(
+        provider.verifyAndParseWebhook(
+          JSON.stringify({ signedPayload: 'notification-jws' }),
+          {},
+        ),
+      ).resolves.toMatchObject({
+        event: { plan: 'yearly', type: expectedType },
+      })
+    },
+  )
 })

apps/core/test/src/modules/membership/membership.controller.e2e-spec.ts

@@ -240,6 +240,7 @@ describe('MembershipController (e2e)', () => {
     membershipConfig.enabled = true
     membershipConfig.provider = 'dodo'
     membershipConfig.webhookSigningKey = 'webhook-key'
+    delete (membershipConfig as { appleAppAppleId?: string }).appleAppAppleId
     delete (membershipConfig as { appleBundleId?: string }).appleBundleId
     delete (membershipConfig as { appleKeyId?: string }).appleKeyId
     delete (membershipConfig as { appleIssuerId?: string }).appleIssuerId
@@ -528,6 +529,7 @@ describe('MembershipController (e2e)', () => {
 
     it('reports appleIap when Apple fields are configured', async () => {
       Object.assign(membershipConfig, {
+        appleAppAppleId: '1234567890',
         appleBundleId: 'dev.yohaku.app',
         appleKeyId: 'KEYID',
         appleIssuerId: 'ISSUER',
@@ -554,6 +556,7 @@ describe('MembershipController (e2e)', () => {
   describe('POST /membership/apple/confirm', () => {
     it('returns the new apple membership for a signed-in reader', async () => {
       Object.assign(membershipConfig, {
+        appleAppAppleId: '1234567890',
         appleBundleId: 'dev.yohaku.app',
         appleKeyId: 'KEYID',
         appleIssuerId: 'ISSUER',

apps/core/test/src/modules/membership/membership.service.spec.ts

@@ -543,6 +543,98 @@ describe('MembershipService', () => {
       expect(result).toMatchObject({ provider: 'dodo', status: 'active' })
     })
 
+    it('returns an existing active manual membership without rewriting it', async () => {
+      const { service, membershipRepository } = createService()
+      const live = createMembership({
+        provider: 'manual',
+        providerCustomerId: null,
+        providerSubscriptionId: null,
+      })
+      membershipRepository.findByReaderId.mockResolvedValue(live)
+
+      const result = await service.confirmAppleTransaction({
+        decoded,
+        readerId: 'reader-1',
+        ...products,
+      })
+
+      expect(membershipRepository.create).not.toHaveBeenCalled()
+      expect(membershipRepository.update).not.toHaveBeenCalled()
+      expect(result).toMatchObject({ provider: 'manual', status: 'active' })
+    })
+
+    it('rebinds an inactive non-Apple row to the confirmed Apple subscription', async () => {
+      const { service, membershipRepository } = createService()
+      const inactive = createMembership({
+        provider: 'dodo',
+        status: 'cancelled',
+        currentPeriodEnd: new Date(now.getTime() - 1_000),
+      })
+      const rebound = createMembership({
+        provider: 'apple',
+        providerCustomerId: 'orig-apple',
+        providerSubscriptionId: 'orig-apple',
+        plan: 'monthly',
+        status: 'active',
+        currentPeriodEnd: new Date(decoded.expiresDate),
+      })
+      membershipRepository.findByReaderId
+        .mockResolvedValueOnce(inactive)
+        .mockResolvedValueOnce(inactive)
+        .mockResolvedValueOnce(rebound)
+
+      const result = await service.confirmAppleTransaction({
+        decoded,
+        readerId: 'reader-1',
+        ...products,
+      })
+
+      expect(membershipRepository.update).toHaveBeenCalledWith(
+        inactive.id,
+        expect.objectContaining({
+          plan: 'monthly',
+          provider: 'apple',
+          providerSubscriptionId: 'orig-apple',
+          status: 'active',
+        }),
+      )
+      expect(result).toMatchObject({ provider: 'apple', status: 'active' })
+    })
+
+    it('does not reactivate a cancelled Apple membership when confirmation is replayed', async () => {
+      const { service, membershipRepository, billingWebhookEventRepository } =
+        createService()
+      const cancelled = createMembership({
+        provider: 'apple',
+        providerCustomerId: 'orig-apple',
+        providerSubscriptionId: 'orig-apple',
+        status: 'cancelled',
+      })
+      membershipRepository.findByProviderSubscriptionId.mockResolvedValue(
+        cancelled,
+      )
+      membershipRepository.findByReaderId.mockResolvedValue(cancelled)
+      billingWebhookEventRepository.create.mockResolvedValue(null)
+      billingWebhookEventRepository.findByProviderAndEventId.mockResolvedValue({
+        id: 'event-apple' as any,
+        provider: 'apple',
+        eventId: decoded.transactionId,
+        type: 'apple.confirm',
+        payload: decoded,
+        processedAt: now,
+        receivedAt: now,
+      })
+
+      const result = await service.confirmAppleTransaction({
+        decoded,
+        readerId: 'reader-1',
+        ...products,
+      })
+
+      expect(membershipRepository.update).not.toHaveBeenCalled()
+      expect(result).toMatchObject({ provider: 'apple', status: 'cancelled' })
+    })
+
     it('rejects when the originalTransactionId is bound to another reader', async () => {
       const { service, membershipRepository } = createService()
       membershipRepository.findByProviderSubscriptionId.mockResolvedValue(

apps/core/test/src/modules/membership/membership.types.spec.ts

@@ -118,6 +118,7 @@ describe('resolveMembershipReturnUrl', () => {
 })
 
 const appleCredentials = {
+  appleAppAppleId: '1234567890',
   appleBundleId: 'dev.yohaku.app',
   appleKeyId: 'KEYID',
   appleIssuerId: 'ISSUER',
@@ -152,4 +153,17 @@ describe('resolveAppleIapAvailability', () => {
       }),
     ).toEqual({ enabled: false })
   })
+
+  it.each([undefined, '', 'not-a-number', '1.5', '0'])(
+    'is disabled when the App Apple ID is not a positive integer (%s)',
+    (appleAppAppleId) => {
+      expect(
+        resolveAppleIapAvailability({
+          enabled: true,
+          ...appleCredentials,
+          appleAppAppleId,
+        }),
+      ).toEqual({ enabled: false })
+    },
+  )
 })