Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't show the Notifications setting for profile only enterprises in the Users section #7307

Merged
merged 1 commit into from Jun 30, 2021
Merged

Don't show the Notifications setting for profile only enterprises in the Users section #7307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 15 additions & 14 deletions app/views/admin/enterprises/form/_users.html.haml
View file
Open in desktop
Expand Up @@ -14,20 +14,21 @@
- else
= owner_email

.row
.three.columns.alpha
=f.label :user_ids, t('.notifications')
- if full_permissions
%span.required *
%div{'ofn-with-tip' => t('.contact_tip')}
%a= t('admin.whats_this')
.eight.columns.omega
- if full_permissions
%select.select2.fullwidth{id: 'receives_notifications_dropdown', name: 'receives_notifications', ng: {model: 'receivesNotifications', init: "receivesNotifications = '#{@enterprise.contact.id}'"}}
%option{ng: {repeat: 'user in Enterprise.users', selected: "user.id == #{@enterprise.contact.id}", hide: '!user.confirmed'}, value: '{{user.id}}'}
{{user.email}}
- else
= @enterprise.contact.email
- if @enterprise.is_distributor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe unless @enterprise.producer_profile_only would more explicitly exclude profile-only enterprises?

Copy link
Contributor Author

@cillian cillian Apr 9, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

.row
.three.columns.alpha
=f.label :user_ids, t('.notifications')
- if full_permissions
%span.required *
%div{'ofn-with-tip' => t('.contact_tip')}
%a= t('admin.whats_this')
.eight.columns.omega
- if full_permissions
%select.select2.fullwidth{id: 'receives_notifications_dropdown', name: 'receives_notifications', ng: {model: 'receivesNotifications', init: "receivesNotifications = '#{@enterprise.contact.id}'"}}
%option{ng: {repeat: 'user in Enterprise.users', selected: "user.id == #{@enterprise.contact.id}", hide: '!user.confirmed'}, value: '{{user.id}}'}
{{user.email}}
- else
= @enterprise.contact.email

.row
.three.columns.alpha
Expand Down
42 changes: 42 additions & 0 deletions spec/views/admin/enterprises/form/_users.html.haml_spec.rb
View file
Open in desktop
@@ -0,0 +1,42 @@
# frozen_string_literal: true

require "spec_helper"

describe "admin/enterprises/form/_users.html.haml" do
let(:enterprise) { build(:enterprise) }

before do
assign(:enterprise, enterprise)
admin_user = build(:admin_user)
allow(admin_user).to receive(:admin?) { true }
allow(view).to receive_messages(
f: enterprise_form,
spree_current_user: admin_user,
)
end

describe "notifications setting" do
it "is visible when an enterprise is a distributor" do
enterprise.sells = "any"

render

expect(rendered).to have_selector("select[name='receives_notifications']")
end

it "is not visible when an enterprise is a only a profile" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if they're a producer though...? Are there no emails in that case? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. order cycle producers notification
    This is email is for sells == 'none' / is_distributor == false

enterprise.sells = "none"

render

expect(rendered).not_to have_selector("select[name='receives_notifications']")
end
end

private

def enterprise_form
form_for(enterprise) { |f| @enterprise_form = f }
@enterprise_form
end
end