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-1783] Don't filter local pickup based on shipping rule #2179

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

ifosli
Copy link
Contributor

@ifosli ifosli commented Oct 17, 2024

📲 What

Previously, we handled local pickup rewards similar to shippable rewards, and required them to match the shipping rule's location. Now we handle them similar to digital rewards instead (they'll always show as a pledge option).

This pr also the reward summary table, which was waiting for a non-nil shipping rule before showing up.

🤔 Why

With the previous setup, we don't include local pickup rewards when determining what locations are selectable. If a project had only local pickup rewards, we filtered out all rewards except the "no reward" option on iOS, with no way to unhide them (the shipping location selector only shows if there's at least one shippable reward).

✅ Acceptance criteria

  • A local pickup can be selected when there's no shipping rule or a mismatch between the selected shipping rule and the local pickup location.
  • In both of these cases, a user's pledge goes through. (Tested in staging - I didn't find a project in staging that only has local pickup rewards, so I hardcoded the shipping rule to nil.)

⏰ TODO

  • Not in this pr, I'm going to file a ticket (if we don't already have one) to add the local pickup view back in to the pledge screen UI. The current UI does not make the "local pickup" part clear enough. Created MBL-1784 to track this.

@ifosli ifosli self-assigned this Oct 17, 2024
@ifosli ifosli changed the title Don't filter local pickup based on shipping rule [MBL-1783] Don't filter local pickup based on shipping rule Oct 17, 2024
@ifosli ifosli marked this pull request as ready for review October 17, 2024 15:54
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 a little concerned that we didn't catch that the pledge summary table was dependent on the shipping rule sooner, considering what this screenshot looked like before, but at least it's fixed now!

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.

Looks good! Thanks for the quick fix!

Copy link
Contributor

@stevestreza-ksr stevestreza-ksr left a comment

Choose a reason for hiding this comment

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

Would be really good to have a test to cover this, but otherwise LGTM!

@ifosli
Copy link
Contributor Author

ifosli commented Oct 17, 2024

Would be really good to have a test to cover this, but otherwise LGTM!

I agree but since it's not already tested and I wanted it done/merged asap, I didn't want to take the time. I'll add a test or two in a followup pr!

@ifosli ifosli merged commit 7184ab9 into main Oct 17, 2024
5 checks passed
@ifosli ifosli deleted the localPickupFix branch October 17, 2024 17:03
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.

3 participants