Skip to content

Refactor checking for available packages when creating package configuration#1307

Merged
vegaro merged 2 commits into
paywallsfrom
use-first-package-if-no-available-packages
Oct 4, 2023
Merged

Refactor checking for available packages when creating package configuration#1307
vegaro merged 2 commits into
paywallsfrom
use-first-package-if-no-available-packages

Conversation

@vegaro

@vegaro vegaro commented Oct 4, 2023

Copy link
Copy Markdown
Member

There shouldn't be any behavior change, but this refactor makes it more clear what happens when none of the configured packages are available.

@codecov

codecov Bot commented Oct 4, 2023

Copy link
Copy Markdown

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (cc1e396) 85.06% compared to head (f7b9099) 85.06%.

❗ Current head f7b9099 differs from pull request most recent head 0a69ed4. Consider uploading reports for the commit 0a69ed4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           paywalls    #1307   +/-   ##
=========================================
  Coverage     85.06%   85.06%           
=========================================
  Files           193      193           
  Lines          6475     6475           
  Branches        942      942           
=========================================
  Hits           5508     5508           
  Misses          600      600           
  Partials        367      367           

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

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much clearer, thanks!

availablePackages = availablePackages,
activelySubscribedProductIdentifiers = activelySubscribedProductIdentifiers,
filter = packageIds,
packageIdsInConfig = paywallData.config.packages,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we should rename the property packages to packageIds, even if the backend response remains the same... But that can be done separately if needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That will make things clearer. I will open a PR

…venuecatui/data/processed/PackageConfigurationFactory.kt

Co-authored-by: Toni Rico <toni.rico.diez@revenuecat.com>
@vegaro vegaro enabled auto-merge (squash) October 4, 2023 13:08
@vegaro vegaro merged commit 22405a5 into paywalls Oct 4, 2023
@vegaro vegaro deleted the use-first-package-if-no-available-packages branch October 4, 2023 13:19
tonidero added a commit that referenced this pull request Oct 31, 2023
…uration (#1307)

Co-authored-by: Toni Rico <toni.rico.diez@revenuecat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants