Fixes for consistent handling of resource errors on admin

[ikraamg]

Description

Referring to the detailed examples in issues #3604 and #3143, there was inconsistency in the way errors were handled due to non-existent resources on the admin panel. The aim was to bring consistency to these errors by redirecting all errors to the relevant page with a relevant flash_message.

This PR fixes all the mentioned issues as well as additional inconsistencies and creates mulitple tests to ensure all the fixes are not affected in the future.

Previously, these nil-resource routes were redirected correctly:

products
/admin/products/nil-product/edit
/admin/products/nil-product/stock
users
/admin/users/nil-user/edit
/admin/users/nil-user/addresses
/admin/users/nil-user/orders
/admin/users/nil-user/items
promotions
/admin/promotion_categories/nil-category/edit
/admin/promotions/nil-promotion/edit
option types
/admin/option_types/nil-type/edit
property types
/admin/properties/nil-prop/edit
taxonomies
/admin/taxonomies/nil-taxonomy/edit

After this PR, the following are fixed and redirected correctly as well:

orders
/admin/orders/nil-order/edit
/admin/orders/nil-order/customer_returns
/admin/orders/nil-order/customer_returns/1/edit
/admin/orders/nil-order/cart
/admin/orders/nil-order/customer/edit
/admin/orders/nil-order/adjustments
/admin/orders/nil-order/payments
/admin/orders/nil-order/return_authorizations
/admin/orders/nil-order/cancellations
products
/admin/products/nil-product/images
/admin/products/nil-product/prices
/admin/products/nil-product/product_properties
/admin/products/nil-product/variants
taxons
/admin/taxonomies/nil-taxon/taxons/3/edit
/admin/taxonomies/1/taxons/nil-taxon/edit
users
/admin/users/nil-user/store_credits

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[ikraamg]

Hi Solidus Team!

This is my first contribution to solidus, I would be glad to receive any feedback for improving the PR and advice on any further next steps I should take to have this merged.

I have a few questions:

• The bin/build did pass all tests locally but with a stream of deprecation warnings, is this expected? (I see that the CircleCI tests are failing as well due to what seems to be the deprecation warnings)

• I had a thought after making these commits that it may have been useful in cases where there was no 404 error raised, to manually raise ActionController::RoutingError.new('Not Found') or something similar to get a 404. Then 'rescue' before redirecting to the correct 'path'. Do you advise I should refactor the code to do this or is that unnecessary since the code works as expected?

Thanks,
Ikraam

[jarednorman]

Thanks for contributing! This PR adds a lot of special cases that I think could be better handled by generalized implementations that are part of Spree::Admin::ResourceController. I'd like to make this stuff more consistent by using the same code everywhere, not by adding more special cases when many of them do fundamentally the same thing.

---

Side note: We like to follow these conventions for git commit formatting. Your commits offer good details and are definitely nothing to complain about, but if you happened to update them to follow the wrapping and summary line conventions, it would be appreciated. 🙂

[ikraamg]

Thanks for contributing! This PR adds a lot of special cases that I think could be better handled by generalized implementations that are part of Spree::Admin::ResourceController. I'd like to make this stuff more consistent by using the same code everywhere, not by adding more special cases when many of them do fundamentally the same thing.

Side note: We like to follow these conventions for git commit formatting. Your commits offer good details and are definitely nothing to complain about, but if you happened to update them to follow the wrapping and summary line conventions, it would be appreciated.

Thanks for the suggestions and pointers, I have made an attempt to refactor the duplicated code into the base_controller. Kindly let me know if this should be further improved.

I am still looking into the return parent.send(controller_name) if parent? vs
return parent.send(controller_name) unless parent.nil? issue. I will mention my findings inside the code conversation above. Using parent.nil? also seems to be the reason why there are so many deprecation warnings in CircleCI.

I trust the latest git commit is formatted as per the conventions 😄

[jarednorman]

Nice changes, I think we're getting close here. A few pieces of feedback:

1. In this project we generally prefer to rebase unmerged branches to address feedback in the commits that made the original changes, rather than add new commits addressing feedback. This keeps our git history clean from the churn as we home in on implementations we're happy with.
2. There's a bit of a naming issue, since nil_parent? probably shouldn't have a question mark since it isn't technically a predicate method. Additionally it describes the situation where it is used more than what is does, so I think we could probably come up with a better name.
3. Rather than using the old-school options hash style of arguments, perhaps we could use keyword arguments for #resource_not_found.

Those are relatively minor things. From a bigger picture perspective, I think we could still stand to make this a bit more elegant. There's too things I think need to be addressed:

1. ActiveRecord::RecordNotFound errors expose the specific model that was not found. This can be used to infer whether it was the parent or the main model that was not found and generate a reasonable default redirect path. It's possible this might be able to cover all the cases, right?
2. I don't think that parent should ever be nil when parent? is true. I'm still missing why/how this might be possible and why we need to handle it.

Also, forgive me if I'm being picky about how we might change classes like Spree::Admin::ResourceController. Because stores and extensions add customizations that depend on these classes (the names of the methods in them, the options those methods take, etc.) we must be very careful that changes here are exactly what we want them to be to avoid creating unnecessary churn and work for stores trying to upgrade.

[ikraamg]

Thanks for the detailed review, I feel a huge difference in how I am able to approach a solution after each review 😄

In reply to some of your notes:

1. ActiveRecord::RecordNotFound errors expose the specific model that was not found. This can be used to infer whether it was the parent or the main model that was not found and generate a reasonable default redirect path. It's possible this might be able to cover all the cases, right?

TheActiveRecord::RecordNotFound error seems to only occur for the 'orders' and 'taxonomies' controller, they both inherit from the 'base_controller'. The image_controller also raises this error. All these controllers do some kind of resource loading through which the error gets raised.

Most of the other errors are 'NoMethodErrors' that occur in the first return statement of the resources#collection method. Though, I think the principle of this approach is the same as I am rescuing errors from both types and redirecting them either via resource_not_found and resource_error_filter.

2. I don't think that parent should ever be nil when parent? is true. I'm still missing why/how this might be possible and why we need to handle it.

I have refactored the code to avoid having to deal with this but I can give an overview of what I found.

It seems that for particularly these links the issue occurs:

/admin/products/nil-product/prices
/admin/products/nil-product/product_properties
/admin/products/nil-product/variants
/admin/users/nil-user/store_credits

The issue seems to occur because #parent? checks to see if the #parent_data exists and it does in these cases, so it returns true:

def parent?
    self.class.parent_data.present?
end

But the confusion comes in because of #parent since parent? is true for these links so the method runs the query (that I have not wrapped my head around). I am assuming it requires that #belongs_to was defined on 'children' of ResourceController to successfully run. But then it returns nil for some reason, creating the NoMethodError for nil:NilClass in the #collection method:

return parent.send(controller_name) if parent?

Here is an image of this scenario when it is tested with the BetterErrors console.

def parent
    if parent?
      @parent ||= self.class.parent_data[:model_class]
                    .includes(self.class.parent_data[:includes])
                    .find_by(self.class.parent_data[:find_by] => params["#{parent_model_name}_id"])
      instance_variable_set("@#{parent_model_name}", @parent)
    else
      Spree::Deprecation.warn "Calling #parent is deprecated on a ResourceController which has not defined a belongs_to"
      nil
    end
  end

Also, forgive me if I'm being picky about how we might change classes like Spree::Admin::ResourceController. Because stores and extensions add customizations that depend on these classes (the names of the methods in them, the options those methods take, etc.) we must be very careful that changes here are exactly what we want them to be to avoid creating unnecessary churn and work for stores trying to upgrade.

That makes complete sense. I assumed that it would be a challenge to meet the required code quality standard in order to contribute, so thank you for your time in guiding me through this.

I have squashed the first 5 commits that I made into one and summarised the text and wrapped it correctly. So there are now 3 larger commits, is this what you had in mind?

Kindly advise me on how to get the tests for store_credits_controller and images_controller to work, I have set them to pending due to Rspec unable able to find the classes User and Product. I assume that I have to somehow include those classes in the test file but I was not able to find information regarding that.

[jarednorman]

Okay, I think I was misunderstanding how this all fit together and I think we're close to what we need.

Perhaps, it should be the case that the #parent method should use find_by! rather than find_by and that the not found error should be rescued right in that method. This way we have access to the correct model for the flash and all that right there. This negates the possibility of parent ever returning nil when parent? is true and completely avoids the no method error (which is never a great error to rescue because of how general it is... it can hide other unintended errors.

It seems to me that we could avoid much of the complications here by capturing the error in parent and the error in find_resource which both have the necessary context to call the not found with the correct arguments explicitly. Non-resource controllers would want to rescue the not found errors in the most specific spot possible, for example in load_order for the OrdersController.

A brief overview of my suggestions:

• Remove the default values from the implementation of resource_not_found in Spree::Admin::BaseController.
• Override that method in Spree::Admin::ResourceController to add the default values (since they only make sense for resource controllers.
• Rescue ActiveRecord::RecordNotFound in Spree::Admin::ResourceController#parent and Spree::Admin::ResourceController#find_resource and call resource_not_found appropriately.
• Add explicit calls to resource_not_found when the resource is being loaded in non-resource controllers.

This makes only a tiny additive change to the signature of the Spree::Admin::ResourceController#resource_not_found (allowing it to accept optional arguments) that shouldn't impact existing customizations/extensions that rely on this and exposes the logic properly to non-resource controllers so they can use it where they need to.

[ikraamg]

So I have finally made some time to have a go at it again and I hope we are really close now.
The suggestions really helped.
There were a couple of challenges that I encountered and I had to make some other changes to get the fixes to work. I wrote up the detailed context in the commit message.

Cheers!

[jarednorman]

I didn't understand your explanation in the commit about the unused err argument. Otherwise I've left what amount to mostly style suggestions. Overall I think this looks good.

[ikraamg]

I hope my explanation of using _err is understandable after replying to your comment above. I ended up using the splat argument as it seemed more concise than _error = nil.

Please see the reply to your comment above for the explanation for the and return. I've changed it so Rubocop doesn't complain.

I found some other 'Order' URLs that also created 404's or 500's. I've updated the list in the PR description and added fixes to the code in the same style that we discussed. I am quite sure it's all covered now 💯

Cheers!

[jarednorman]

One minor thing, otherwise I think I'm happy with this change. I had not thought about how the rescue_from handler receives the exception object if it accepts an argument.

[jarednorman]

Alright, I think I'm happy with this change now. I don't think it'll cause any serious incompatibility issues for this code, but we'll have to see if someone else from core has any thoughts oon that.

[ikraamg]

Alright, I think I'm happy with this change now. I don't think it'll cause any serious incompatibility issues for this code, but we'll have to see if someone else from core has any thoughts oon that.

Perfect. Thanks Jared 👍

[aldesantis]

Thanks for the contribution @ikraamg, I left a couple of comments.

[kennyadsl]

Left some comments, thanks for the great work here!

[ikraamg]

The latest commit is rebased onto master and except for the && parent, the corrections discussed are made. 👍

Please have a look at my reply to the && parent discussion, perhaps a conclusion can be found.

[ikraamg]

I believe the only blocking item in this PR is the use of the && parent condition in #collection.

In order to summarise the discussion, I have made a temporary commit which implements and explains the alternative implementation to replace && parent as mentioned in the above thread.

[jarednorman]

Thanks for your hard work on this. I've been a little short on time this week, but I've made a note to come back and take a look at this.

[jarednorman]

I think the final commit makes this less elegant... I'd rather stick with the extra condition. I think it makes sense.

[kennyadsl]

I agree with @jarednorman, there's something going on there that we don't get now. Keeping the extra condition seems a good trade-off. If we go with removing the last commit and keep the extra condition, can we document well why are we keeping it via the commit description? Thanks, this work is awesome!

[ikraamg]

Thanks @jarednorman and @kennyadsl for helping close the discussion. I agree that the alternative does not look very elegant.

I trust the commit description is adequate as it now explains the && parent. I am excited to have this contribution pulled in!

@jarednorman I appreciate the thorough guidance throughout the PR 🤝

[jarednorman]

Thanks for all your work on this! I'm happy here once @aldesantis's thread is resolved.

[aldesantis]

Good to go as far as I'm concerned!

[aldesantis]

Thanks so much @ikraamg, this is great. 🙂