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

Load closed shops in a separate request on /shops page #5156

Merged
merged 4 commits into from
Apr 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,13 @@ Darkswarm.controller "EnterprisesCtrl", ($scope, $rootScope, $timeout, $location
$scope.show_closed = false
$scope.filtersActive = false
$scope.distanceMatchesShown = false
$scope.closed_shops_loading = false
$scope.closed_shops_loaded = false

$scope.$watch "query", (query)->
$scope.resetSearch(query)

$scope.resetSearch = (query) ->
Enterprises.flagMatching query
Search.search query
$rootScope.$broadcast 'enterprisesChanged'
Expand All @@ -19,6 +24,7 @@ Darkswarm.controller "EnterprisesCtrl", ($scope, $rootScope, $timeout, $location
$timeout ->
Enterprises.calculateDistance query, $scope.firstNameMatch()
$rootScope.$broadcast 'enterprisesChanged'
$scope.closed_shops_loading = false

$timeout ->
if $location.search()['show_closed']?
Expand Down Expand Up @@ -73,6 +79,12 @@ Darkswarm.controller "EnterprisesCtrl", ($scope, $rootScope, $timeout, $location
undefined

$scope.showClosedShops = ->
unless $scope.closed_shops_loaded
$scope.closed_shops_loading = true
$scope.closed_shops_loaded = true
Enterprises.loadClosedEnterprises().then ->
$scope.resetSearch($scope.query)

$scope.show_closed = true
$location.search('show_closed', '1')

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Darkswarm.controller "HubNodeCtrl", ($scope, HashNavigation, CurrentHub, $http,
$scope.shopfront_loading = true
$scope.toggle_tab(event)

$http.get("/api/enterprises/" + $scope.hub.id + "/shopfront")
$http.get("/api/shops/" + $scope.hub.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

.success (data) ->
$scope.shopfront_loading = false
$scope.hub = data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Darkswarm.controller "ProducerNodeCtrl", ($scope, HashNavigation, $anchorScroll,
$scope.shopfront_loading = true
$scope.toggle_tab(event)

$http.get("/api/enterprises/" + $scope.producer.id + "/shopfront")
$http.get("/api/shops/" + $scope.producer.id)
.success (data) ->
$scope.shopfront_loading = false
$scope.producer = data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Darkswarm.factory "EnterpriseModal", ($modal, $rootScope, $http)->
scope = $rootScope.$new(true) # Spawn an isolate to contain the enterprise
scope.embedded_layout = window.location.search.indexOf("embedded_shopfront=true") != -1

$http.get("/api/enterprises/" + enterprise.id + "/shopfront").success (data) ->
$http.get("/api/shops/" + enterprise.id).success (data) ->
scope.enterprise = data
$modal.open(templateUrl: "enterprise_modal.html", scope: scope)
.error (data) ->
Expand Down
19 changes: 14 additions & 5 deletions app/assets/javascripts/darkswarm/services/enterprises.js.coffee
Original file line number Diff line number Diff line change
@@ -1,27 +1,30 @@
Darkswarm.factory 'Enterprises', (enterprises, CurrentHub, Taxons, Dereferencer, Matcher, Geo, $rootScope) ->
Darkswarm.factory 'Enterprises', (enterprises, ShopsResource, CurrentHub, Taxons, Dereferencer, Matcher, Geo, $rootScope) ->
new class Enterprises
enterprises: []
enterprises_by_id: {}

constructor: ->
# Populate Enterprises.enterprises from json in page.
@enterprises = enterprises
@initEnterprises(enterprises)

initEnterprises: (enterprises) ->
# Map enterprises to id/object pairs for lookup.
for enterprise in enterprises
@enterprises.push enterprise
@enterprises_by_id[enterprise.id] = enterprise

# Replace enterprise and taxons ids with actual objects.
@dereferenceEnterprises()
@dereferenceEnterprises(enterprises)

@producers = @enterprises.filter (enterprise)->
enterprise.category in ["producer_hub", "producer_shop", "producer"]
@hubs = @enterprises.filter (enterprise)->
enterprise.category in ["hub", "hub_profile", "producer_hub", "producer_shop"]

dereferenceEnterprises: ->
dereferenceEnterprises: (enteprises) ->
if CurrentHub.hub?.id
CurrentHub.hub = @enterprises_by_id[CurrentHub.hub.id]
for enterprise in @enterprises
for enterprise in enterprises
@dereferenceEnterprise enterprise

dereferenceEnterprise: (enterprise) ->
Expand All @@ -42,6 +45,12 @@ Darkswarm.factory 'Enterprises', (enterprises, CurrentHub, Taxons, Dereferencer,
for enterprise in new_enterprises
@enterprises_by_id[enterprise.id] = enterprise

loadClosedEnterprises: ->
request = ShopsResource.closed_shops {}, (data) =>
@initEnterprises(data)

request.$promise

flagMatching: (query) ->
for enterprise in @enterprises
enterprise.matches_name_query = if query? && query.length > 0
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Darkswarm.factory 'ShopsResource', ($resource) ->
$resource('/api/shops/:id.json', {}, {
'closed_shops':
method: 'GET'
isArray: true
url: '/api/shops/closed_shops.json'
})
5 changes: 5 additions & 0 deletions app/assets/stylesheets/darkswarm/hubs.css.scss
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,10 @@

.more-controls {
text-align: center;

.spinner {
height: 2.25em;
margin-right: 0.5em;
}
}
}
7 changes: 0 additions & 7 deletions app/controllers/api/enterprises_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ class EnterprisesController < BaseController
before_filter :override_sells, only: [:create, :update]
before_filter :override_visible, only: [:create, :update]
respond_to :json
skip_authorization_check only: [:shopfront]

def create
authorize! :create, Enterprise
Expand Down Expand Up @@ -42,12 +41,6 @@ def update_image
end
end

def shopfront
enterprise = Enterprise.find_by_id(params[:id])

render text: Api::EnterpriseShopfrontSerializer.new(enterprise).to_json, status: :ok
end

private

def override_owner
Expand Down
27 changes: 27 additions & 0 deletions app/controllers/api/shops_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# frozen_string_literal: true

module Api
class ShopsController < BaseController
respond_to :json
skip_authorization_check only: [:show, :closed_shops]

def show
enterprise = Enterprise.find_by_id(params[:id])

render text: Api::EnterpriseShopfrontSerializer.new(enterprise).to_json, status: :ok
end

def closed_shops
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be a filter specified by URL query param for an API::ShopsController#index so we follow Rails REST conventions. But I want to discuss this in another PR and without any rush. This is awesome as it is.

@active_distributor_ids = []
@earliest_closing_times = []
Comment on lines +15 to +16
Copy link
Member

Choose a reason for hiding this comment

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

What do we need these for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are used in OpenFoodNetwork::EnterpriseInjectionData.new a few lines later, and will trigger big queries (that we don't need in the case of closed shops) unless these two ivars are already set.


serialized_closed_shops = ActiveModel::ArraySerializer.new(
ShopsListService.new.closed_shops,
each_serializer: Api::EnterpriseSerializer,
data: OpenFoodNetwork::EnterpriseInjectionData.new
)

render json: serialized_closed_shops
end
end
end
9 changes: 1 addition & 8 deletions app/controllers/shops_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,6 @@ class ShopsController < BaseController
before_filter :enable_embedded_shopfront

def index
@enterprises = Enterprise
.activated
.visible
.is_distributor
.includes(address: [:state, :country])
.includes(:properties)
.includes(supplied_products: :properties)
.all
@enterprises = ShopsListService.new.open_shops
end
end
23 changes: 23 additions & 0 deletions app/services/shops_list_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# frozen_string_literal: true

class ShopsListService
def open_shops
shops_list.ready_for_checkout.all
end

def closed_shops
shops_list.not_ready_for_checkout.all
end

private

def shops_list
Enterprise
.activated
.visible
.is_distributor
.includes(address: [:state, :country])
.includes(:properties)
.includes(supplied_products: :properties)
end
end
11 changes: 7 additions & 4 deletions app/views/shops/_hubs.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,11 @@
%a{href: "", "ng-click" => "showDistanceMatches()"}
= t :hubs_distance_filter, location: "{{ nameMatchesFiltered[0].name }}"
.more-controls
%a.button{href: "", ng: {click: "showClosedShops()", show: "!show_closed"}}
= t '.show_closed_shops'
%a.button{href: "", ng: {click: "hideClosedShops()", show: "show_closed"}}
= t '.hide_closed_shops'
%img.spinner.text-center{ng: {show: "closed_shops_loading"}, src: "/assets/spinning-circles.svg" }
%span{ng: {if: "!show_closed", cloak: true}}
%a.button{href: "", ng: {click: "showClosedShops()"}}
= t '.show_closed_shops'
%span{ng: {if: "show_closed", cloak: true}}
%a.button{href: "", ng: {click: "hideClosedShops()"}}
= t '.hide_closed_shops'
%a.button{href: main_app.map_path}= t '.show_on_map'
6 changes: 4 additions & 2 deletions config/routes/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@

resource :logo, only: [:destroy]
resource :promo_image, only: [:destroy]
end

member do
get :shopfront
resources :shops, only: [:show] do
collection do
get :closed_shops
end
end

Expand Down
24 changes: 0 additions & 24 deletions spec/controllers/api/enterprises_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,29 +81,5 @@ module Api
end
end
end

context "as a non-authenticated user" do
let!(:hub) {
create(:distributor_enterprise, with_payment_and_shipping: true, name: 'Shopfront Test Hub')
}
let!(:producer) { create(:supplier_enterprise, name: 'Shopfront Test Producer') }
let!(:category) { create(:taxon, name: 'Fruit') }
let!(:product) { create(:product, supplier: producer, primary_taxon: category ) }
let!(:relationship) { create(:enterprise_relationship, parent: hub, child: producer) }

before do
allow(controller).to receive(:spree_current_user) { nil }
end

describe "fetching shopfronts data" do
it "returns data for an enterprise" do
spree_get :shopfront, id: producer.id, format: :json

expect(json_response['name']).to eq 'Shopfront Test Producer'
expect(json_response['hubs'][0]['name']).to eq 'Shopfront Test Hub'
expect(json_response['supplied_taxons'][0]['name']).to eq 'Fruit'
end
end
end
end
end
46 changes: 46 additions & 0 deletions spec/controllers/api/shops_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# frozen_string_literal: true

require 'spec_helper'

module Api
describe ShopsController, type: :controller do
include AuthenticationWorkflow
render_views

context "as a non-authenticated user" do
let!(:hub) {
create(:distributor_enterprise, with_payment_and_shipping: true, name: 'Shopfront Test Hub')
}
let!(:producer) { create(:supplier_enterprise, name: 'Shopfront Test Producer') }
let!(:category) { create(:taxon, name: 'Fruit') }
let!(:product) { create(:product, supplier: producer, primary_taxon: category ) }
let!(:relationship) { create(:enterprise_relationship, parent: hub, child: producer) }
let!(:closed_hub1) { create(:distributor_enterprise) }
let!(:closed_hub2) { create(:distributor_enterprise) }

before do
allow(controller).to receive(:spree_current_user) { nil }
end

describe "#show" do
it "returns shopfront data for an enterprise" do
spree_get :show, id: producer.id

expect(json_response['name']).to eq 'Shopfront Test Producer'
expect(json_response['hubs'][0]['name']).to eq 'Shopfront Test Hub'
expect(json_response['supplied_taxons'][0]['name']).to eq 'Fruit'
end
end

describe "#closed_shops" do
it "returns data for all closed shops" do
spree_get :closed_shops, nil

expect(json_response).not_to match hub.name
expect(json_response[0]['id']).to eq closed_hub1.id
expect(json_response[1]['id']).to eq closed_hub2.id
end
end
end
end
end
6 changes: 1 addition & 5 deletions spec/controllers/shops_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
include WebHelper
render_views

let!(:distributor) { create(:distributor_enterprise) }

before do
allow(OpenFoodNetwork::EnterpriseInjectionData).to receive(:active_distributor_ids) { [distributor.id] }
end
let!(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) }

it 'renders distributed product properties' do
product_property = create(:property, presentation: 'eggs')
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
describe "Enterprises service", ->
Enterprises = null
Enterprises = $rootScope = null
CurrentHubMock = {}
Geo =
OK: 'ok'
Expand Down Expand Up @@ -36,15 +36,17 @@ describe "Enterprises service", ->
angular.module('Darkswarm').value('enterprises', enterprises)
angular.module('Darkswarm').value('taxons', taxons)

inject ($injector)->
inject ($injector, _$rootScope_)->
Enterprises = $injector.get("Enterprises")
$rootScope = _$rootScope_

it "stores enterprises as id/object pairs", ->
expect(Enterprises.enterprises_by_id["1"]).toBe enterprises[0]
expect(Enterprises.enterprises_by_id["2"]).toBe enterprises[1]

it "stores enterprises as an array", ->
expect(Enterprises.enterprises).toBe enterprises
$rootScope.$digest()
expect(Enterprises.enterprises).toEqual enterprises

it "puts the same objects in enterprises and enterprises_by_id", ->
expect(Enterprises.enterprises[0]).toBe Enterprises.enterprises_by_id["1"]
Expand Down