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

Update cancel method on subscription to respect minimum date #178

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

seand7565
Copy link
Contributor

Previously, the cancel method would add one day to your
minimum_cancellation_date. This updates that! More info here:
#177

@aldesantis
Copy link
Member

@seand7565 thanks! A couple of things:

  • Can you reword the commit message to explain the issue there directly, rather than asking people to also open the GH issue? I think it's more practical. 🙂
  • Can we make the default 0.days rather than 1.day? The latter seems like an arbitrary chosen number that could easily lead to problems.

@seand7565
Copy link
Contributor Author

@aldesantis Done, let me know if that looks good to you!

@aldesantis
Copy link
Member

@seand7565 thanks! Could you also lock canonical-rails to a version that fixes the whitelisted_parameters problem? The tests are failing because of that. 😩

Previously, the cancel method would check if the minimum
cancellation date minus the current day is in the future before
cancelling.

However, that adds one day to whatever minimum cancellation date
that you set - if you set one day, you would expect the user not
to be able to cancel on the day that the subscription ships, but
because the method checks if the date is in the future, the user
can't cancel the day of, or the day before.

This checks if the date is in the future OR if it's today, which
removes the added day from the minimum_cancellation_notice.

Also this sets the default to 0.days, as 1.day is pretty arbitrary

Lastly, this locks canonical-rails down to 0.2.9, because Solidus
still uses the `whitelisted_attributes` method, which was renamed
in 0.2.10
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.

3 participants