`Offering` metadata #2498

joshdholtz · 2023-05-17T03:10:18Z

Motivation

Added metadata to Offering 😊

Description

Added metadata to Offering
- Had to use a private internal Metadata struct to make Swift compiler happy
Updated unit tests
Updated backend integration tests
Updated load shedder tests

### Changes: - Added `SnapshotTesting` to `BackendIntegrationTests` target - Verify `OfferingsResponse` content with snapshots This relies on the new serializable `OfferingsResponse` from #2495. It will make it trivial to verify the changes with the upcoming metadata in #2498.

Tests/BackendIntegrationTests/LoadShedderIntegrationTests.swift

Tests/BackendIntegrationTests/StoreKitIntegrationTests.swift

Tests/BackendIntegrationTests/Constants.swift

Sources/Purchasing/Offering.swift

Tests/UnitTests/Networking/Responses/Fixtures/OfferingsWithMetadata.json

### Changes: - Added `SnapshotTesting` to `BackendIntegrationTests` target - Verify `OfferingsResponse` content with snapshots This relies on the new serializable `OfferingsResponse` from #2495. It will make it trivial to verify the changes with the upcoming metadata in #2498.

codecov · 2023-05-20T00:26:40Z

Codecov Report

Merging #2498 (ac7f805) into main (d5b9236) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2498   +/-   ##
=======================================
  Coverage   87.85%   87.85%           
=======================================
  Files         199      199           
  Lines       13655    13664    +9     
=======================================
+ Hits        11996    12005    +9     
  Misses       1659     1659

Impacted Files	Coverage Δ
...urces/Networking/Responses/OfferingsResponse.swift	`100.00% <ø> (ø)`
Sources/Purchasing/Offering.swift	`93.33% <100.00%> (+0.65%)`	⬆️
Sources/Purchasing/OfferingsFactory.swift	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

NachoSoto

I think this is ready 👍🏻 Just need to fix those potential crashes in the test, I'll send a PR on Monday with a new method on Collection so we can do that.

NachoSoto · 2023-05-20T01:12:51Z

Tests/BackendIntegrationTests/__Snapshots__/StoreKitIntegrationTests/testCanGetOfferings.1.json

