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

chahmedejaz · 2024-09-10T19:57:45Z

What? Why?

Closes Hide producer column when there's only one producer in the admin account #11200
This PR adds the ability to hide the producer column if the there's only one producer available.

What should we test?

As mentioned in the issue

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

chahmedejaz · 2024-09-10T21:03:40Z

app/helpers/admin/products_helper.rb

+    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?


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.

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.

rioug

It's a good start but I think we can improve/clarify a few things

spec/system/admin/products_v3/create_spec.rb

app/models/column_preference.rb

app/helpers/admin/products_helper.rb

rioug · 2024-09-11T00:21:20Z

app/models/column_preference.rb

@@ -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') }


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

It's fixed here: 5be53a4

rioug · 2024-09-11T00:23:17Z

app/helpers/admin/products_helper.rb

+    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?


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.

spec/system/admin/products_v3/actions_spec.rb

dacook

Great, this has been on the list of things to do for a long time! I like the solution using the column defaults: it means the user can still choose to show it if they want to.

But it looks like there's some leftover code to remove?

dacook · 2024-09-13T06:59:24Z

app/helpers/admin/products_helper.rb

+    # if user hasn't saved any preferences on products page and there's only one producer;
+    # we need to hide producer column
+    def hide_producer_column?(producer_options)
+      spree_current_user.column_preferences.bulk_edit_product.empty? && producer_options.one?


It seems unnecessary to create a scope for this single use. I think using the where command directly would have been fine here.

I think scopes are a more cleaner way to query, don't you think? 😅

I guess it's a matter of personal preference. In general, yes it's better to use a scope which ensures the logic about the model, is in the model 👍

dacook · 2024-09-13T07:04:41Z

app/views/admin/products_v3/_table.html.haml

@@ -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' : '') }


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

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.

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

It's the existing style:

openfoodnetwork/app/webpacker/css/admin/products_v3.scss

Lines 185 to 195 in 6849155

$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.

dacook

Thanks for answering my questions, all good.

rioug

Looks good 👍 , Thanks for adding the comments !

filipefurtad0 · 2024-09-19T21:41:57Z

Hey @chahmedejaz ,

I had a quick test on this one:

Before staging

After staging

The column is hidden by default, but the user can still opt-in, if desired.

Merging! 🎉

11200: coniditonally hide producer column

1580d53

github-actions bot assigned chahmedejaz Sep 10, 2024

11200: add specs

f8d3467

chahmedejaz commented Sep 10, 2024

View reviewed changes

chahmedejaz changed the title ~~11200: coniditonally hide producer column~~ [BUU2] Hide producer column when there's only one producer in the admin account Sep 10, 2024

chahmedejaz marked this pull request as ready for review September 10, 2024 21:17

rioug self-requested a review September 10, 2024 23:37

rioug requested changes Sep 11, 2024

View reviewed changes

rioug added the user facing changes Thes pull requests affect the user experience label Sep 11, 2024

chahmedejaz added 3 commits September 11, 2024 11:41

11200: add explanations

76fdf37

11200: rename products scope

5be53a4

11200: add spec for display producer column

243a4a5

chahmedejaz requested a review from rioug September 11, 2024 08:31

dacook requested changes Sep 13, 2024

View reviewed changes

chahmedejaz requested a review from dacook September 13, 2024 22:15

dacook approved these changes Sep 15, 2024

View reviewed changes

rioug approved these changes Sep 15, 2024

View reviewed changes

filipefurtad0 self-assigned this Sep 19, 2024

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-fr staging.coopcircuits.fr labels Sep 19, 2024

filipefurtad0 merged commit 83ab959 into openfoodfoundation:master Sep 19, 2024
55 of 57 checks passed

RachL removed pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-fr staging.coopcircuits.fr labels Sep 21, 2024

dacook mentioned this pull request Oct 9, 2024

[BUU] Add missing specs #12898

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chahmedejaz commented Sep 10, 2024 •

edited

Loading

chahmedejaz Sep 10, 2024

rioug Sep 11, 2024

rioug left a comment

rioug Sep 11, 2024

chahmedejaz Sep 11, 2024

rioug Sep 11, 2024

dacook left a comment

dacook Sep 13, 2024

chahmedejaz Sep 13, 2024

dacook Sep 15, 2024

dacook Sep 13, 2024

dacook Sep 13, 2024

chahmedejaz Sep 13, 2024

dacook left a comment

rioug left a comment

filipefurtad0 commented Sep 19, 2024

	$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;
	}
	}
	}
	}

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

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

Conversation

chahmedejaz commented Sep 10, 2024 • edited Loading

What? Why?

What should we test?

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Sep 19, 2024

chahmedejaz commented Sep 10, 2024 •

edited

Loading