Don't show the Notifications setting for profile only enterprises in the Users section #7307
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7307 +/- ##
=======================================
Coverage 93.26% 93.26%
=======================================
Files 637 637
Lines 18119 18119
=======================================
Hits 16899 16899
Misses 1220 1220 Continue to review full report at Codecov.
|
| expect(rendered).to have_selector("select[name='receives_notifications']") | ||
| end | ||
|
|
||
| it "is not visible when an enterprise is a only a profile" do |
There was a problem hiding this comment.
What if they're a producer though...? Are there no emails in that case? 🤔
There was a problem hiding this comment.
I wouldn't be the most familiar with this but I had a look in wiki/Emails and I couldn't see any obvious emails for enterprises with sells == 'none' / is_distributor == false
There was a problem hiding this comment.
- order cycle producers notification
This is email is forsells == 'none'/is_distributor == false
| {{user.email}} | ||
| - else | ||
| = @enterprise.contact.email | ||
| - if @enterprise.is_distributor |
There was a problem hiding this comment.
Maybe unless @enterprise.producer_profile_only would more explicitly exclude profile-only enterprises?
There was a problem hiding this comment.
Yes unless @enterprise.producer_profile_only sounds better and I was going to change it there but I think I spotted a bug.
I don't think producer_profile_only ever gets set to true anymore, see https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/assets/javascripts/admin/enterprises/controllers/index_producer_panel_controller.js.coffee#L12 it's assigning with = instead of comparing with == That makes me wonder if the producer_profile_only column/method can be deprecated. What do you think?
There was a problem hiding this comment.
That's a great catch @cillian, I'm not sure of the consequences, tbh.
Even before this PR, I'm not sure how to set the producer_profile_only to true. This should correspond to set an enterprise as a "Producer" and "Profile only" (i.e., not a Shop, and not a Hub), right?
I've found this spec, https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/controllers/admin/enterprises_controller_spec.rb#L385 :
context "setting producer_profile_only" do
it "is ignored" do
spree_post :register, id: enterprise, sells: 'none', producer_profile_only: true
expect(response).to redirect_to spree.admin_dashboard_path
expect(enterprise.reload.producer_profile_only).to be false
end
end
So, if I understand this correctly, it seems to ignores this setting and as such, enterprise.producer_profile_only seems never to be true...
Update: I saw #7835 only after posting the comment above 😉
There was a problem hiding this comment.
I think this can be merged as it is. I created #7835 for the producer_profile_only field issue. I'm going to move this to Test Ready unless you disagree @Matt-Yorkley (the failing spec is flaky).
filipefurtad0
added
pr-staged-uk
…the Users settings section If a enterprise has only set up a profile they won't be getting any order notification emails so showing this setting could be confusing. #1797
andrewpbrett
force-pushed
the
hide-notifications-setting-for-profiles
branch
from
0d7f49e
to
2bb406f
Compare
|
Hey @cillian , I tried all profile combinations, and checked whether the
We're good to go, thank you! 🎉 |
|
Hi team, |
Fixes #1797
If a enterprise has only set up a profile they won't be getting any order notification emails so showing this setting could be confusing.
What should we test?
Release notes
Don't show the Notifications setting for profile only enterprises in the Users section
Changelog Category: User facing changes