Update admin cart page dynamically #1715

jhawthorn · 2017-02-16T21:18:06Z

Built on top of #1710

This fully models the displayed information on the cart page in backbone, which allows it to update dynamically without relying on the previous window.location.reload().

This allows for smoother interaction by the user, and avoids undesirable changes from the refresh like closing in-progress edits.

vfonic · 2017-02-17T01:49:03Z

backend/app/assets/javascripts/spree/backend/format_money.js

@@ -0,0 +1,10 @@
+(function() {
+  var defaultLocale = document.documentElement.lang;


Is there a default fallback language for when locale is not set?

I'd assume that would be English. In that case, this makes more sense:

var defaultLocale = document.documentElement.lang || 'en';

It will always be set. I don't think we need to be defensive here.

vfonic · 2017-02-17T01:54:43Z

backend/app/assets/javascripts/spree/backend/format_money.js

+  Spree.formatMoney = function(amount, currency, options) {
+    options = (options || {});
+    var locale = options.locale || defaultLocale;
+    var format = new Intl.NumberFormat(locale, {style: 'currency', currency: currency})


There's a big discussion whether semicolons should be always added or not in JS. I'd lean towards the "mandatory semicolons", simply because it prevents unexpected errors from happening. You can see one such example in this SO answer:
http://stackoverflow.com/a/8108912/336806

Speaking of, is there any linter used for JS? What are the JS style rules (e.g. semicolons)

Nope. Please feel free to file an issue.

vfonic · 2017-02-17T01:56:03Z

backend/app/assets/javascripts/spree/backend/format_money.js

+(function() {
+  var defaultLocale = document.documentElement.lang;
+
+  Spree.formatMoney = function(amount, currency, options) {


This function looks like something I'd like to be able to also use on frontend. It seems to be impossible without including solidus_backend. Is there a central place where such functions could go?

Also, defaultLocale could be calculated in a centralized place.

I can appreciate that, but I'm not 100% sure that this will be the permanent implementation of this method. We might have to end up including accounting.js or similar, which we would not want cluttering up the frontend. For that reason, I want to keep this backend-only.

I guess we can both agree that "temporary fix" tends to stay around much longer than what we planned it to stay. :)

This is not a temporary fix. The frontend and backend have different javascript, different requirements, and different browser support.

vfonic · 2017-02-17T02:15:23Z

backend/app/assets/stylesheets/spree/backend/shared/_forms.scss

@@ -124,6 +124,11 @@ button, .button {
    width: 100%;
    text-align: center;
  }
+
+  &[disabled], &.disabled {


I think the more preferred way would be to use :disabled CSS3 selector. Although they are the same, some explanation why use one over the [disabled] can be found here:
http://stackoverflow.com/a/20146936/336806

It works in cross-browser:
http://caniuse.com/#feat=css-sel3

Good call 👍

vfonic · 2017-02-17T02:23:22Z

backend/app/views/spree/admin/orders/_adjustments.html.erb

+    <thead>
+      <tr>
+        <th><%= Spree::Adjustment.human_attribute_name(:name)%></th>
+        <th><%= Spree::Adjustment.human_attribute_name(:amount)%></th>


These should be translated:
Screenshot:

Nevermind, this is a solidus_i18n issue

vfonic · 2017-02-17T02:39:58Z

backend/app/views/spree/admin/orders/_adjustments.html.erb

+      <% adjustments.eligible.group_by(&:label).each do |label, adjustments| %>
+        <tr class="total">
+          <td><strong><%= label %>:</strong></td>
+          <td class="total align-center"><span><%= Spree::Money.new(adjustments.sum(&:amount), currency: adjustments.first.order.try(:currency)) %></span></td>


Not related to these changes, but you could add this fix in:

currency: adjustments.first.order.try(:currency)

=>

currency: order.currency

...since you're always passing order variable and it's always defined.

Good call. We should address this in this PR

vfonic · 2017-02-17T02:43:51Z

backend/spec/features/admin/orders/order_details_spec.rb

-        sku = order.line_items.first.variant.sku
-        expect(page).to have_content("SKU: #{sku}")
+      context "with a completed order" do
+        let!(:order) { create(:completed_order_with_totals) }


I think it's better having this inlined inside of the it block:

order = create(:completed_order_with_totals)

It's always better to have the code contained in the single place, than trying to figure out where did this variable come from (especially if let! is used and not let).

Using let is better for composability and is always preferred. This let actually overrides a let!(:order) higher up in the file, which the local definition would not do. This is a good thing.

Do you mean it's always preferred in solidus project(s) or it's generally always preferred? Because I prefer not to use it and I can find some good arguments why it should be avoided:
https://robots.thoughtbot.com/lets-not
http://linduxed.com/blog/2014/08/24/writing-more-readable-rspec-tests/

Of course there are always arguments pro and contra. Just asking out of curiosity.

vfonic · 2017-02-17T02:46:18Z

core/config/locales/en.yml

@@ -711,6 +711,7 @@ en:
    add_action_of_type: Add action of type
    add_country: Add Country
    add_coupon_code: Add Coupon Code
+    add_line_item: Add Item


Is there any process for reporting this new translation to solidus_i18n? Perhaps an issue could be created on the solidus_i18n repo when translation-changes/new-translation-is-added?

There is a rake task in solidus_i18n for this purpose. This question is way outside the scope of this PR.

vfonic · 2017-02-18T10:09:16Z

backend/app/assets/javascripts/spree/backend/format_money.js

+  Spree.formatMoney = function(amount, currency, options) {
+    options = (options || {});
+    var locale = options.locale || defaultLocale;
+    var format = new Intl.NumberFormat(locale, {style: 'currency', currency: currency})


vfonic · 2017-02-18T10:09:18Z

backend/app/assets/javascripts/spree/backend/format_money.js

+(function() {
+  var defaultLocale = document.documentElement.lang;
+
+  Spree.formatMoney = function(amount, currency, options) {


I guess we can both agree that "temporary fix" tends to stay around much longer than what we planned it to stay. :)

vfonic · 2017-02-18T10:11:06Z

backend/spec/features/admin/orders/order_details_spec.rb

-        sku = order.line_items.first.variant.sku
-        expect(page).to have_content("SKU: #{sku}")
+      context "with a completed order" do
+        let!(:order) { create(:completed_order_with_totals) }


Do you mean it's always preferred in solidus project(s) or it's generally always preferred? Because I prefer not to use it and I can find some good arguments why it should be avoided:
https://robots.thoughtbot.com/lets-not
http://linduxed.com/blog/2014/08/24/writing-more-readable-rspec-tests/

Of course there are always arguments pro and contra. Just asking out of curiosity.

tvdeyen

A great UX improvement. 👍

I have some little code styling nits and I'm not sure if we should put all these backbone models and views into large files or if it would be better to have them separate overridable files instead.

As always 🍻 work!

tvdeyen · 2017-02-22T21:14:39Z

backend/app/views/spree/admin/orders/_adjustments.html.erb

+      <% adjustments.eligible.group_by(&:label).each do |label, adjustments| %>
+        <tr class="total">
+          <td><strong><%= label %>:</strong></td>
+          <td class="total align-center"><span><%= Spree::Money.new(adjustments.sum(&:amount), currency: adjustments.first.order.try(:currency)) %></span></td>


Good call. We should address this in this PR

tvdeyen · 2017-02-22T21:22:12Z

backend/app/assets/javascripts/spree/backend/orders/models.js

+  });
+  model.fetch(options);
+  return model;
+}


Maybe we should split this file into files for each model? So devs could override individual models (the order for instance)? Not sure though, if this could ever be the case.

I agree. Going to split these into separate files and also move to a different namespace, Spree.Models.Order, which will also make more sense as we add more models (like Spree.Models.Variant)

👍🏻 for the namespace

tvdeyen · 2017-02-22T21:23:57Z

backend/app/assets/javascripts/spree/backend/orders/line_items.js.coffee

+
+  render: ->
+    @$el.prop("disabled", !this.collection.length || this.collection.some( (item) -> item.isNew() ))
+


I kinda confuses me, that some files are coffeescript and some are javascript. I would prefer to go with JS from now on. But we should be consistent.

Also maybe splitting these views into separate overridable files would be preferable?

I also prefer JS, but this file was originally coffeescript and the changes were each incremental

Ok. Fair enough.

tvdeyen · 2017-02-22T21:28:24Z

backend/app/assets/javascripts/spree/backend/orders/order_details.js

+    this.$el.toggleClass('hidden', !lineItemCount)
+    this.$('.order-total').text(this.model.get("display_total"))
+  }
+})


Also consider to split this file up

tvdeyen · 2017-02-22T21:30:11Z

backend/app/assets/javascripts/spree/backend/orders/order_summary.js

+    this.$('dd.order-shipment_state').html(this.renderState('shipment_state', this.model.get("shipment_state")))
+
+    this.$('.order-payment_state').toggleClass("hidden", !this.model.get("completed_at"))
+    this.$('dd.order-payment_state').html(this.renderState('payment_state', this.model.get("payment_state")))


Not a backbone expert, but do we really need to use this.$ over just $?

Yes. this.$ is scoped within the view's element. It's great!

TIL. Indeed very nice 👌🏻

tvdeyen · 2017-02-22T21:31:42Z

backend/app/assets/javascripts/spree/backend/templates/orders/line_item.hbs

+	<a class="edit-line-item fa fa-edit no-text with-tip" href="#" title="{{ t "actions.edit" }}"></a>
+	<a class="delete-line-item fa fa-trash no-text with-tip" href="#" title="{{ t "actions.delete" }}"></a>
+{{/if}}
+</td>


I think the indentation in this file messed up somehow.

tvdeyen · 2017-02-22T21:32:44Z

backend/app/assets/stylesheets/spree/backend/sections/_orders.scss

+  }
+
+  .select2-container.error {
+    border-radius: 4px;


I believe we have a global $border-radius variable we can use here.

tvdeyen · 2017-02-22T21:34:23Z

backend/app/views/spree/admin/orders/_form.html.erb

-        :title => Spree.t(:order_adjustments)
-   } %>
+  <%= render :partial => "spree/admin/orders/carton", :collection => @order.cartons.order(:shipped_at), :locals => { :order => order } %>
+  <%= render :partial => "spree/admin/orders/shipment", :collection => @order.shipments.order(:created_at), :locals => { :order => order } %>


We should only use Ruby 1.9 Hash syntax if we touch lines of code

tvdeyen · 2017-02-22T21:39:36Z

backend/app/views/spree/admin/shared/_order_summary.html.erb

-      <dt><%= Spree::Shipment.model_name.human %>: </dt>
-      <dd id='shipment_status'><span class="state <%= @order.shipment_state %>"><%= Spree.t(@order.shipment_state, :scope => :shipment_states, :default => [:missing, "none"]) %></span></dd>
-      <dt><%= Spree::Payment.model_name.human %>: </dt>
-      <dd id='payment_status'><span class="state <%= @order.payment_state %>"><%= Spree.t(@order.payment_state, :scope => :payment_states, :default => [:missing, "none"]) %></span></dd>
      <dt data-hook='admin_order_tab_date_completed_title'><%= Spree::Order.human_attribute_name(:completed_at) %>:</dt>
      <dd id='date_complete'><%= pretty_time(@order.completed_at) %></dd>
    <% end %>


This view could profit from some indenting and usage of Ruby 1.9 hash syntax.

jhawthorn · 2017-03-02T01:32:12Z

@tvdeyen This is ready for review :)

Models and views are now namespaced and in the models/ and views/ directories. I believe I have addressed the other feedback.

Tests have passed, but CircleCI is being CircleCI and hasn't updated github.

tvdeyen

🎉 I really love the way this goes. Very well structured backend code. This is a huge improvement for us. Thanks for working on this, John! 🍬 🥃 🎂

Just one thought. Could we get rid off the need to create the Spree.Models and Spree.Views objects in each file by adding spree/backend/models/base and spree/backend/views/base files and require these instead? Nothing we need to tackle now.

This is ready to be merged IMO 👍

tvdeyen · 2017-03-02T07:58:20Z

backend/app/assets/javascripts/spree/backend/models/line_item.js

@@ -0,0 +1,9 @@
+//= require spree/backend/routes


I don't think we need routes in this file

tvdeyen · 2017-03-02T08:30:07Z

@jhawthorn I see a fixup commit. Maybe you need to rebase once more?

This hack existed from before the "cart" page existed to allow stores which didn't perform deliveries to still edit items.

Works around an issue where an Order was being fetched by the server at the same time the spec was creating an order by factory. SQLite3::BusyException: database is locked:

This simplifies fetching the order from the API, which we will want to do in several places. It hides the implementation detail of the placeholder arrays for the line_items and shipments collections.

tvdeyen · 2017-03-02T21:06:47Z

🚀

vfonic suggested changes Feb 17, 2017

View reviewed changes

jhawthorn added the Code review needed label Feb 17, 2017

vfonic approved these changes Feb 18, 2017

View reviewed changes

This was referenced Feb 20, 2017

Admin: Adding Variant Product Properties is confusing #1724

Closed

Admin: Make actions table columns show inside of a table #1725

Closed

jhawthorn mentioned this pull request Feb 22, 2017

[WIP] Update admin shipments page dynamically #1731

Closed

tvdeyen requested changes Feb 22, 2017

View reviewed changes

tvdeyen mentioned this pull request Feb 28, 2017

Admin cart page revamp #1710

Merged

jhawthorn force-pushed the cart_page_ajax_updates branch from 6610fe4 to e7cc666 Compare March 1, 2017 22:12

tvdeyen approved these changes Mar 2, 2017

View reviewed changes

jhawthorn added 18 commits March 2, 2017 11:18

Move line_items.js.coffee into orders/

170980b

Add order_summary JS

7538643

Use backbone to fetch Order for order_summary

fc369f7

Add Backbone Order#advance

7442a6e

Listen to changes on line item

728af86

Split Order model into own file

ef751c1

Use LineItemCollection

b2bbd2e

Add backbone-nested-models

3609f25

Use Collection.push to add new line items

a3011d8

Render line_items through relation

a7e94bb

Use additional Backbone views on cart page

ed429b6

Move initialization into own file and remove hacks

44f2027

Avoid inline JS and globals on cart page

103dabd

Don't fall back to line items on shipments page

36a4ce0

This hack existed from before the "cart" page existed to allow stores which didn't perform deliveries to still edit items.

Move extract common order details into partial

9fcac32

Wrap adjustments in div

c58f70b

Hide order details via CSS

4f43b6c

Update Total in order details dynamically

d7a9175

jhawthorn added 9 commits March 2, 2017 11:18

Update adjustments rows dynamically

7bf6b19

Work around SQLite3::BusyException in specs

a5116fb

Works around an issue where an Order was being fetched by the server at the same time the spec was creating an order by factory. SQLite3::BusyException: database is locked:

Hide empty adjustment tables from confirm page

8753ae7

Add a new item row if the last item is deleted

b75b441

Introduce Spree.Models.Order.fetch

ace6acd

This simplifies fetching the order from the API, which we will want to do in several places. It hides the implementation detail of the placeholder arrays for the line_items and shipments collections.

Require models and collections

f1967f4

Split line_items.js.coffee into separate JS files

f36b8fe

Split order details into separate view files

2d20830

Move order_summary.js into views

4ea9fb9

jhawthorn force-pushed the cart_page_ajax_updates branch from e7cc666 to 4ea9fb9 Compare March 2, 2017 19:19

jhawthorn merged commit 663aee0 into solidusio:master Mar 2, 2017

		@@ -0,0 +1,10 @@
		(function() {
		var defaultLocale = document.documentElement.lang;


		render: ->
		@$el.prop("disabled", !this.collection.length \|\| this.collection.some( (item) -> item.isNew() ))

Update admin cart page dynamically #1715

Update admin cart page dynamically #1715

Conversation

jhawthorn commented Feb 16, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfonic Feb 17, 2017 • edited Loading

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

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

tvdeyen 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

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

jhawthorn Feb 23, 2017 • edited Loading

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

jhawthorn commented Mar 2, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented Mar 2, 2017

tvdeyen commented Mar 2, 2017

jhawthorn commented Feb 16, 2017 •

edited

Loading

vfonic Feb 17, 2017 •

edited

Loading

jhawthorn Feb 23, 2017 •

edited

Loading