feat(payments-next): Add eligibility check for free trials#20190
feat(payments-next): Add eligibility check for free trials#20190david1alvarez wants to merge 1 commit intomainfrom
Conversation
david1alvarez
force-pushed
the
PAY-3554
branch
3 times, most recently
from
47629bd to
d05fc34
Compare
|
|
||
| export class FreeTrialConfig { | ||
| @IsString() | ||
| public readonly collectionName!: string; |
There was a problem hiding this comment.
Can we rename this to firestoreCollectionName
| return this.firestore.collection(this.config.collectionName); | ||
| } | ||
|
|
||
| async getRecord( |
There was a problem hiding this comment.
This should be removed as a method (the repository handles this type of operation)
| return getFreeTrialRecordData(this.collectionRef, uid, freeTrialConfigId); | ||
| } | ||
|
|
||
| async upsertRecord(uid: string, freeTrialConfigId: string): Promise<void> { |
There was a problem hiding this comment.
This should be named something more akin to "recordFreeTrial" or "consumeFreeTrialForUser" or "startFreeTrialForUid". The way this is named currently is more akin to a repository record, rather than a manager method.
There was a problem hiding this comment.
I like "recordFreeTrial" for this.
| }); | ||
| } | ||
|
|
||
| async isCooldownElapsed( |
There was a problem hiding this comment.
Should this be named something more akin to isEligible since this doesn't throw when the user has no cooldown at all?
There was a problem hiding this comment.
isEligible I think is not specific enough, as this really only checks against the firestore records and the users free trial eligibility is more complex than that. Maybe getFirestoreEligibility? We could also invert it, and call it usedRecently or isBlockedByCooldown.
There was a problem hiding this comment.
isBlockedByCooldown sounds perfect to me!
| const existingRecord = await db | ||
| .where('uid', '==', data.uid) | ||
| .where('freeTrialConfigId', '==', data.freeTrialConfigId) | ||
| .limit(1) | ||
| .get(); | ||
|
|
||
| if (!existingRecord.empty) { | ||
| const doc = existingRecord.docs[0]; | ||
| await doc.ref.update({ startedAt: data.startedAt }); | ||
| } else { | ||
| await db.add({ | ||
| uid: data.uid, | ||
| freeTrialConfigId: data.freeTrialConfigId, | ||
| startedAt: data.startedAt, | ||
| }); |
There was a problem hiding this comment.
This upsert is vulnerable to a race condition between reading the existing record and adding a new record. If two calls for upsertFreeTrialRecord occur in reasonably quick succession (within ~70ms of each other in prod, given our Firestore latency), a duplicate will be created.
There was a problem hiding this comment.
Looking into it more, it seems like Firestore doesn't really seem to have much in the way of preventative measures here. The main option I see is to plan around there being multiple records, and only serving up the latest record when queried. Firestore should be able to handle the number of records we'd be creating this way.
I'm planning to refactor this into getLatestFreeTrialRecordData and createFreeTrialRecord. Any suggestions for other approaches?
Because: * Free trial eligibility is a complex check that requires data from several locations This commit: * Adds in the free trial repository and manager to handle requests for free trial records * Adds an eligibility checking method to the checkout service * Adds a free trial firestore collection * Adds in configs and tests to support the changes * Updates a test that was outdated and failing Closes #PAY-3554
2 participants
Because:
This commit:
Closes #PAY-3554
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.