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

MBL-1316: Use isAvailable to filter visible add-ons #2012

Merged
merged 6 commits into from Apr 2, 2024

Conversation

amy-at-kickstarter
Copy link
Contributor

📲 What

Use the GraphQL field isAvailable to filter visible add-ons

🤔 Why

Our existing implementation did not take late pledges into account, so it was filtering out add-ons that were actually still available for late pledges. I spoke with @zan-rosenthal and apparently the gold-standard solution is to use the available field from GraphQL.

Most of this PR is updating plumbing to get available into Reward.

@@ -24,7 +24,8 @@ extension Reward {
shippingRulesExpanded: $1.shippingRulesExpanded,
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'm still formulating some thoughts about how we can replace the Foo.template.lens |> ... pattern, so I used this as an opportunity to actually get into the guts of one to learn more about it.

isUnlimitedOrAvailable
]
.allSatisfy(isTrue)
return addOn.isAvailable ?? false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replace all the start time logic with isAvailable.

@@ -467,6 +484,7 @@ final class RewardAddOnSelectionViewModelTests: TestCase {
|> Reward.lens.remaining .~ nil
|> Reward.lens.startsAt .~ (MockDate().timeIntervalSince1970 + 30)
|> Reward.lens.endsAt .~ (MockDate().timeIntervalSince1970 + 60)
|> Reward.lens.isAvailable .~ false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test actually includes some unavailable rewards, so updating it to reflect that.

@amy-at-kickstarter amy-at-kickstarter marked this pull request as ready for review April 1, 2024 20:51
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

Nice! Sounds like this change is actually a long time coming anyway.

We should note this change in our regression testing since impacts all rewards

@amy-at-kickstarter amy-at-kickstarter merged commit 6fcf7ee into main Apr 2, 2024
5 checks passed
@amy-at-kickstarter amy-at-kickstarter deleted the feat/adyer/MBL-1316/fix-add-ons branch April 2, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants