Authorize only changed vos #6789
Conversation
sauloperez
force-pushed
the
authorize-only-changed-vos
branch
from
f03c30f to
119da1f
Compare
Closes openfoodfoundation#6727. This avoids the authorization of all the VOs of the hub, which will go through VOs that may have become invalid due to their underlying product not belonging to the supplier the hub has permissions with (or any other data integrity issue). This is utterly confusing for the user who is only given a generic error and doesn't understand what's wrong with the particular VO they changed, while it may be fine after all. What's more, this often results in a customer support request, which then may end up with a dev finding out which VO is broken. Also, there's no point in loading them from DB if the users didn't touch them.
This removes the following annoying deprecation warnings that happen in each test. ``` DEPRECATION WARNING: You are trying to generate the URL for a named route called :main_app but no such route was found. In the future, this will result in an `ActionController::UrlGenerationError` exception. (called from process_action_with_route at /usr/s rc/app/spec/support/controller_requests_helper.rb:49) DEPRECATION WARNING: Passing the `use_route` option in functional tests are deprecated. Support for this option in the `process` method (and the related `get`, `head`, `post`, `patch`, `put` and `delete` helpers) will be removed in the next version without replacement. Functional tests are essentially unit tests for controllers and they should not require knowledge to how the application's routes are configured. Instead, you should explicitly pass the appropiate params to the `process` method. Previously th e engines guide also contained an incorrect example that recommended using this option to test an engine's controllers within the dummy application. That recommendation was incorrect and has since been corrected. Instead, you should override the `@routes` variable in the test case with `Foo::Engine.routes`. See the updated engines guide for details. (called from process_action_with_route at /usr/src/app/spec/support/controller_requests_helper.rb:49) ```
sauloperez
force-pushed
the
authorize-only-changed-vos
branch
from
119da1f to
a19acea
Compare
filipefurtad0
added
pr-staged-uk
|
Hey @sauloperez, I've tried to reproduce the use case which was breaking the inventory. i) On a given Enterprise with permissions to
ii) As superadmin
iii) checked back the inventory on the inventory of the enterprise:
However, if I simply refresh the inventory page on the Enterprise session, than the issue is solved - as item is removed from the inventory. I think in the issue you described @RachL the item was still on the Inventory list. So, I'm not sure I'm reproducing the issue correctly. Any advice here? |
|
Almost @filipefurtad0. What we are changing here is that for a variant override X supplied by Producer A, modifying any other VO supplied by a separate producer, for which permissions are correctly set up, succeeds without any error message. Only modifying VO X should cause an error and forbid the change. Before this change, in the scenario you outlined above, any change to any VO would cause an error. |
|
That was close! Got it now :-D Updated: Adding step iv) to the test cases above (i.e, before staging this PR):
On the next comment I'll go through this test case, and add some other. |
filipefurtad0
added
pr-staged-uk
|
After going through steps i) to iv) (comments above), it was verified that:
I did a brief sanity check on the inventory functionality, and checked that VO permissions work as before. Good to go! |
What? Why?
Closes #6727.
This avoids the authorization of all the VOs of the hub, which will go through VOs that may have become invalid due to their underlying product not belonging to the supplier the hub has permissions with (or any other data integrity issue).
This is utterly confusing for the user who is only given a generic error and doesn't understand what's wrong with the particular VO they changed, while it may be fine after all. What's more, this often results in a customer support request, which then may end up with a dev finding out which VO is broken.
Also, there's no point in loading them from DB if the users didn't touch them.
What should we test?
This seems to be quite well covered with tests but it wouldn't harm to check that indeed the inventory keeps working with no regressions. While doing so, and if manual tests pass, we'll hopefully increase our confidence in this part of the test suite.
Also, a newly added test proves the change fixes the issue.
Release notes
Don't authorize variant overrides that haven't been modified by the user so inventory changes fail only if the specific variant override is not valid.
Changelog Category: User facing changes