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

Add Prices tab to Products overview #1117

Merged
merged 6 commits into from May 20, 2016
Merged

Add Prices tab to Products overview #1117

merged 6 commits into from May 20, 2016

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented May 6, 2016

We have great functionality for dealing with multiple prices for each variant. This is an
absolutely basic interface so that as a store admin, it's possible to edit and update prices.

I'm unsure about allowing deletion and have left it out for now: When you delete a price,
you might end up with no price that has is_default set, which will then make the variant
in question become "unavailable". As a new price gets the is_default boolean anyways, the old
one becomes so meaningless it's as if it was deleted.

There are plans for a much better interface, but I need something simple that works for 1.3
to get out the door.

@mamhoff
Copy link
Contributor Author

mamhoff commented May 6, 2016

Index view without deletion
solidus administration prices 2016-05-06 17-33-44

Edit view (you can only edit the price itself, for existing prices changing currency or variant is confusing and disabled)
solidus administration prices 2016-05-06 17-34-15

New view (all fields can be edited)
solidus administration prices 2016-05-06 17-34-33

expect(page).to have_content(master_price.money.to_s)
expect(page).to have_content(master_price.currency)
expect(page).to have_content(other_price.money.to_s)
expect(page).to have_content(other_price.currency)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please set expectations on the actual value (e.g. 'USD') instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do.

@mamhoff
Copy link
Contributor Author

mamhoff commented May 17, 2016

@gmacdougall incorporated your feedback, and added pagination.

<% end %>
<% if can?(:destroy, price) %>
&nbsp;
<%#= link_to_delete(price, :no_text => true) unless price.deleted? %>
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional to disable the delete link here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is - there's no route for deletion yet. I should add that I guess 🐑 .

@jhawthorn
Copy link
Contributor

👍 tried this out. More work is needed, but I think it's good enough to merge. We can then collaborate on making it better.

Future steps:

  • Validate entered currency (it's possible currently to make a price in an invalid currency, ex. "XYZ")
  • Grouping common prices (@Dkendal and @Mandily did some investigation of this)
  • We have the master price both on the main product page as well as the pricing page. This will probably be cause of some confusion.
  • Showing "Master Variant" in the pricing table feels like we're leaking an implementation detail

@tvdeyen
Copy link
Member

tvdeyen commented May 19, 2016

I also think, this is good enough. How to we ensure to not lose track on the follow up tasks? Maybe a "pricing todo" issue?

@jhawthorn jhawthorn mentioned this pull request May 19, 2016
4 tasks
@jhawthorn
Copy link
Contributor

@tvdeyen done. We can use #1167 as a meta-issue to track these

@tvdeyen
Copy link
Member

tvdeyen commented May 19, 2016

Thanks John.

@mamhoff
Copy link
Contributor Author

mamhoff commented May 20, 2016

#1169 should fix the validation issue. I've amended this PR to use a select2 instead of a text input to make it harder to input invalid data. As for the last point: I've removed the .master_price translation from this PR and switched to instead using Variant#descriptive_name (which has the same leakage issue, but is already there as model-level logic).

mamhoff added 6 commits May 20, 2016 16:25
We have great functionality for dealing with multiple prices for each variant. This is an
absolutely basic interface so that as a store admin, it's possible to edit and update prices.

I'm unsure about allowing deletion and have left it out for now: When you delete a price,
you might end up with no price that has `is_default` set, which will then make the variant
in question become "unavailable". As a new price gets the `is_default` boolean anyways, the old
one becomes so meaningless it's as if it was deleted.

There are plans for a much better interface, but I need something simple that works for 1.3
to get out the door.
This defaults to ten prices per page. We can add a config option
for this later.
This prevents invalid currencies from being entered. Unfortunately,
translating the currencies' name carries loading lots and lots of
CLDR data, so the string presenting individual currencies in the select
box doesn't pass through `I18n`.
We want to minimize the amount of logic in views, and there is
a method that basically does this.
The table works just as well or better without us telling the browser how exactly
to render it.
@jhawthorn jhawthorn merged commit 8075984 into solidusio:master May 20, 2016
@jhawthorn
Copy link
Contributor

Thanks as always Martin

@mamhoff mamhoff deleted the pricing-tab branch May 24, 2016 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants