Fix: Admin visiting non-existent enterprise raises error #13224
Conversation
Currently when an admin tries to edit an no-existing enterprise, a NoMethodError is raised. This commit adds a set_enterprise setter method to the enterprises controller that sets the @enterprise instance variable to have the same value as the enterprise object defined in the the edit method; this method also rescues the NotFound error in case the enterprise is not found and redirects the user to the enterprises index page with a error message.
sigmundpetersen
moved this from All the things 💤
to Code review 🔎
in OFN Delivery board
rioug
added
the
user facing changes
There was a problem hiding this comment.
Thanks @Tresor11 🙏
Your solution does fix the problem, but looking at the code I can't tell where this @enterprise is coming from. I wonder if the better fix would be to replace the @enterprise by @object, which represent the model we are looking at. It should be loaded properly via Admin::ResourceController Let's see what other reviewers think.
There was a problem hiding this comment.
Thanks for looking into this. It appears that it solves the issue, but it would be good to add a request spec to clarify exactly what it solves.
From what I can see, this change shouldn't be necessary. The EnterprisesController#load_methods_and_fees method is expected to run after ResourceController#load_resource. load_resource calls the ActiveRecord find method, which should raise an exception if the resource (enterprise) is not found (see https://github.com/dacook/openfoodnetwork/blob/master/app/controllers/admin/resource_controller.rb#L153-L159).
The ResourceController is intended to handle that and redirect you to the index page, just as your fix does (see resource_not_found).
So it would be good to investigate why that functionality wasn't working in this case. The best way would be to write a request spec as mentioned, then you can poke and prod (eg with binding.pry) to see what's happening.
Would you be able to have a go at that?
github-project-automation
bot
moved this from Code review 🔎
to In Progress ⚙
in OFN Delivery board
Tresor11
force-pushed
the
tb-rescue-not-found-error-for-enterprise
branch
2 times, most recently
from
c8ecf63 to
72c855e
Compare
This commit updates the method name to be called required_enterprise since we only expect it to raise an error when the enterprise is not found.
Tresor11
force-pushed
the
tb-rescue-not-found-error-for-enterprise
branch
from
72c855e to
507705a
Compare
|
Thanks for the review. @dacook @rioug I agree that reassigning the enterprise instance doesn't feel quite right, since we are already handling the |
filipefurtad0
added
pr-staged-au
|
Hey @Tresor11, welcome 💐 Many thanks for fixing this one. I've reproduced the issue, by logging in as an admin and visiting the enterprise admin path, with a non-existing enterprise name, which triggered the error 500 you described. After staging the PR, repeating the same scenario displayed the error message:
The translation is being picked up correctly - repeating the steps before, with FR as a locale:
Looks great! Merging 🎉 |
github-project-automation
bot
moved this from Test Ready 🧪
to Done
in OFN Delivery board
What? Why?
Currently, when an admin tries to edit a no-existing enterprise, a
NoMethodErroris raised.This commit adds a require_enterprise method that raises a
ActiveRecord::RecordNotFounderror when the enterprise is not found and redirects the user to the enterprise index page with an error message.What should we test?
/admin/enterprises/no-extisting/edityou should be redirected to the enterprises path with an error messageRelease notes
Changelog Category (reviewers may add a label for the release notes):