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

New Package.presentedOfferingContext #3690

Merged
merged 15 commits into from
Feb 14, 2024

Conversation

joshdholtz
Copy link
Member

@joshdholtz joshdholtz commented Feb 14, 2024

Motivation

Allow for storing of other presented offering context on a Package

Description

  • Replaced offeringIdentifier: String on Package with new presentedOfferingContext: PresentedOfferingContext
    • Currently it only has a offeringIdentifier: String
    • Eventually this will add new optional properties for targeting and placements

@joshdholtz joshdholtz force-pushed the move-presented_offering_identifier-to-object branch 2 times, most recently from 7984c51 to 7d1d7ce Compare February 14, 2024 03:55
@joshdholtz joshdholtz changed the title [WIP] Move presented_offering_identifier to an object [WIP] Refactor presented_offering_identifier to PresentedOfferingContext Feb 14, 2024
super.init()
}

public override func isEqual(_ object: Any?) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you override this you also need to override hash or else bad things will happen when using these in maps and sets.

Sources/Purchasing/Package.swift Show resolved Hide resolved
Sources/Purchasing/Package.swift Outdated Show resolved Hide resolved
Sources/Purchasing/Package.swift Outdated Show resolved Hide resolved
Sources/Purchasing/Package.swift Outdated Show resolved Hide resolved
Sources/Purchasing/Purchases/Purchases.swift Outdated Show resolved Hide resolved
Sources/Purchasing/Purchases/PurchasesOrchestrator.swift Outdated Show resolved Hide resolved
@joshdholtz joshdholtz changed the title [WIP] Refactor presented_offering_identifier to PresentedOfferingContext Refactor presented_offering_identifier to PresentedOfferingContext Feb 14, 2024
@joshdholtz joshdholtz marked this pull request as ready for review February 14, 2024 17:52
Copy link
Contributor

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

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

Just one last missing piece: api tests.
Also I'd change this to feat since it's adding a public API.

///
/// Stores information about how a ``Package`` was presented.
///
@objc(RCPresentedOfferingContext) public final class PresentedOfferingContext: NSObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this to the Swift/Obj-C API testers?

Copy link
Member Author

Choose a reason for hiding this comment

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

OMG, how did I forget...

Purchases.shared.cachePresentedOfferingIdentifier(
offering.identifier,
Purchases.shared.cachePresentedOfferingContext(
PresentedOfferingContext(offeringIdentifier: offering.identifier),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I like to do this to avoid all the duplicated "offering context"

Suggested change
PresentedOfferingContext(offeringIdentifier: offering.identifier),
.init(offeringIdentifier: offering.identifier),

@NachoSoto
Copy link
Contributor

Maybe rename the PR to

New Package.presentedOfferingContext

So it's a clearer callout in the release notes, since it's more than a refactor.

@joshdholtz joshdholtz changed the title Refactor presented_offering_identifier to PresentedOfferingContext New Package.presentedOfferingContext Feb 14, 2024
@joshdholtz joshdholtz added pr:feat A new feature and removed refactor do not merge labels Feb 14, 2024
@implementation RCPresentOfferingContextAPI

+ (void)checkAPI {
RCPresentedOfferingContext *poc;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't used?

Comment on lines 12 to 16
private var context: PresentedOfferingContext!
func checkPresentedOfferingContextAPI() {
let oID: String = context.offeringIdentifier

print(context!, oID)
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been simplifying these:

Suggested change
private var context: PresentedOfferingContext!
func checkPresentedOfferingContextAPI() {
let oID: String = context.offeringIdentifier
print(context!, oID)
func checkPresentedOfferingContextAPI(context: PresentedOfferingContext! = nil) {
let _: String = context.offeringIdentifier
}

@NachoSoto
Copy link
Contributor

Do you mind rebasing? I just rebased 5.0-dev from main.

@joshdholtz joshdholtz force-pushed the move-presented_offering_identifier-to-object branch from 477ade0 to b884022 Compare February 14, 2024 19:19
@joshdholtz joshdholtz merged commit cf39db2 into 5.0-dev Feb 14, 2024
24 checks passed
@joshdholtz joshdholtz deleted the move-presented_offering_identifier-to-object branch February 14, 2024 22:03
MarkVillacampa pushed a commit that referenced this pull request Feb 21, 2024
### Motivation

Allow for storing of other presented offering context on a `Package`

### Description

- Replaced `offeringIdentifier: String` on `Package` with new
`presentedOfferingContext: PresentedOfferingContext`
  - Currently it only has a `offeringIdentifier: String`
- Eventually this will add new optional properties for targeting and
placements
joshdholtz added a commit that referenced this pull request Feb 23, 2024
### Motivation

Allow for storing of other presented offering context on a `Package`

### Description

- Replaced `offeringIdentifier: String` on `Package` with new
`presentedOfferingContext: PresentedOfferingContext`
  - Currently it only has a `offeringIdentifier: String`
- Eventually this will add new optional properties for targeting and
placements
joshdholtz added a commit that referenced this pull request Feb 28, 2024
This was already approved and merged into `5.0-dev` but bringing this
into `4.x` (see #3690)

### Motivation

Allow for storing of other presented offering context on a `Package`

### Description

- Replaced `offeringIdentifier: String` on `Package` with new
`presentedOfferingContext: PresentedOfferingContext`
  - Currently it only has a `offeringIdentifier: String`
- Eventually this will add new optional properties for targeting and
placements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:feat A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants