-
Notifications
You must be signed in to change notification settings - Fork 316
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
PostReceiptDataOperation
: added initiationSource
parameter
#1957
Conversation
d5f0a90
to
57e2e33
Compare
PostReceiptDataOperation
: added userInitiated
parameterPostReceiptDataOperation
: added initationSource
parameter
PostReceiptDataOperation
: added initationSource
parameterPostReceiptDataOperation
: added initiationSource
parameter
56088bc
to
e48708e
Compare
This is ready (just need to fix the snapshots). |
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.
LGTM, just a naming suggestion but NABD
@@ -20,6 +20,20 @@ import StoreKit | |||
/// - SeeAlso: `Backend/post(receiptData:appUserID:isRestore:productData:...` | |||
struct ProductRequestData { | |||
|
|||
/// Determins what triggered a receipt to be posted | |||
enum InitiationSource: CaseIterable { |
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.
Hmm what do you think about ReceiptPostSource
? Feels like a better name to me but looks like renaming would require a backend change and I don't think it's a big deal.
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.
We don't have to match the backend, but I can see either way.
If we don't, we can have something more "Swifty", but I think the advantage of having this match the backend is familiarity, so this term is the same across different repos.
What do you think?
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.
Maybe another thing to realize is that this isn't just InitiationSource
, it's namespaced as ProductRequestData.InitiationSource
.
|
||
// MARK: - InitiationSource | ||
|
||
extension ProductRequestData.InitiationSource: Encodable, RawRepresentable { |
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.
Hmm I do wonder if this would be better placed in the ProductRequestData.swift
file... If it's only needed from here, this would be ok though
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.
My goal with this is to keep the type definition simpler. This is more of an implementation detail of how the source is posted to the backend.
49c5f8b
to
e7843d2
Compare
…e transactions (#2430) All StoreKit 1 transactions were being sent with `InitiationSource.purchased`. #1957 introduced this, but it was only set correctly for SK2. The consequence of this is that users might see "invalid receipt" errors when posting receipts due to updated transactions from the queue, which can only happen for `InitiationSource.purchased`.
…e transactions (#2430) All StoreKit 1 transactions were being sent with `InitiationSource.purchased`. #1957 introduced this, but it was only set correctly for SK2. The consequence of this is that users might see "invalid receipt" errors when posting receipts due to updated transactions from the queue, which can only happen for `InitiationSource.purchased`.
For TRIAGE-82.