[BUU] Fix non-admin saving #12412
[BUU] Fix non-admin saving #12412
Conversation
So that it can be used for more general purposes.
Instead of replacing frame contents with unhelpful text 'Content missing'.
| @@ -173,7 +178,7 @@ | |||
| search_by_category "Category 1" | |||
|
|
|||
| # expect(page).to have_content "1 product found for your search criteria." | |||
| expect(page).to have_select "category_id", selected: "Category 1" | |||
| expect(page).to have_select "category_id", selected: "Category 1" # succeeds in CI but not dev. | |||
There was a problem hiding this comment.
Hmm I forgot to go back and investigate why, but maybe next time..
| include WebHelper | ||
| include AuthenticationHelper | ||
| include FileHelper | ||
|
|
||
| let(:producer) { create(:supplier_enterprise) } |
There was a problem hiding this comment.
In many places the codebase says "supplier". We now prefer the term "producer", which is used in general communication, so I've named the variables as such.
Maybe one day we'll update the codebase to avoid confusion..
There was a problem hiding this comment.
Oh, I didn't actually know this. "Supplier" is a bit more general. The enterprise may not be producing the product. 🤷
There was a problem hiding this comment.
I recall having a conversation in Slack I think, and that "producer" was the winner.
I agree that "supplier" makes more sense, although arguably the distributor also "supplies" the product, so it could be ambiguous...
I can't find that conversation though, so now I'm not sure! I will seek clarification in #product-circle.
There was a problem hiding this comment.
I think this is the conversation https://openfoodnetwork.slack.com/archives/C01T75H6G0Z/p1679368367236359
Seems we ended up with "supplier" back then 🙂
There was a problem hiding this comment.
Thanks Sigmund, I forgot my own comments! 😅
I'm going to write it on a post-it note on my computer screen..
I forgot to do this in openfoodfoundation#12328 [BUU] Remove Stimulus Reflex from Products screen
dacook
force-pushed
the
buu/fix-non-admin-saving-12403
branch
from
9b9e459
to
b846d0f
Compare
| } else { | ||
| alert(I18n.t("errors.general_error.message")); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Hmm, in Rails we arrange code like this in a folder called initializers. Maybe we could do the same with JS next time.
This comment was marked as resolved.
This comment was marked as resolved.
|
FYI I did test with a non admin role on staging FR and fields like updating images or stock are working ok. Now I see however that updating product unit name for example raises the error "Content is missing". Sorry for that! I've used an admin role for testing those as it required to have several pages of product :( |
Fully agree, thanks @dacook . Unfortunately, this is a common practice in our tests, we should consider improving this and making sessions more user specific (instead of always being logged as superadmin). |
filipefurtad0
added
no-staging-FR
|
Hey @dacook , I've staged master and found that indeed, for some attributes (like SKU or as @RachL menitons, product unit name) we get a 401 error, as described in the issue, while the UI throws this error - this reproduces the issue:
After staging the PR, I could not observer the error anymore, nor the 401 error on the console:
I've tested updating all fields, including images. Also, since you mention this touches checkout, I felt it was a good idea to have a smoke test on checking out: looking good! As I understood from @RachL comment, this was before staging the PR, right? If so, then I think we're good to merge, but I'll await your confirmation. |
|
yes before staging the PR - I was referring to the PR that introduced this. |
|
Ok! Merging :-) |
Project code: #7198 Back Office Uplift (Product): Build
What? Why?
The
Spree::Admin::BaseControllerrequires a permission defined for each controller action, so I've now added this.It wasn't covered by the system spec because it only tested as an admin (which automatically has access to everything). I've updated the spec to always use an enterprise user, which is a better test.
But the error message was pretty disruptive and unhelpful (see issue). So in case it happens again, I've added a general error handler to show a message, based on the HTTP code (401 - Unauthorized).
What should we test?
Just a quick validation ideally by Konrad:
This also touches the general Turbo code which I think is used in the checkout process. But I believe this change is sufficiently covered by specs.
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.