Admin pricing view #1510

harmonymjb · 2016-10-07T22:57:54Z

This PR expands on the Admin Prices view to clarify master/variant pricing. See #1167

Master and Variant related prices have been separated into their own tables utilising new scopes on the Price model. Additionally, Option Types and Option Values are shown in a table as a reference for the user. As recommended by @Mandily Tooltips are shown in the Master Variant and Options tables to clarify their content.

Tests regarding assigns in this context have been updated to reflect these changes.

Here is a screenshot for reference:

mamhoff · 2016-10-08T14:00:06Z

I love this. 👍

mamhoff · 2016-10-08T13:45:25Z

backend/app/controllers/spree/admin/prices_controller.rb

@@ -7,8 +7,14 @@ def index
        params[:q] ||= {}

        @search = @product.prices.accessible_by(current_ability, :index).ransack(params[:q])
-        @prices = @search.result


Changing instance variable names in controllers can be hazardous if people overrode those views. This has not been around for long at all, and I do not suspect many people have used this instance variable name, so I won't ask for a deprecation warning, but maybe a short note in the ChangeLog.

Thanks, noted in the ChangeLog.

isaacfreeman · 2016-10-10T00:18:11Z

👍

I'd like to see master variants displayed to users throughout Solidus admin alongside regular variants. That is, instead of saying "this is a product with no variants", we should be saying "this is a product with one variant – click here if you want to make another". I appreciate the original intent was to avoid complexity for beginners, but I've worked on too many sites where clients and former devs had apparently never encountered the concept of a variant at all, and instead extended the product model in cumbersome ways. Variants aren't hard to understand, and the concept only has to be learnt once to forestall a lot of expensive re-writing later.

So... this is a positive step.

Mandily · 2016-10-11T20:28:47Z

core/config/locales/en.yml

@@ -1115,6 +1115,8 @@ en:
    hints:
      spree/price:
        country: "This determines in what country the price is valid.<br/>Default: Any Country"
+        master_variant: "Changing these prices will not change variant prices below, but will be used to populate all new variants"
+        options: "These are the options used in the table below. They can be changed in the variants tab"


Is it possible to make the text "variants tab" actually link to the variants tab?

Be more explicit with the name of the table. "These options are used to create variants in the variant table. They can be changed in the variants tab"

Yes, but not if it is displayed as a Tooltip because the Tooltip is only displayed on mouseover. We could instead place this text so it is always visible and attach the link.

The new bootstrap layouts have a place for this kind of information where we could also put links. Let's leave it in the tooltip for now, and when we convert over to the new layouts we can augment the text with a link in the sidebar.

Mandily · 2016-10-11T20:29:17Z

This looks great! My only comment/concern would be pulling those table titles out of the merged table cell and making them a proper heading so that it's a bit easier to scan the page. We could also then add one for "Variant pricing" which could be referred to in the tooltip messaging instead of "the table below".

Mandily · 2016-10-11T20:33:09Z

core/config/locales/en.yml

@@ -1115,6 +1115,8 @@ en:
    hints:
      spree/price:
        country: "This determines in what country the price is valid.<br/>Default: Any Country"
+        master_variant: "Changing these prices will not change variant prices below, but will be used to populate all new variants"


Change "these" to "master variant" to be more explicit

harmonymjb · 2016-10-11T21:15:50Z

Here is what it looks like with fieldset legend style headers.

Mandily · 2016-10-11T21:59:29Z

While not a fan of that fieldset style, I do like the hierarchy it gives to the page and it's a better middle ground for us to work from when we update styles. 👍

jhawthorn · 2016-11-07T21:13:07Z

Though this works better for products with variants, it's now extremely confusing on products with only a master variant.

Maybe we need to special-case that to only show the master variant prices and lose the heading/hint.

jhawthorn · 2016-11-07T21:15:07Z

CHANGELOG.md

@@ -36,6 +36,9 @@

 *   The `lastname` field on `Address` is now optional. [#1369](https://github.com/solidusio/solidus/pull/1369)

+*   Change backend `prices_controller` instance variable `@prices` to `@master_prices` and


This should probably mention what user-facing change was made. Maybe:

The backend prices listing page now shows master prices separate from variant prices. This changes instance variable @prices to @master_prices and @variant_prices.

jhawthorn · 2016-11-07T21:55:06Z

I don't understand the purpose of the Options table here.

I think the option types relate only loosely to Prices (in that the options are what variants use, and variants have prices) and is going to be more confusing to users than helpful. New variants aren't created from this page.

I think that table would make sense on the Variants page, but not here.

mamhoff · 2016-11-07T22:08:53Z

@jhawthorn Yep, I have to agree. The user needs to know which option types are selected for a particular variant she's pricing, but she doesn't create new variants here, which she would need that table for.

harmonymjb · 2016-11-07T22:23:39Z

@jhawthorn @mamhoff The options table was suggested to help the user quickly reference if they have prices set for all the relevant variants.
A select box of variants is available to the user when creating a new price, so options are not very useful to display on the prices index page.

harmonymjb · 2016-11-07T22:50:42Z

A couple of updated screenshots with options table removed, and only showing variants table if the product has variants.

Without variants:

With variants:

This has been addressed.

harmonymjb force-pushed the admin_pricing_view branch from db966e7 to da34d38 Compare October 7, 2016 23:06

mamhoff previously requested changes Oct 8, 2016

View reviewed changes

Mandily reviewed Oct 11, 2016

View reviewed changes

harmonymjb force-pushed the admin_pricing_view branch from 1647224 to d16b057 Compare October 11, 2016 21:10

harmonymjb force-pushed the admin_pricing_view branch from d16b057 to 06e2a64 Compare October 11, 2016 21:34

jhawthorn added UI changelog:solidus_backend Changes to the solidus_backend gem labels Oct 12, 2016

jhawthorn reviewed Nov 7, 2016

View reviewed changes

Graham Bouvier added 7 commits November 7, 2016 14:16

Add for_master and for_variant scopes to price model

03306bd

Partition prices for master and variants in prices controller

9eec41f

Expand price table view to show prices for master seperately

01b6f85

Add I18n variables for static text in price table

31f43b0

Add table view for option types and values to admin prices view

106f1e6

Update prices controller spec to reflect new assigns

cd5f6f9

Update pricing feature spec to reflect new assigns

edcd253

harmonymjb force-pushed the admin_pricing_view branch from 06e2a64 to 77e0847 Compare November 7, 2016 22:41

Graham Bouvier added 3 commits November 7, 2016 14:41

Update changelog

431971c

Add fieldset legends to sections of admin price page

471979f

Remove options table view from admin prices view

a793e5e

harmonymjb force-pushed the admin_pricing_view branch from 77e0847 to bcff2f0 Compare November 7, 2016 22:48

Render variants only if some exist prices on admin prices page

bcff2f0

jhawthorn approved these changes Nov 9, 2016

View reviewed changes

jhawthorn merged commit 08cce83 into solidusio:master Nov 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin pricing view #1510

Admin pricing view #1510

harmonymjb commented Oct 7, 2016

mamhoff commented Oct 8, 2016

mamhoff Oct 8, 2016

harmonymjb Oct 11, 2016

isaacfreeman commented Oct 10, 2016

Mandily Oct 11, 2016

Mandily Oct 11, 2016

harmonymjb Oct 11, 2016

Mandily Oct 11, 2016

Mandily commented Oct 11, 2016

Mandily Oct 11, 2016

harmonymjb commented Oct 11, 2016

Mandily commented Oct 11, 2016

jhawthorn commented Nov 7, 2016

jhawthorn Nov 7, 2016

jhawthorn commented Nov 7, 2016

mamhoff commented Nov 7, 2016

harmonymjb commented Nov 7, 2016

harmonymjb commented Nov 7, 2016

		@@ -36,6 +36,9 @@

		* The `lastname` field on `Address` is now optional. [#1369](https://github.com/solidusio/solidus/pull/1369)

		* Change backend `prices_controller` instance variable `@prices` to `@master_prices` and

Admin pricing view #1510

Admin pricing view #1510

Conversation

harmonymjb commented Oct 7, 2016

mamhoff commented Oct 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacfreeman commented Oct 10, 2016

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

Mandily commented Oct 11, 2016

Choose a reason for hiding this comment

harmonymjb commented Oct 11, 2016

Mandily commented Oct 11, 2016

jhawthorn commented Nov 7, 2016

Choose a reason for hiding this comment

jhawthorn commented Nov 7, 2016

mamhoff commented Nov 7, 2016

harmonymjb commented Nov 7, 2016

harmonymjb commented Nov 7, 2016