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

3576 Fix bulk order management race condition #3590

Merged
merged 13 commits into from
Mar 21, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -26,30 +26,31 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout,
$scope.startDate = moment(OrderCycles.byID[$scope.orderCycleFilter].orders_open_at).format('YYYY-MM-DD')
$scope.endDate = moment(OrderCycles.byID[$scope.orderCycleFilter].orders_close_at).startOf('day').format('YYYY-MM-DD')

formatted_start_date = moment($scope.startDate).format()
formatted_end_date = moment($scope.endDate).add(1,'day').format()

RequestMonitor.load $scope.orders = Orders.index(
"q[state_not_eq]": "canceled",
"q[completed_at_not_null]": "true",
"q[completed_at_gteq]": "#{moment($scope.startDate).format()}",
"q[completed_at_lt]": "#{moment($scope.endDate).add(1,'day').format()}"
"q[completed_at_gteq]": formatted_start_date,
"q[completed_at_lt]": formatted_end_date
)

RequestMonitor.load $scope.lineItems = LineItems.index(
"q[order][state_not_eq]": "canceled",
"q[order][completed_at_not_null]": "true",
"q[order][completed_at_gteq]": "#{moment($scope.startDate).format()}",
"q[order][completed_at_lt]": "#{moment($scope.endDate).add(1,'day').format()}"
"q[order][completed_at_gteq]": formatted_start_date,
"q[order][completed_at_lt]": formatted_end_date
)

unless $scope.initialized
RequestMonitor.load $scope.distributors = Enterprises.index(action: "visible", ams_prefix: "basic", "q[sells_in][]": ["own", "any"])
RequestMonitor.load $scope.orderCycles = OrderCycles.index(ams_prefix: "basic", as: "distributor", "q[orders_close_at_gt]": "#{moment().subtract(90,'days').format()}")
RequestMonitor.load $scope.suppliers = Enterprises.index(action: "visible", ams_prefix: "basic", "q[is_primary_producer_eq]": "true")

RequestMonitor.load $q.all([$scope.orders.$promise, $scope.distributors.$promise, $scope.orderCycles.$promise]).then ->
RequestMonitor.load $q.all([$scope.orders.$promise, $scope.distributors.$promise, $scope.orderCycles.$promise, $scope.suppliers.$promise, $scope.lineItems.$promise]).then ->
Dereferencer.dereferenceAttr $scope.orders, "distributor", Enterprises.byID
Dereferencer.dereferenceAttr $scope.orders, "order_cycle", OrderCycles.byID

RequestMonitor.load $q.all([$scope.orders.$promise, $scope.suppliers.$promise, $scope.lineItems.$promise]).then ->
Dereferencer.dereferenceAttr $scope.lineItems, "supplier", Enterprises.byID
Dereferencer.dereferenceAttr $scope.lineItems, "order", Orders.byID
$scope.bulk_order_form.$setPristine()
Expand All @@ -59,8 +60,6 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout,
$timeout ->
$scope.resetSelectFilters()

$scope.refreshData()

