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

PaywallData validation #1273

Merged
merged 33 commits into from
Sep 26, 2023

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Sep 20, 2023

Add validation to paywall data and creates default template if validation fails

@vegaro vegaro added the pr:feat A new feature label Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2ea162f) 85.46% compared to head (6ed190e) 85.46%.

Additional details and impacted files
@@            Coverage Diff            @@
##           paywalls    #1273   +/-   ##
=========================================
  Coverage     85.46%   85.46%           
=========================================
  Files           190      190           
  Lines          6385     6385           
  Branches        930      930           
=========================================
  Hits           5457     5457           
  Misses          568      568           
  Partials        360      360           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NachoSoto
Copy link
Contributor

This is looking good 👍🏻

@vegaro vegaro force-pushed the cesar/pwl-193-implement-paywalldata-validation branch from 2d44608 to bb02408 Compare September 21, 2023 12:54
Base automatically changed from cesar/pwl-193-implement-paywalldata-validation to paywalls September 21, 2023 13:13
@vegaro vegaro force-pushed the cesar/pwl-193-implement-paywalldata-validation-2 branch 2 times, most recently from bf8bb92 to a899d61 Compare September 22, 2023 09:51
@vegaro vegaro changed the base branch from paywalls to cesar/pwl-192-create-default-paywalldata September 22, 2023 10:19
@@ -63,6 +64,7 @@ private fun getPaywallViewModel(offering: Offering?, listener: PaywallViewListen
LocalContext.current.applicationContext.toAndroidContext(),
offering,
listener,
MaterialTheme.colors,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check if light/dark theme colors get automatically updated in the default paywall. My strong bet is that no. In that case, we should maybe store the last state in the view model so we can update the state when it changes, but we can do that in another PR.

)
} catch (e: Exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're not doing this anymore, it may crash in the TemplateConfigurationFactory.create since we require a that there is at least some packages. Should we perform that validation before to avoid the crash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I missed those require

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vegaro vegaro force-pushed the cesar/pwl-193-implement-paywalldata-validation-2 branch from c6d5e40 to 1f821dd Compare September 22, 2023 14:45
Base automatically changed from cesar/pwl-192-create-default-paywalldata to paywalls September 22, 2023 15:59
@vegaro vegaro force-pushed the cesar/pwl-193-implement-paywalldata-validation-2 branch from a13c1f4 to 2f424c7 Compare September 22, 2023 16:13
@vegaro vegaro marked this pull request as ready for review September 22, 2023 16:40
@vegaro vegaro changed the title [WIP] PaywallData validation PaywallData validation Sep 22, 2023
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 a few minor things left, but this is perfect 🎉

@vegaro
Copy link
Contributor Author

vegaro commented Sep 26, 2023

Fixed the light/dark colors. We will default to the current color scheme now, when it changes, and we react to the change

@vegaro
Copy link
Contributor Author

vegaro commented Sep 26, 2023

@tonidero I will do the icons validation in another PR so we can merge this one. Do you mind taking a look?

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.

Looks great!

@@ -10,37 +10,35 @@ internal object TemplateConfigurationFactory {
fun create(
variableDataProvider: VariableDataProvider,
mode: PaywallViewMode,
paywallData: PaywallData,
validatedPaywallData: PaywallData,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this should keep being paywallData to not be confused with ValidatedPaywallResult... I think it's ok though.

@vegaro vegaro merged commit dc1d023 into paywalls Sep 26, 2023
7 checks passed
@vegaro vegaro deleted the cesar/pwl-193-implement-paywalldata-validation-2 branch September 26, 2023 16:49
tonidero pushed a commit that referenced this pull request Oct 31, 2023
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