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

Serializes PaywallData #1222

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Sep 6, 2023

Tests will go in a followup PR

@vegaro vegaro marked this pull request as draft September 6, 2023 08:32
@vegaro vegaro added the pr:feat A new feature label Sep 6, 2023
Base automatically changed from cesar/pwl-148-sdk-parse-offerings-response-paywall-data-and-add_2 to paywalls September 6, 2023 10:56
@vegaro vegaro force-pushed the cesar/pwl-148-sdk-parse-offerings-response-paywall-data-and-add_3 branch from 67a5905 to 9a50363 Compare September 6, 2023 10:57
@vegaro vegaro changed the title [WIP] Adds annotations for Serialization Serializes PaywallData Sep 6, 2023
@vegaro vegaro marked this pull request as ready for review September 6, 2023 10:57
// TODO-PAYWALLS: uncomment after testing
private val json = Json {
// ignoreUnknownKeys = true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will refactor this and add it as a dependency in a followup PR. Also I think until we make sure all keys are there, we should not ignore unknown keys

internal val localization: Map<String, LocalizedConfiguration>,
@SerialName("asset_base_url") @Serializable(with = URLSerializer::class) val assetBaseURL: URL,
// TODO-PAYWALLS: do we need this one?
@SerialName("default_locale") val defaultLocale: String,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

iOS doesn't have this property, but it's returned in the JSON

cc @NachoSoto

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm we should confirm with the team, but maybe we should make it optional in case it disappears?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see what they say I can fix in a follow up if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is no longer required and will be removed from the backend. It's only still there for backwards compatibility with Josh's app 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol ok. I'll remove it then

@@ -86,5 +86,6 @@ internal val testOffering: Offering
availablePackages = listOf(
Package("package_id", PackageType.ANNUAL, storeProduct, "offering_id"),
),
paywall = null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests will be in a followup PR so we can start building the view asap

@vegaro vegaro requested a review from a team September 6, 2023 11:00
data class PaywallData(
/**
* The type of template used to display this paywall.
*/
val templateName: String,
@SerialName("template_name") val templateName: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking most fields need to add this to support the snake_case to camelCase transformation and it would be good to do this automatically... But from a quick search, looks like there is JsonNamingStrategy but it's still experimental... So this might be the best for now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Thankfully it looks like they will add it soon

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.

LGTM!

internal val localization: Map<String, LocalizedConfiguration>,
@SerialName("asset_base_url") @Serializable(with = URLSerializer::class) val assetBaseURL: URL,
// TODO-PAYWALLS: do we need this one?
@SerialName("default_locale") val defaultLocale: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm we should confirm with the team, but maybe we should make it optional in case it disappears?

@vegaro vegaro merged commit 7fd52a8 into paywalls Sep 6, 2023
@vegaro vegaro deleted the cesar/pwl-148-sdk-parse-offerings-response-paywall-data-and-add_3 branch September 6, 2023 13:51
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.

Awesome 👏🏻 data driven FTW

@@ -47,28 +53,29 @@ data class PaywallData(
/**
* Whether the background image will be blurred (in templates with one).
*/
val blurredBackgroundImage: Boolean = false,
@SerialName("blurred_background_image") val blurredBackgroundImage: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the serializer decode the default value if it's not present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes


/**
* If set, the paywall will display a privacy policy link.
*/
val privacyURL: URL? = null,
@SerialName("privacy_url") @Serializable(with = URLSerializer::class) val privacyURL: URL? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this ignore errors if the URL is invalid? Better for it to become null than failing the decode the whole thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't actually tested it. I'll check but I think it's going to crash. Good call

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.

3 participants