$scope.$watch 'bulk_order_form.$dirty', (newVal, oldVal) ->
if newVal == true
StatusMessage.display 'notice', t('js.unsaved_changes')
Expand Down Expand Up @@ -154,3 +153,5 @@ angular.module("admin.lineItems").controller 'LineItemsCtrl', ($scope, $timeout,
if lineItem.quantity > 0
lineItem.final_weight_volume = LineItems.pristineByID[lineItem.id].final_weight_volume * lineItem.quantity / LineItems.pristineByID[lineItem.id].quantity
$scope.weightAdjustedPrice(lineItem)

$scope.refreshData()
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ angular.module("admin.resources").factory 'Orders', ($q, OrderResource, RequestM
request = OrderResource.index params, (data) =>
@load(data)
(callback || angular.noop)(data)
RequestMonitor.load(request.$promise)
Copy link
Member

Choose a reason for hiding this comment

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

Is it really okay to remove this line? It doesn't seem necessary to remove, and many pages including the bulk order management use RequestMonitor.

Copy link
Member Author

Choose a reason for hiding this comment

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

That call was redundant. The only call of this method is in the line items controller and it uses the RequestMonitor already:

@all.$promise = request.$promise
@all

load: (data) ->
Expand Down
8 changes: 8 additions & 0 deletions app/assets/stylesheets/admin/components/ng-cloak.css.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// https://docs.angularjs.org/api/ng/directive/ngCloak
[ng-cloak],
[data-ng-cloak],
[x-ng-cloak],
.ng-cloak,
.x-ng-cloak {
display: none !important;
}
8 changes: 4 additions & 4 deletions app/views/spree/admin/orders/bulk_management.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@

%hr.divider.sixteen.columns.alpha.omega{ ng: { show: 'unitsVariantSelected()' } }

%div.sixteen.columns.alpha.omega#group_buy_calculation{ ng: { show: 'unitsVariantSelected()' } }
%div.sixteen.columns.alpha.omega#group_buy_calculation{ ng: { show: 'unitsVariantSelected()', cloak: true } }
%div.shared_resource{ :class => "four columns alpha" }
%span{ :class => 'three columns alpha' }
%input{ type: 'checkbox', :id => 'shared_resource', 'ng-model' => 'sharedResource'}
Expand Down Expand Up @@ -106,11 +106,11 @@
%h1
= t("admin.orders.bulk_management.loading")

%div{ :class => "sixteen columns alpha", 'ng-show' => '!RequestMonitor.loading && filteredLineItems.length == 0'}
%div{ class: "sixteen columns alpha", ng: { show: '!RequestMonitor.loading && filteredLineItems.length == 0', cloak: true } }
%h1#no_results
= t("admin.orders.bulk_management.no_results")

.margin-bottom-50{ 'ng-hide' => 'RequestMonitor.loading || filteredLineItems.length == 0' }
.margin-bottom-50{ ng: { hide: 'RequestMonitor.loading || filteredLineItems.length == 0', cloak: true } }
%form{ name: 'bulk_order_form' }
%table.index#listing_orders.bulk{ :class => "sixteen columns alpha", ng: { show: "initialized" } }
%thead
Expand Down Expand Up @@ -157,7 +157,7 @@
= t("admin.orders.bulk_management.ask")
%input{ :type => 'checkbox', 'ng-model' => "confirmDelete" }

%tr.line_item{ 'ng-repeat' => "line_item in filteredLineItems = ( lineItems | filter:quickSearch | selectFilter:supplierFilter:distributorFilter:orderCycleFilter | variantFilter:selectedUnitsProduct:selectedUnitsVariant:sharedResource | orderBy:sorting.predicate:sorting.reverse )", 'ng-class-even' => "'even'", 'ng-class-odd' => "'odd'", ng: { attr: { id: "li_{{line_item.id}}" } } }
%tr.line_item{ ng: { repeat: "line_item in filteredLineItems = ( lineItems | filter:quickSearch | selectFilter:supplierFilter:distributorFilter:orderCycleFilter | variantFilter:selectedUnitsProduct:selectedUnitsVariant:sharedResource | orderBy:sorting.predicate:sorting.reverse )", 'class-even' => "'even'", 'class-odd' => "'odd'", attr: { id: "li_{{line_item.id}}" } } }
%td.bulk
%input{ :type => "checkbox", :name => 'bulk', 'ng-model' => 'line_item.checked', 'ignore-dirty' => true }
%td.order_no{ 'ng-show' => 'columns.order_no.visible' } {{ line_item.order.number }}
Expand Down
66 changes: 30 additions & 36 deletions spec/features/admin/bulk_order_management_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@

context "listing orders" do
before :each do
login_to_admin_section
quick_login_as_admin
end

it "displays a message when number of line items is zero" do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
expect(page).to have_text 'No orders found.'
end

Expand All @@ -27,7 +27,7 @@
let!(:li3) { create(:line_item, order: o3 ) }

before :each do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
end

it "displays a list of line items" do
Expand All @@ -44,8 +44,7 @@
let!(:li2) { create(:line_item, order: o2, product: create(:product_with_option_types) ) }

before :each do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "displays a column for user's full name" do
Expand Down Expand Up @@ -92,8 +91,7 @@
let!(:li2) { create(:line_item, order: o2) }

before do
visit spree.admin_bulk_order_management_path
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "sorts by customer name when the customer name header is clicked" do
Expand Down Expand Up @@ -123,15 +121,15 @@

context "altering line item properties" do
before :each do
admin_user = quick_login_as_admin
quick_login_as_admin
end

context "tracking changes" do
let!(:o1) { create(:order_with_distributor, state: 'complete', completed_at: Time.zone.now ) }
let!(:li1) { create(:line_item, order: o1, :quantity => 5 ) }

before :each do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
end

it "adds the class 'ng-dirty' to input elements when value is altered" do
Expand All @@ -147,7 +145,7 @@

before :each do
li1.variant.update_attributes(on_hand: 1, on_demand: false)
visit '/admin/orders/bulk_management'
visit_bulk_order_management
end

context "when acceptable data is sent to the server" do
Expand Down Expand Up @@ -179,7 +177,7 @@

context "using page controls" do
before :each do
admin_user = quick_login_as_admin
quick_login_as_admin
end

let!(:p1) { create(:product_with_option_types, group_buy: true, group_buy_unit_size: 5000, variant_unit: "weight", variants: [create(:variant, unit_value: 1000)] ) }
Expand All @@ -191,7 +189,7 @@

context "modifying the weight/volume of a line item" do
it "price is altered" do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
toggle_columns "Weight/Volume", "Price"
within "tr#li_#{li1.id}" do
expect(page).to have_field "price", with: "50.00"
Expand All @@ -208,7 +206,7 @@

context "modifying the quantity of a line item" do
it "price is altered" do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
toggle_columns "Price"
within "tr#li_#{li1.id}" do
expect(page).to have_field "price", with: "#{format("%.2f",li1.price * 5)}"
Expand All @@ -220,7 +218,7 @@

context "modifying the quantity of a line item" do
it "weight/volume is altered" do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
toggle_columns "Weight/Volume"
within "tr#li_#{li1.id}" do
expect(page).to have_field "final_weight_volume", with: "#{li1.final_weight_volume.round}"
Expand All @@ -232,7 +230,7 @@

context "using column display toggle" do
it "shows a column display toggle button, which shows a list of columns when clicked" do
visit '/admin/orders/bulk_management'
visit_bulk_order_management

expect(page).to have_selector "th", :text => "NAME"
expect(page).to have_selector "th", text: I18n.t("admin.orders.bulk_management.order_date").upcase
Expand Down Expand Up @@ -261,8 +259,7 @@
let!(:li2) { create(:line_item, order: o1, product: create(:product, supplier: s2) ) }

before :each do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "displays a select box for producers, which filters line items by the selected supplier" do
Expand Down Expand Up @@ -300,8 +297,7 @@
let!(:li2) { create(:line_item, order: o2 ) }

before :each do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "displays a select box for distributors, which filters line items by the selected distributor" do
Expand All @@ -319,10 +315,8 @@
end

it "displays all line items when 'All' is selected from distributor filter" do
expect(page).to have_selector "tr#li_#{li1.id}"
expect(page).to have_selector "tr#li_#{li2.id}"
select2_select d1.name, from: "distributor_filter"
expect(page).to have_selector "tr#li_#{li1.id}"
expect(page).to have_no_selector "tr#li_#{li2.id}"
select2_select "All", from: "distributor_filter"
expect(page).to have_selector "tr#li_#{li1.id}"
Expand All @@ -340,8 +334,7 @@
let!(:li2) { create(:line_item, order: o2 ) }

before do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "displays a select box for order cycles, which filters line items by the selected order cycle" do
Expand Down Expand Up @@ -381,8 +374,7 @@
let!(:li2) { create(:line_item, order: o2, product: p2 ) }

before :each do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "allows filters to be used in combination" do
Expand Down Expand Up @@ -432,8 +424,7 @@
let!(:li3) { create(:line_item, order: o3 ) }

before :each do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "displays a quick search input" do
Expand Down Expand Up @@ -462,8 +453,7 @@
let!(:li4) { create(:line_item, order: o4, :quantity => 4 ) }

before :each do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "displays date fields for filtering orders, with default values set" do
Expand Down Expand Up @@ -535,8 +525,7 @@
let!(:li2) { create(:line_item, order: o2 ) }

before :each do
visit '/admin/orders/bulk_management'
wait_until { request_monitor_finished 'LineItemsCtrl' }
visit_bulk_order_management
end

it "displays a checkbox for each line item in the list" do
Expand Down Expand Up @@ -608,7 +597,7 @@
let!(:li2) { create(:line_item, order: o2 ) }

before :each do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
end

it "shows an edit button for line_items, which takes the user to the standard edit page for the order" do
Expand Down Expand Up @@ -642,7 +631,7 @@
let!(:li2) { create(:line_item, order: o2 ) }

before :each do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
end

it "removes a line item when the relevant delete button is clicked" do
Expand All @@ -652,7 +641,7 @@
end
expect(page).to have_no_selector "a.delete-line-item", :count => 2
expect(page).to have_selector "a.delete-line-item", :count => 1
visit '/admin/orders/bulk_management'
visit_bulk_order_management
expect(page).to have_selector "a.delete-line-item", :count => 1
end
end
Expand All @@ -670,7 +659,7 @@
let!(:li4) { create(:line_item, order: o2, variant: v3, quantity: 1, max_quantity: 3 ) }

before :each do
visit '/admin/orders/bulk_management'
visit_bulk_order_management
within "tr#li_#{li3.id}" do
find("a", text: li3.product.name + ": " + li3.variant.options_text).click
end
Expand Down Expand Up @@ -746,13 +735,18 @@
end

it "shows only line item from orders that I distribute, and not those that I supply" do
visit '/admin/orders/bulk_management'
visit_bulk_order_management

expect(page).to have_selector "tr#li_#{line_item_distributed.id}", :visible => true
expect(page).to have_no_selector "tr#li_#{line_item_not_distributed.id}", :visible => true
end
end

def visit_bulk_order_management
visit spree.admin_bulk_order_management_path
expect(page).to have_no_text 'Loading orders'
end

def select_date(date)
# Wait for datepicker to open and be associated to the datepicker trigger.
expect(page).to have_selector("#ui-datepicker-div")
Expand Down