@@ -4,6 +4,9 @@
    {
      "description" : "standard set of packages",
      "identifier" : "default",
+      "metadata" : {


NachoSoto · 2023-05-20T01:13:56Z

Tests/UnitTests/Networking/Responses/OfferingsDecodingTests.swift

@@ -48,10 +49,48 @@ class OfferingsDecodingTests: BaseHTTPResponseTest {
    }

    func testDecodesSecondOffering() throws {
-        let offering = try XCTUnwrap(self.response.offerings.last)
+        let offering = try XCTUnwrap(self.response.offerings[1])


This will make the test crash. I'll help you on Monday, we'll need to define a new method that returns an Optional for an index.

NachoSoto · 2023-05-20T01:14:00Z

Tests/UnitTests/Networking/Responses/OfferingsDecodingTests.swift

+    }
+
+    func testDecodesMetadataOffering() throws {
+        let offering = try XCTUnwrap(self.response.offerings[2])


NachoSoto · 2023-05-20T01:14:23Z

Tests/UnitTests/Networking/Responses/OfferingsDecodingTests.swift

+        expect(offering.metadata) == [:]
+        expect(offering.packages).to(haveCount(1))
+
+        let package = offering.packages[0]


To avoid the test crashing (instead of failing):

Suggested change

let package = offering.packages[0]

let package = try XCTUnwrap(offering.packages.onlyElement)

NachoSoto · 2023-05-20T01:14:45Z

Tests/UnitTests/Networking/Responses/OfferingsDecodingTests.swift

+        ]
+        expect(offering.packages).to(haveCount(1))
+
+        let package = offering.packages[0]


Ditto:

Suggested change

let package = offering.packages[0]

try XCTUnwrap(offering.packages.onlyElement)

NachoSoto · 2023-05-20T01:14:55Z

Tests/UnitTests/Networking/Responses/OfferingsDecodingTests.swift

+    }
+
+    func testDecodesNullMetadataOffering() throws {
+        let offering = try XCTUnwrap(self.response.offerings[3])


joshdholtz · 2023-05-22T02:12:21Z

@NachoSoto I think I fixed all the safety things that could crash the tests! But let me know if you want me to do it a different way or move code around 😇

NachoSoto

This is ready :) just one more suggested test.

NachoSoto · 2023-05-22T02:37:11Z

Tests/UnitTests/Networking/Responses/OfferingsDecodingTests.swift

+private extension Collection {
+
+    /// Returns the element at the specified index if it exists, otherwise nil.
+    subscript (safe index: Index) -> Element? {


Cool this is exactly what I had in mind 👍🏻
I can extract this an add unit tests for it in a separate PR since I think it's useful to have.

NachoSoto · 2023-05-22T02:43:47Z

Tests/UnitTests/Networking/Responses/OfferingsDecodingTests.swift

+        expect(offering.metadata) == [:]
+        expect(offering.packages).to(haveCount(1))
+
+        let package = try XCTUnwrap(offering.packages[safe: 0])


I'd use first here which is the same:

Suggested change

let package = try XCTUnwrap(offering.packages[safe: 0])

let package = try XCTUnwrap(offering.packages.first)

NachoSoto · 2023-05-22T02:47:45Z

Tests/UnitTests/Purchasing/OfferingsManagerTests.swift

@@ -495,6 +495,7 @@ private extension OfferingsManagerTests {
                    Offering(
                        identifier: offering.identifier,
                        serverDescription: offering.description,
+                        metadata: [:],


The other tests only check OfferingsResponse. Let's add some metadata here so you can check in another test in this file that they get copied correctly to Offering (basically thatoffering.metadata.mapValues(\.asAny)).

NachoSoto · 2023-05-22T20:41:49Z

Tests/UnitTests/Purchasing/OfferingsManagerTests.swift

        static let anyBackendOfferingsResponse: OfferingsResponse = .init(
            currentOfferingId: "base",
            offerings: [
                .init(identifier: "base",
                      description: "This is the base offering",
                      packages: [
                        .init(identifier: "$rc_monthly", platformProductIdentifier: "monthly_freetrial")
-                      ])
+                      ],
+                      metadata: _metadata)


Nit per our style guide:

Suggested change

metadata: _metadata)

metadata: Self.metadata)

NachoSoto · 2023-05-22T20:42:03Z

Tests/UnitTests/Purchasing/OfferingsManagerTests.swift

@@ -465,14 +466,27 @@ private extension OfferingsManagerTests {
    enum MockData {
        static let anyAppUserID = ""

+        @DefaultDecodable.EmptyDictionary


Hmm this shouldn't be here since this type isn't Decodable.

NachoSoto · 2023-05-23T14:36:43Z

Tests/UnitTests/Purchasing/OfferingsTests.swift

@@ -153,6 +153,55 @@ class OfferingsTests: TestCase {
        expect(offerings.current) == offerings["offering_a"]
    }

+    func testOfferingsWithMetadataIsCreated() throws {


tonidero · 2023-05-23T16:11:26Z

Sources/Purchasing/Offering.swift

+extension Offering {
+
+    struct Metadata {
+        let data: [String: Any]


I wonder, if we want to force developers to always have a default value, should we hide the raw data and provide a method to access values from the metadata that requires users to put a default value? Something like func getValue(key: String, defaultValue: Any)

Yeah, or maybe as an additional API 👍🏻

extension Offering { func getMetadataValue<T>(for key: String, defaultValue: T) -> T { return self.metadata.data[key] as? T ?? defaultValue } }

This would be Swift only, but I think it would be useful.

Also, we might want to have different accessors so people don't need to handle Any. Things like getStringValue, getIntValue, getBoolValue,... Not sure if we want to support different types though!

That's not very Swifty.
For Obj-C the current API works well.
For Swift I'd go with my suggested generic API.

Do we want getMetadataValue() on Offering? Or should we make the Metadata object public with a subscript thing?

Something like this on Metadata... 🤷‍♂️

public subscript<T>(key: String, defaultValue defaultValue: T) -> T { return self.data[key] as? T ?? defaultValue }

Metadata is a struct therefore it wouldn't be available to ObjC, so it needs to be on Offering.

Sources/Purchasing/Offering.swift

NachoSoto

Marking this as request changes since I want to look at the final version with the pending changes :)

Sources/Purchasing/Offering.swift

NachoSoto

Almost ready! Just some nit-picks about the API.

NachoSoto · 2023-05-23T19:24:01Z

Sources/Purchasing/Offering.swift

+extension Offering {
+
+    /**
+     Gets a value from `metadata` and falls back to a specified default value.


Maybe:

Suggested change

Gets a value from `metadata` and falls back to a specified default value.

- Returns: the `metadata` value associated to `key` for the expected type,

or `defaultValue` if not found, or it's not the expected type.

NachoSoto · 2023-05-23T19:24:59Z

Sources/Purchasing/Offering.swift

+    /**
+     Gets a value from `metadata` and falls back to a specified default value.
+     */
+    public func getMetadataValue<T>(for key: String, defaultValue: T) -> T {


Being nit-picky since this is public so we want to get it right from the beginning.
Maybe:

Suggested change

public func getMetadataValue<T>(for key: String, defaultValue: T) -> T {

public func getMetadataValue<T>(for key: String, default: T) -> T {

Swift guidelines require avoiding that alliteration.

NachoSoto · 2023-05-23T19:26:03Z

Tests/APITesters/SwiftAPITester/SwiftAPITester/OfferingAPI.swift

+    let metadataString: String = off.getMetadataValue(for: "", defaultValue: "")
+    let metadataInt: Int = off.getMetadataValue(for: "", defaultValue: 0)


I know you were following the pattern here, but it's leftover from the first implementation which was kind of messy.
We should get rid of all these print calls.

To avoid increasing the noise, I'd change these to:

Suggested change

let metadataString: String = off.getMetadataValue(for: "", defaultValue: "")

let metadataInt: Int = off.getMetadataValue(for: "", defaultValue: 0)

let _: String = off.getMetadataValue(for: "", defaultValue: "")

let _: Int = off.getMetadataValue(for: "", defaultValue: 0)

NachoSoto · 2023-05-23T19:26:35Z

Tests/UnitTests/Purchasing/OfferingsTests.swift

+        let offeringA = try XCTUnwrap(offerings["offering_a"])
+        expect(offeringA.metadata.count) == metadata.count


Nit: I'd change this to haveCount, otherwise the test is great 👍🏻

NachoSoto · 2023-05-23T19:27:07Z

Tests/UnitTests/Purchasing/OfferingsTests.swift

+        expect(offeringA.getMetadataValue(for: "int", defaultValue: 0)) == 5
+        expect(offeringA.getMetadataValue(for: "double", defaultValue: 0.0)) == 5.5
+        expect(offeringA.getMetadataValue(for: "boolean", defaultValue: false)) == true
+        expect(offeringA.getMetadataValue(for: "string", defaultValue: "")) == "five"


Can you add tests for not finding the key, and also having the wrong type?

NachoSoto

Almost ready! Just some nit-picks about the API.

NachoSoto · 2023-05-23T19:27:48Z

Tests/UnitTests/Purchasing/OfferingsTests.swift

@@ -153,6 +153,59 @@ class OfferingsTests: TestCase {
        expect(offerings.current) == offerings["offering_a"]
    }

+    func testOfferingsWithMetadataIsCreated() throws {
+        @DefaultDecodable.EmptyDictionary


Also reminder to remove this

I think this is needed though 🤔 I couldn't get it to compile without that 😬

NachoSoto

Ship it!

tonidero

Looks good!

tonidero · 2023-05-24T09:29:24Z

Sources/Purchasing/Offering.swift

+     - Returns: the `metadata` value associated to `key` for the expected type,
+     or `defaultValue` if not found, or it's not the expected type.
+     */
+    public func getMetadataValue<T>(for key: String, default: T) -> T {


Would this also support null default values? Like let metadataInt: Int? = offering.getMetadataValue(for: "key", default: nil). I think that could be useful.

It should be let me write another test for that! 😊

Yup, this works!

let optionalInt: Int? = offeringA.getMetadataValue(for: "optionalInt", default: nil) expect(optionalInt).to(beNil())

### Changes: - Added `SnapshotTesting` to `BackendIntegrationTests` target - Verify `OfferingsResponse` content with snapshots This relies on the new serializable `OfferingsResponse` from #2495. It will make it trivial to verify the changes with the upcoming metadata in #2498.

Added `metadata` to `Offering` 😊 - Added `metadata` to `Offering` - Had to use a private internal `Metadata` struct to make Swift compiler happy - Updated unit tests - Updated backend integration tests - Updated load shedder tests

NachoSoto mentioned this pull request May 17, 2023

Integration Tests: added snapshot test for OfferingsResponse #2499

Merged

NachoSoto reviewed May 17, 2023

View reviewed changes

Tests/BackendIntegrationTests/LoadShedderIntegrationTests.swift Outdated Show resolved Hide resolved

NachoSoto reviewed May 17, 2023

View reviewed changes

Tests/BackendIntegrationTests/StoreKitIntegrationTests.swift Outdated Show resolved Hide resolved

NachoSoto reviewed May 17, 2023

View reviewed changes

Tests/BackendIntegrationTests/Constants.swift Outdated Show resolved Hide resolved

Sources/Purchasing/Offering.swift Outdated Show resolved Hide resolved

Tests/UnitTests/Networking/Responses/Fixtures/OfferingsWithMetadata.json Outdated Show resolved Hide resolved

joshdholtz force-pushed the offering-metadata branch from 6aaac9b to a3ef198 Compare May 19, 2023 20:58

joshdholtz added the pr:feat A new feature label May 19, 2023

joshdholtz added 7 commits May 19, 2023 19:08

Initial commit

a67a526

Need to make metadata public

0fdd5b7

Fixes

0dbe39b

Fixed some lint and compile errors

1d41b39

Tests for metadata decoding

1b6932f

Fixed integration tests

c8ba355

Updated integration test

8006a72

joshdholtz force-pushed the offering-metadata branch from 9c1cbfd to 8006a72 Compare May 20, 2023 00:08

NachoSoto reviewed May 20, 2023

View reviewed changes

joshdholtz added 2 commits May 21, 2023 20:46

Updated offerings json for load shedder

c443678

Prevents crashing in tests

8b9ae7e

joshdholtz requested a review from NachoSoto May 22, 2023 02:11

NachoSoto reviewed May 22, 2023

View reviewed changes

Last of test fixes... hopefully :)

e1068bc

joshdholtz requested a review from NachoSoto May 22, 2023 20:25

joshdholtz changed the title ~~[WIP] Offering metadata~~ Offering metadata May 22, 2023

NachoSoto approved these changes May 22, 2023

View reviewed changes

joshdholtz added 2 commits May 22, 2023 15:44

Failing tests from disk cache

6261093

Adding more metadata tests

f0a0ad6

NachoSoto reviewed May 23, 2023

View reviewed changes

tonidero reviewed May 23, 2023

View reviewed changes

NachoSoto reviewed May 23, 2023

View reviewed changes

Sources/Purchasing/Offering.swift Show resolved Hide resolved

NachoSoto suggested changes May 23, 2023

View reviewed changes

Sources/Purchasing/Offering.swift Show resolved Hide resolved

joshdholtz added 4 commits May 23, 2023 14:47

Added API tests and new getMetadataValue method

d6ec387

Making the metadata thing private

59ec195

LINT

ae17857

Docs, public method, and adding to API tester

abc4078

joshdholtz marked this pull request as ready for review May 23, 2023 19:00

joshdholtz requested a review from NachoSoto May 23, 2023 19:13

NachoSoto changed the title ~~Offering metadata~~ Offering metadata May 23, 2023

NachoSoto reviewed May 23, 2023

View reviewed changes

joshdholtz added 3 commits May 23, 2023 16:45

More tests and fixed bug

3d6d516

Oops, forgot a file

206f3e6

Take that wrapped value yo

305964f

joshdholtz requested a review from NachoSoto May 23, 2023 21:27

this is the one

62c24d3

NachoSoto approved these changes May 23, 2023

View reviewed changes

tonidero approved these changes May 24, 2023

View reviewed changes

Added test for optional default value

ac7f805

joshdholtz merged commit 7fe13aa into main May 24, 2023

joshdholtz deleted the offering-metadata branch May 24, 2023 17:06

RCGitBot mentioned this pull request May 24, 2023

[AUTOMATIC] Release/4.20.0 #2537

Closed

NachoSoto mentioned this pull request May 25, 2023

[DO NOT MERGE] Release/4.20.0 #2543

Closed

This was referenced May 31, 2023

[AUTOMATIC] Release/4.21.0 #2569

Closed

[AUTOMATIC] Release/4.21.0 #2571

Merged

	let package = offering.packages[0]
	let package = try XCTUnwrap(offering.packages.onlyElement)

	let package = offering.packages[0]
	try XCTUnwrap(offering.packages.onlyElement)

	Gets a value from `metadata` and falls back to a specified default value.
	- Returns: the `metadata` value associated to `key` for the expected type,
	or `defaultValue` if not found, or it's not the expected type.

	public func getMetadataValue<T>(for key: String, defaultValue: T) -> T {
	public func getMetadataValue<T>(for key: String, default: T) -> T {

		let metadataString: String = off.getMetadataValue(for: "", defaultValue: "")
		let metadataInt: Int = off.getMetadataValue(for: "", defaultValue: 0)

		let offeringA = try XCTUnwrap(offerings["offering_a"])
		expect(offeringA.metadata.count) == metadata.count

Offering metadata #2498

Offering metadata #2498

Conversation

joshdholtz commented May 17, 2023 • edited Loading

Motivation

Description

codecov bot commented May 20, 2023 • edited Loading

Codecov Report

NachoSoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdholtz commented May 22, 2023

NachoSoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NachoSoto May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NachoSoto left a comment

Choose a reason for hiding this comment

NachoSoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NachoSoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NachoSoto left a comment

Choose a reason for hiding this comment

tonidero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`Offering` metadata #2498

`Offering` metadata #2498

joshdholtz commented May 17, 2023 •

edited

Loading

codecov bot commented May 20, 2023 •

edited

Loading

NachoSoto May 23, 2023 •

edited

Loading