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

Remove Spree ResourceController callbacks #7174

Merged

Conversation

Matt-Yorkley
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley commented Mar 21, 2021

What? Why?

Removes Spree's custom implementation of additional controller callbacks. We only use this in a couple of places, and they can be easily replaced with regular Rails controller callbacks or simple method calls.

The point of these extra callback hooks is to allow the maximum possible customization by apps using Spree as a gem, without having to modify the source. We're not in that position any more in relation to Spree code, so this extra callbacks layer is essentially a huge mess with no real value.

🔥🔥🔥

What should we test?

  • Updating a Schedule: when successfully update, should sync relevant subscriptions as before
  • Creating a PaymentMethod: should work correctly when creating a payment method for a hub
  • Creating a ReturnAuthorization: should work as before
  • Creating a Variant: should create it successfully, and with a valid price

Release notes

Remove Spree's resource controller callbacks implementation

Changelog Category: Technical changes

cleanup

@Matt-Yorkley Matt-Yorkley self-assigned this Mar 21, 2021
@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review March 21, 2021 14:30
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

😌 So good!

We have to be careful though. I've seen PRs recently introducing new calls. We have to check all current PR unless we do some deprecation first.

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

Yes, please!! It should be easy to trace if they are still called somewhere even by raising an exception when any of the ActionCallbacks methods are used and see if the build passes. However, it seems we fixed them all according to my git grep results.

You didn't add testing steps @Matt-Yorkley. Perhaps you want to check if these actions are covered by at least controller tests and see if the build is enough.

@luisramos0
Copy link
Contributor

Wonderful PR!!! 👏 👏 👏

@mkllnk
Copy link
Member

mkllnk commented Mar 22, 2021

Well, I found the one PR I remembered that added usage of these callbacks. So this should be fine as long as we keep that in mind when reviewing.

@filipefurtad0 filipefurtad0 self-assigned this Apr 1, 2021
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Apr 1, 2021
@filipefurtad0
Copy link
Contributor

Hey @Matt-Yorkley ,

Thanks for pointing out the relevant places/test cases for this clean-up 🙏

Tested:

  • Updating a Schedule: when successfully update, should sync relevant subscriptions as before - this appears to work just as before. Deleted, created and changed the attribution of schedules to OCs (see additional notes, below).
  • Creating a PaymentMethod - all as before
  • Creating a ReturnAuthorization - all as before
  • Creating a Variant - created a new variant, added to OC, placed an order - all as before this PR

This looks good to go 👍

Additional notes:

While testing schedules/subs I've ran into this one again: #7294 (payments not successful) and this scenario, as reported here

It seems to be possible to delete a schedule (as a workaround for openfoodfoundation/wishlist#283), by removing the schedule from all the previously associated OCs here (clicking the x):

image

If this is done, then the hub can run into the situation in which a subscription exists, but has no schedule associated with it, although it appears so like it does:

Peek 2021-04-01 15-03

Moving on to check if this was reported already elsewhere; Not related to this PR so -> ready to go!

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.

6 participants