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.

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

[BUU2] Hide producer column when there's only one producer in the admin account #12854

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions app/helpers/admin/products_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ def product_image_form_path(product)
end
end

def prepare_new_variant(product)
product.variants.build
def prepare_new_variant(product, producer_options)
# e.g producer_options = [['producer name', id]]
product.variants.build do |new_variant|
new_variant.supplier_id = producer_options.first.second if producer_options.one?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If only one producer exists, the producer column will be hidden according to the issue. In that case, the producer should be auto-assigned so that UI doesn't show an error on saving the variant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nicer to have producer_options as an array of hashes: [ { name: 'producer name', id: id } ], anyway its fine to keep it like this.

end
end

def unit_value_with_description(variant)
Expand All @@ -37,5 +40,9 @@ def products_return_to_url(url_filters)

"#{admin_products_path}#{url_filters.empty? ? '' : "#?#{url_filters.to_query}"}"
end

def hide_producer_column?(producer_options)
spree_current_user.column_preferences.products.empty? && producer_options.one?
end
rioug marked this conversation as resolved.
Show resolved Hide resolved
end
end
14 changes: 12 additions & 2 deletions app/models/column_preference.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@ class ColumnPreference < ApplicationRecord
validates :column_name, presence: true, inclusion: { in: proc { |p|
valid_columns_for(p.action_name)
} }
scope :products, -> { where(action_name: 'products_v3_index') }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should rename this to bulk_edit_product so it's clear we are talking about a page and not a product.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's fixed here: 5be53a4


def self.for(user, action_name)
stored_preferences = where(user_id: user.id, action_name:)
default_preferences = __send__("#{action_name}_columns")
default_preferences = get_default_preferences(action_name, user)
filter(default_preferences, user, action_name)
default_preferences.each_with_object([]) do |(column_name, default_attributes), preferences|
stored_preference = stored_preferences.find_by(column_name:)
Expand All @@ -36,7 +37,7 @@ def self.for(user, action_name)
end

def self.valid_columns_for(action_name)
__send__("#{action_name}_columns").keys.map(&:to_s)
get_default_preferences(action_name, Spree::User.new).keys.map(&:to_s)
end

def self.known_actions
Expand All @@ -52,4 +53,13 @@ def self.filter(default_preferences, user, action_name)

default_preferences.delete(:schedules)
end

def self.get_default_preferences(action_name, user)
case action_name
when 'products_v3_index'
products_v3_index_columns(user)
else
__send__("#{action_name}_columns")
end
end
rioug marked this conversation as resolved.
Show resolved Hide resolved
end
1 change: 1 addition & 0 deletions app/models/spree/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class User < ApplicationRecord
has_many :credit_cards, dependent: :destroy
has_many :report_rendering_options, class_name: "::ReportRenderingOptions", dependent: :destroy
has_many :webhook_endpoints, dependent: :destroy
has_many :column_preferences, dependent: :destroy
has_one :oidc_account, dependent: :destroy

accepts_nested_attributes_for :enterprise_roles, allow_destroy: true
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/products_v3/_product_variant_row.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
%tr.condensed{ id: dom_id(variant), 'data-controller': "variant", 'class': "nested-form-wrapper", 'data-new-record': variant.new_record? ? "true" : false }
= render partial: 'variant_row', locals: { variant:, f: variant_form, category_options:, tax_category_options:, producer_options: }

= form.fields_for("products][#{product_index}][variants_attributes][NEW_RECORD", prepare_new_variant(product)) do |new_variant_form|
= form.fields_for("products][#{product_index}][variants_attributes][NEW_RECORD", prepare_new_variant(product, producer_options)) do |new_variant_form|
%template{ 'data-nested-form-target': "template" }
%tr.condensed{ 'data-controller': "variant", 'class': "nested-form-wrapper", 'data-new-record': "true" }
= render partial: 'variant_row', locals: { variant: new_variant_form.object, f: new_variant_form, category_options:, tax_category_options:, producer_options: }
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/products_v3/_table.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
= hidden_field_tag :producer_id, @producer_id
= hidden_field_tag :category_id, @category_id

%table.products{ 'data-column-preferences-target': "table" }
%table.products{ 'data-column-preferences-target': "table", class: (hide_producer_column?(producer_options) ? 'hide-producer' : '') }
Copy link
Member

Choose a reason for hiding this comment

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

I can't see a CSS rule for this class, did you forget to commit it?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm maybe it's not necessary at all: the column defaults take care of hiding it.
So I guess we don't need this hide_producer_column? helper.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't see a CSS rule for this class, did you forget to commit it?

It's the existing style:

$columns:
"image", "name", "sku", "unit_scale", "unit", "price", "on_hand", "producer", "category",
"tax_category", "inherits_properties";
@each $col in $columns {
&.hide-#{$col} {
.col-#{$col} {
display: none;
}
}
}
}

the column defaults take care of hiding it.

We need it because JS takes time to load and hide the column. Using it in the HTML immediately hides it when necessary.

%colgroup
-# The `min-width` property works in Chrome but not Firefox so is considered progressive enhancement.
%col.col-image{ width:"44px" }= # (image size + padding)
Expand Down
13 changes: 11 additions & 2 deletions lib/open_food_network/column_preference_defaults.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@ def products_index_columns
}
end

def products_v3_index_columns
def products_v3_index_columns(user)
producer_visibility = display_producer_column?(user)

I18n.with_options scope: 'admin.products_page.columns' do
{
image: { name: t(:image), visible: true },
Expand All @@ -87,7 +89,7 @@ def products_v3_index_columns
unit_scale: { name: t(:unit_scale), visible: true },
price: { name: t(:price), visible: true },
on_hand: { name: t(:on_hand), visible: true },
producer: { name: t(:producer), visible: true },
producer: { name: t(:producer), visible: producer_visibility },
category: { name: t(:category), visible: true },
tax_category: { name: t(:tax_category), visible: true },
inherits_properties: { name: t(:inherits_properties), visible: true },
Expand Down Expand Up @@ -134,5 +136,12 @@ def subscriptions_index_columns
shipping_method: { name: I18n.t("admin.shipping_method"), visible: false }
}
end

def display_producer_column?(user)
producers = OpenFoodNetwork::Permissions.new(user)
.managed_product_enterprises.is_primary_producer

producers.many?
end
end
end
77 changes: 47 additions & 30 deletions spec/system/admin/products_v3/actions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,42 +34,59 @@
describe "column selector" do
let!(:product) { create(:simple_product) }

before do
visit admin_products_url
end
context "with one producer only" do
before do
visit admin_products_url
end

it "hides column and remembers saved preference" do
# Name shows by default
expect(page).to have_checked_field "Name"
expect(page).to have_selector "th", text: "Name"
expect_other_columns_visible

it "hides column and remembers saved preference" do
# Name shows by default
expect(page).to have_checked_field "Name"
expect(page).to have_selector "th", text: "Name"
expect_other_columns_visible
# Producer is hidden by if only one producer is present
expect(page).not_to have_checked_field "Producer"
expect(page).not_to have_selector "th", text: "Producer"

# Name is hidden
ofn_drop_down("Columns").click
within ofn_drop_down("Columns") do
uncheck "Name"
# Name is hidden
ofn_drop_down("Columns").click
within ofn_drop_down("Columns") do
uncheck "Name"
end
expect(page).not_to have_selector "th", text: "Name"
expect_other_columns_visible

# Preference saved
click_on "Save as default"
expect(page).to have_content "Column preferences saved"
refresh

# Preference remembered
ofn_drop_down("Columns").click
within ofn_drop_down("Columns") do
expect(page).to have_unchecked_field "Name"
end
expect(page).not_to have_selector "th", text: "Name"
expect_other_columns_visible
rioug marked this conversation as resolved.
Show resolved Hide resolved
end
expect(page).not_to have_selector "th", text: "Name"
expect_other_columns_visible

# Preference saved
click_on "Save as default"
expect(page).to have_content "Column preferences saved"
refresh

# Preference remembered
ofn_drop_down("Columns").click
within ofn_drop_down("Columns") do
expect(page).to have_unchecked_field "Name"

def expect_other_columns_visible
expect(page).to have_selector "th", text: "Price"
expect(page).to have_selector "th", text: "On Hand"
end
expect(page).not_to have_selector "th", text: "Name"
expect_other_columns_visible
end

def expect_other_columns_visible
expect(page).to have_selector "th", text: "Producer"
expect(page).to have_selector "th", text: "Price"
expect(page).to have_selector "th", text: "On Hand"
context "with multiple producers" do
let!(:producer2) { create(:supplier_enterprise, owner: user) }

before { visit admin_products_url }

it "has selected producer column by default" do
# Producer shows by default
expect(page).to have_checked_field "Producer"
expect(page).to have_selector "th", text: "Producer"
end
end
end

Expand Down
1 change: 1 addition & 0 deletions spec/system/admin/products_v3/create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
include WebHelper

let!(:supplier) { create(:supplier_enterprise) }
let!(:supplier2) { create(:supplier_enterprise) }
rioug marked this conversation as resolved.
Show resolved Hide resolved
let!(:taxon) { create(:taxon) }

describe "creating a new product" do
Expand Down
1 change: 0 additions & 1 deletion spec/system/admin/products_v3/index_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
expect(page).to have_selector "th", text: "Unit"
expect(page).to have_selector "th", text: "Price"
expect(page).to have_selector "th", text: "On Hand"
expect(page).to have_selector "th", text: "Producer"
expect(page).to have_selector "th", text: "Category"
expect(page).to have_selector "th", text: "Tax Category"
expect(page).to have_selector "th", text: "Inherits Properties?"
Expand Down
3 changes: 2 additions & 1 deletion spec/system/admin/products_v3/update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
include FileHelper

let(:producer) { create(:supplier_enterprise) }
let(:user) { create(:user, enterprises: [producer]) }
let(:producer2) { create(:supplier_enterprise) }
let(:user) { create(:user, enterprises: [producer, producer2]) }

before do
login_as user
Expand Down
Loading