Skip to content
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

Add currentOffering(forPlacement: String) to Offerings #3707

Merged

Conversation

guido732
Copy link
Contributor

@guido732 guido732 commented Feb 22, 2024

Motivation

New function to get Offering by a placement identifier

Description

New currentOffering(forPlacement: String): Offering? method on Offerings

  • Added demo button for this in purchase tester app
// Example usage
if let offering = offerings.currentOffering(forPlacement: "onboarding") {
  showPaywall(offering)
}

Logic flow

  • If a placement identifier explicitly exists within the placements.offeringIdsByPlacement dictionary, the method attempts to fetch the corresponding Offering without resorting to a fallback
    • Example: when there is a nil / null value for a placement, we don't want to use the fallback
  • If the placement identifier does not exist, the method then attempts to fetch a fallback offering defined by placements.fallbackOfferingId

New placements object in offerings response

{
    // other offerings stuff
    "placements": {
        "fallback_offering_id": "offering_a", // can be null
        "offering_ids_by_placement": {
            "onboarding_start": null,
            "onboarding_end": "offering_b",
        }
  }
}

@guido732 guido732 self-assigned this Feb 22, 2024
@guido732 guido732 added the feat A new feature label Feb 22, 2024
@joshdholtz joshdholtz changed the title Guido/fia 2856 rework ios add getcurrentoffering to offerings Add getCurrentOffering(forPlacement: String) to Offerings Feb 23, 2024
@joshdholtz joshdholtz force-pushed the guido/fia-2856-rework-ios-add-getcurrentoffering-to-offerings branch from 860e177 to d744585 Compare February 23, 2024 18:03
@joshdholtz joshdholtz changed the base branch from 5.0-dev to new-presented-offering-context February 23, 2024 18:03
@joshdholtz joshdholtz requested review from a team February 27, 2024 15:41
Comment on lines +110 to +131
let returnOffering: Offering?
if let explicitOfferingId: String? = placements.offeringIdsByPlacement[placementIdentifier] {
// Don't use fallback since placement id was explicity set in the dictionary
returnOffering = explicitOfferingId.flatMap { self.all[$0] }
} else {
// Use fallback since the placement didn't exist
returnOffering = placements.fallbackOfferingId.flatMap { self.all[$0]}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placements.offeringIdsByPlacement[placementIdentifier] returns at Offering?? because placements.offeringIdsByPlacement is a [String: String?]

The result of could be an explicit nil value. In that case, we don't want to use the fallback offering.

If placements.offeringIdsByPlacement[placementIdentifier] is not found and goes into the else, it means no key was found and we should use the fallback offering.

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a meeting, will continue reviewing after

Sources/Purchasing/Placements.swift Outdated Show resolved Hide resolved
return Package(identifier: pkg.identifier,
packageType: pkg.packageType,
storeProduct: pkg.storeProduct,
presentedOfferingContext: newContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but should we have an internal copy constructor from package so we can create a new package with a new context without having to pass all the parameters again?

Sources/Purchasing/Purchases/PurchasesOrchestrator.swift Outdated Show resolved Hide resolved
@joshdholtz joshdholtz force-pushed the guido/fia-2856-rework-ios-add-getcurrentoffering-to-offerings branch from 207e09d to 25762bf Compare February 27, 2024 16:54
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some non-blocking comments. Looks good!

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking great, left a couple of questions

Comment on lines 118 to 119
@objc(getCurrentOfferingForPlacement:)
func getCurrentOffering(forPlacement placementIdentifier: String) -> Offering? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it more swift-style to make this (for placement placementIdentifier: String) instead?
so the callsite is

getCurrentOffering(for: "HomePageBanner")

instead of

getCurrentOffering(forPlacement: "HomePageBanner")

or even

currentOffering(for: "HomePageBanner")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we've historically reserved verbs like get or fetch as ways to indicate that it'll issue some work like network requests

@@ -19,18 +19,31 @@ import Foundation
///
@objc(RCPresentedOfferingContext) public final class PresentedOfferingContext: NSObject {

/// The identifier of the ``Offering`` containing this Package.
/// The identifier of the ``Offering`` containing this ``Package``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm late to the game here, but... what do I do with this class as a developer? Is it so I can use it for analytics?
The docstring for the class doesn't really provide any context on it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aboedo Oops, missed this comment from yesterday! This is mainly for internal usage. Instead of passing offeringIdentifier from get offerings to post receipt, this objects gets passed around instead. It holds offering identifier, placement information, and targeting information.

Base automatically changed from new-presented-offering-context to main February 28, 2024 19:53
joshdholtz and others added 16 commits February 28, 2024 13:57
@joshdholtz joshdholtz force-pushed the guido/fia-2856-rework-ios-add-getcurrentoffering-to-offerings branch from 04abfe4 to 3eb9898 Compare February 28, 2024 20:01
@joshdholtz joshdholtz changed the title Add getCurrentOffering(forPlacement: String) to Offerings Add currentOffering(forPlacement: String) to Offerings Feb 28, 2024
@joshdholtz joshdholtz merged commit 61d66bb into main Feb 28, 2024
24 checks passed
@joshdholtz joshdholtz deleted the guido/fia-2856-rework-ios-add-getcurrentoffering-to-offerings branch February 28, 2024 21:11
joshdholtz added a commit that referenced this pull request Mar 5, 2024
**This is an automatic release.**

### New Features
* Paywalls: add `updateWithDisplayCloseButton` to
`PaywallViewController` (#3708) via Cesar de la Vega (@vegaro)
* New `syncAttributesAndOfferingsIfNeeded` method (#3709) via Burdock
(@lburdock)
* Add targeting to `PresentedOfferingContext` (#3730) via Josh Holtz
(@joshdholtz)
* Add `currentOffering(forPlacement: String)` to `Offerings` (#3707) via
Guido Torres (@guido732)
* New `Package.presentedOfferingContext` (#3712) via Josh Holtz
(@joshdholtz)
### Bugfixes
* Mark methods with StaticString for appUserID as deprecated (#3739) via
Mark Villacampa (@MarkVillacampa)
### Other Changes
* [EXTERNAL] Spelling typo fix to comment (#3713) via @vdeaugustine
(#3740) via Mark Villacampa (@MarkVillacampa)

---------

Co-authored-by: Josh Holtz <me@joshholtz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants