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

Refactor checking for available packages when creating package configuration #1307

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

vegaro
Copy link
Contributor

@vegaro vegaro commented Oct 4, 2023

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
Copy link

codecov bot commented Oct 4, 2023

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.

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.

Much clearer, thanks!

activelySubscribedProductIdentifiers = activelySubscribedProductIdentifiers,
filter = packageIds,
packageIdsInConfig = paywallData.config.packages,
Copy link
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
Contributor 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 <[email protected]>
@vegaro vegaro enabled auto-merge (squash) October 4, 2023 13:08
@vegaro vegaro merged commit 22405a5 into paywalls Oct 4, 2023
5 checks passed
@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
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