Merge pull request #4057 from luisramos0/remove_spree_api_2

Remove dependency to spree_api - step 2 - routes and views

Gemfile

@@ -20,7 +20,6 @@ gem 'pg'
 # OFN-maintained and patched version of Spree v2.0.4. See
 # https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-2.0-upgrade
 # for details.
-gem 'spree_api', github: 'openfoodfoundation/spree', branch: '2-0-4-stable'
 gem 'spree_backend', github: 'openfoodfoundation/spree', branch: '2-0-4-stable'
 gem 'spree_core', github: 'openfoodfoundation/spree', branch: '2-0-4-stable'
 

Gemfile.lock

@@ -806,7 +806,6 @@ DEPENDENCIES
   simple_form!
   simplecov
   spinjs-rails
-  spree_api!
   spree_backend!
   spree_core!
   spree_i18n!

app/assets/javascripts/admin/bulk_product_update.js.coffee

@@ -1,4 +1,4 @@
-angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, SpreeApiAuth, Columns, tax_categories, RequestMonitor) ->
+angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, Columns, tax_categories, RequestMonitor) ->
   $scope.StatusMessage = StatusMessage
 
   $scope.columns = Columns.columns
@@ -39,12 +39,7 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
   $scope.DisplayProperties = DisplayProperties
 
   $scope.initialise = ->
-    SpreeApiAuth.authorise()
-    .then ->
-      $scope.spree_api_key_ok = true
-      $scope.fetchProducts()
-    .catch (message) ->
-      $scope.api_error_msg = message
+    $scope.fetchProducts()
 
   $scope.$watchCollection '[query, producerFilter, categoryFilter, importDateFilter, per_page]', ->
     $scope.page = 1 # Reset page when changing filters for new search

app/assets/javascripts/admin/index_utils/services/spree_api_auth.js.coffee

@@ -1,16 +0,0 @@
-angular.module("admin.indexUtils").factory "SpreeApiAuth", ($q, $http, SpreeApiKey) ->
-  new class SpreeApiAuth
-    authorise: ->
-      deferred = $q.defer()
-
-      $http.get("/api/users/authorise_api?token=" + SpreeApiKey)
-      .success (response) ->
-        if response?.success == "Use of API Authorised"
-          $http.defaults.headers.common["X-Spree-Token"] = SpreeApiKey
-          deferred.resolve()
-
-      .error (response) ->
-        error = response?.error || t('js.unauthorized')
-        deferred.reject(error)
-
-      deferred.promise

app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee

@@ -1,4 +1,4 @@
-angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", ($scope, $http, $timeout, Indexer, Columns, Views, SpreeApiAuth, PagedFetcher, StatusMessage, RequestMonitor, hubs, producers, hubPermissions, InventoryItems, VariantOverrides, DirtyVariantOverrides) ->
+angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", ($scope, $http, $timeout, Indexer, Columns, Views, PagedFetcher, StatusMessage, RequestMonitor, hubs, producers, hubPermissions, InventoryItems, VariantOverrides, DirtyVariantOverrides) ->
   $scope.hubs = Indexer.index hubs
   $scope.hub_id = if hubs.length == 1 then hubs[0].id else null
   $scope.products = []
@@ -39,13 +39,7 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl",
     $scope.producerFilter != 0 || $scope.query != ''
 
   $scope.initialise = ->
-    SpreeApiAuth.authorise()
-    .then ->
-      $scope.spree_api_key_ok = true
-      $scope.fetchProducts()
-    .catch (message) ->
-      $scope.api_error_msg = message
-
+    $scope.fetchProducts()
 
   $scope.fetchProducts = ->
     url = "/api/products/overridable?page=::page::;per_page=100"

app/controllers/api/base_controller.rb

@@ -1,16 +1,44 @@
 # Base controller for OFN's API
-# Includes the minimum machinery required by ActiveModelSerializers
+require_dependency 'spree/api/controller_setup'
+
 module Api
-  class BaseController < Spree::Api::BaseController
-    # Need to include these because Spree::Api::BaseContoller inherits
-    # from ActionController::Metal rather than ActionController::Base
-    # and they are required by ActiveModelSerializers
+  class BaseController < ActionController::Metal
+    include Spree::Api::ControllerSetup
+    include Spree::Core::ControllerHelpers::SSL
+    include ::ActionController::Head
+
+    respond_to :json
+
+    attr_accessor :current_api_user
+
+    before_filter :set_content_type
+    before_filter :authenticate_user
+    after_filter  :set_jsonp_format
+
+    rescue_from Exception, with: :error_during_processing
+    rescue_from CanCan::AccessDenied, with: :unauthorized
+    rescue_from ActiveRecord::RecordNotFound, with: :not_found
+
+    helper Spree::Api::ApiHelpers
+
+    ssl_allowed
+
+    # Include these because we inherit from ActionController::Metal
+    #   rather than ActionController::Base and these are required for AMS
     include ActionController::Serialization
     include ActionController::UrlFor
     include Rails.application.routes.url_helpers
+
     use_renderers :json
     check_authorization
 
+    def set_jsonp_format
+      return unless params[:callback] && request.get?
+
+      self.response_body = "#{params[:callback]}(#{response_body})"
+      headers["Content-Type"] = 'application/javascript'
+    end
+
     def respond_with_conflict(json_hash)
       render json: json_hash, status: :conflict
     end
@@ -19,16 +47,62 @@ module Api
 
     # Use logged in user (spree_current_user) for API authentication (current_api_user)
     def authenticate_user
-      @current_api_user = try_spree_current_user
-      super
+      return if @current_api_user = try_spree_current_user
+      if api_key.blank?
+        # An anonymous user
+        @current_api_user = Spree.user_class.new
+        return
+      end
+
+      return if @current_api_user = Spree.user_class.find_by_spree_api_key(api_key.to_s)
+
+      invalid_api_key
     end
 
-    # Allows API access without authentication, but only for OFN controllers which inherit
-    # from Api::BaseController. @current_api_user will now initialize an empty Spree::User
-    # unless one is present. We now also apply devise's `check_authorization`. See here for
-    # details: https://github.com/CanCanCommunity/cancancan/wiki/Ensure-Authorization
-    def requires_authentication?
-      false
+    def set_content_type
+      content_type = case params[:format]
+                     when "json"
+                       "application/json"
+                     when "xml"
+                       "text/xml"
+                     end
+      headers["Content-Type"] = content_type
+    end
+
+    def error_during_processing(exception)
+      render(text: { exception: exception.message }.to_json,
+             status: :unprocessable_entity) && return
+    end
+
+    def current_ability
+      Spree::Ability.new(current_api_user)
+    end
+
+    def api_key
+      request.headers["X-Spree-Token"] || params[:token]
+    end
+    helper_method :api_key
+
+    def invalid_resource!(resource)
+      @resource = resource
+      render(json: { error: I18n.t(:invalid_resource, scope: "spree.api"),
+                     errors: @resource.errors },
+             status: :unprocessable_entity)
+    end
+
+    def invalid_api_key
+      render(json: { error: I18n.t(:invalid_api_key, key: api_key, scope: "spree.api") },
+             status: :unauthorized) && return
+    end
+
+    def unauthorized
+      render(json: { error: I18n.t(:unauthorized, scope: "spree.api") },
+             status: :unauthorized) && return
+    end
+
+    def not_found
+      render(json: { error: I18n.t(:resource_not_found, scope: "spree.api") },
+             status: :not_found) && return
     end
   end
 end

app/controllers/api/products_controller.rb

@@ -47,7 +47,6 @@ module Api
       render json: @product, serializer: Api::Admin::ProductSerializer, status: 204
     end
 
-    # TODO: This should be named 'managed'. Is the action above used? Maybe we should remove it.
     def bulk_products
       product_query = OpenFoodNetwork::Permissions.new(current_api_user).
         editable_products.merge(product_scope)
@@ -94,10 +93,13 @@ module Api
 
     private
 
-    # Copied and modified from SpreeApi::BaseController to allow
-    # enterprise users to access inactive products
+    def find_product(id)
+      product_scope.find_by_permalink!(id.to_s)
+    rescue ActiveRecord::RecordNotFound
+      product_scope.find(id)
+    end
+
     def product_scope
-      # This line modified
       if current_api_user.has_spree_role?("admin") || current_api_user.enterprises.present?
         scope = Spree::Product
         if params[:show_deleted]

app/controllers/spree/admin/users_controller.rb

@@ -58,14 +58,14 @@ module Spree
 
       def generate_api_key
         if @user.generate_spree_api_key!
-          flash[:success] = Spree.t('api.key_generated')
+          flash[:success] = t('spree.api.key_generated')
         end
         redirect_to edit_admin_user_path(@user)
       end
 
       def clear_api_key
         if @user.clear_spree_api_key!
-          flash[:success] = Spree.t('api.key_cleared')
+          flash[:success] = t('spree.api.key_cleared')
         end
         redirect_to edit_admin_user_path(@user)
       end

app/controllers/spree/api/base_controller.rb

@@ -1,130 +0,0 @@
-require_dependency 'spree/api/controller_setup'
-
-module Spree
-  module Api
-    class BaseController < ActionController::Metal
-      include Spree::Api::ControllerSetup
-      include Spree::Core::ControllerHelpers::SSL
-      include ::ActionController::Head
-
-      self.responder = Spree::Api::Responders::AppResponder
-
-      respond_to :json
-
-      attr_accessor :current_api_user
-
-      before_filter :set_content_type
-      before_filter :check_for_user_or_api_key, :if => :requires_authentication?
-      before_filter :authenticate_user
-      after_filter  :set_jsonp_format
-
-      rescue_from Exception, :with => :error_during_processing
-      rescue_from CanCan::AccessDenied, :with => :unauthorized
-      rescue_from ActiveRecord::RecordNotFound, :with => :not_found
-
-      helper Spree::Api::ApiHelpers
-
-      ssl_allowed
-
-      def set_jsonp_format
-        if params[:callback] && request.get?
-          self.response_body = "#{params[:callback]}(#{response_body})"
-          headers["Content-Type"] = 'application/javascript'
-        end
-      end
-
-      def map_nested_attributes_keys(klass, attributes)
-        nested_keys = klass.nested_attributes_options.keys
-        attributes.inject({}) do |h, (k, v)|
-          key = nested_keys.include?(k.to_sym) ? "#{k}_attributes" : k
-          h[key] = v
-          h
-        end.with_indifferent_access
-      end
-
-      private
-
-      def set_content_type
-        content_type = case params[:format]
-                       when "json"
-                         "application/json"
-                       when "xml"
-                         "text/xml"
-                       end
-        headers["Content-Type"] = content_type
-      end
-
-      def check_for_user_or_api_key
-        # User is already authenticated with Spree, make request this way instead.
-        return true if @current_api_user = try_spree_current_user ||
-                                           !requires_authentication?
-
-        return if api_key.present?
-        render("spree/api/errors/must_specify_api_key", status: :unauthorized) && return
-      end
-
-      def authenticate_user
-        return if @current_api_user
-
-        if requires_authentication? || api_key.present?
-          unless @current_api_user = Spree.user_class.find_by_spree_api_key(api_key.to_s)
-            render("spree/api/errors/invalid_api_key", status: :unauthorized) && return
-          end
-        else
-          # An anonymous user
-          @current_api_user = Spree.user_class.new
-        end
-      end
-
-      def unauthorized
-        render("spree/api/errors/unauthorized", status: :unauthorized) && return
-      end
-
-      def error_during_processing(exception)
-        render(text: { exception: exception.message }.to_json,
-               status: :unprocessable_entity) && return
-      end
-
-      def requires_authentication?
-        true
-      end
-
-      def not_found
-        render("spree/api/errors/not_found", status: :not_found) && return
-      end
-
-      def current_ability
-        Spree::Ability.new(current_api_user)
-      end
-
-      def invalid_resource!(resource)
-        @resource = resource
-        render "spree/api/errors/invalid_resource", status: :unprocessable_entity
-      end
-
-      def api_key
-        request.headers["X-Spree-Token"] || params[:token]
-      end
-      helper_method :api_key
-
-      def find_product(id)
-        product_scope.find_by_permalink!(id.to_s)
-      rescue ActiveRecord::RecordNotFound
-        product_scope.find(id)
-      end
-
-      def product_scope
-        if current_api_user.has_spree_role?("admin")
-          scope = Product
-          if params[:show_deleted]
-            scope = scope.with_deleted
-          end
-        else
-          scope = Product.active
-        end
-
-        scope.includes(:master)
-      end
-    end
-  end
-end

app/controllers/spree/api/users_controller.rb

@@ -1,7 +0,0 @@
-module Spree
-  module Api
-    class UsersController < Spree::Api::BaseController
-      respond_to :json
-    end
-  end
-end

app/views/spree/admin/products/index/_indicators.html.haml

@@ -1,6 +1,3 @@
-%div{ 'ng-show' => '!spree_api_key_ok' }
-  {{ api_error_msg }}
-
 %div.sixteen.columns.alpha#loading{ 'ng-if' => 'RequestMonitor.loading' }
   %br
   %img.spinner{ src: "/assets/spinning-circles.svg" }

app/views/spree/admin/shared/_routes.html.erb

@@ -3,6 +3,6 @@
     :variants_search       => spree.admin_search_variants_path(:format => 'json'),
     :taxons_search         => main_app.api_taxons_path(:format => 'json'),
     :user_search           => spree.admin_search_users_path(:format => 'json'),
-    :orders_api            => spree.api_orders_path(:format => 'json')
+    :orders_api            => main_app.api_orders_path
   }.to_json %>;
 </script>

app/views/spree/admin/taxonomies/edit.haml

@@ -17,7 +17,7 @@
       = label_tag nil, t("spree.tree")
       %br/
       :javascript
-        Spree.routes.taxonomy_taxons_path = "#{spree.api_taxonomy_taxons_path(@taxonomy)}";
+        Spree.routes.taxonomy_taxons_path = "#{main_app.api_taxonomy_taxons_path(@taxonomy)}";
         Spree.routes.admin_taxonomy_taxons_path = "#{spree.admin_taxonomy_taxons_path(@taxonomy)}";
       #taxonomy_tree.tree
     #progress{style: "display:none;"}

app/views/spree/admin/users/_api_fields.html.haml

@@ -0,0 +1,18 @@
+%fieldset.omega.six.columns
+  %legend= t('spree.api.access')
+  - if @user.spree_api_key.present?
+    .field
+      = label_tag t('spree.api.key')
+      = ":"
+      = @user.spree_api_key
+    .filter-actions.actions
+      = form_tag spree.clear_api_key_admin_user_path(@user), method: :put do
+        = button t('spree.api.clear_key'), 'icon-trash'
+      %span.or= t(:or)
+      = form_tag spree.generate_api_key_admin_user_path(@user), method: :put do
+        = button t('spree.api.regenerate_key'), 'icon-refresh'
+  - else
+    .no-objects-found= t('spree.api.no_key')
+    .filter-actions.actions
+      = form_tag spree.generate_api_key_admin_user_path(@user), method: :put do
+        = button t('spree.api.generate_key'), 'icon-key'

app/views/spree/admin/users/edit.html.haml

@@ -13,3 +13,5 @@
       = render partial: "form", locals: { f: f }
       %div{"data-hook" => "admin_user_edit_form_button"}
         = render partial: "spree/admin/shared/edit_resource_links"
+
+= render partial: 'spree/admin/users/api_fields'

app/views/spree/api/users/authorise_api.v1.rabl

@@ -1,2 +0,0 @@
-object false
-node(:success) { "Use of API Authorised" }

config/locales/en.yml

@@ -3339,3 +3339,19 @@ See the %{link} to find out more about %{sitename}'s features and to start using
         allow_charges?: "Allow Charges?"
     localized_number:
       invalid_format: has an invalid format. Please enter a number.
+    api:
+      invalid_api_key: "Invalid API key (%{key}) specified."
+      unauthorized: "You are not authorized to perform that action."
+      invalid_resource: "Invalid resource. Please fix errors and try again."
+      resource_not_found: "The resource you were looking for could not be found."
+      access: "API Access"
+      key: "Key"
+      clear_key: "Clear key"
+      regenerate_key: "Regenerate Key"
+      no_key: "No key"
+      generate_key: "Generate API key"
+      key_generated: "Key generated"
+      key_cleared: "Key cleared"
+      shipment:
+        cannot_ready: "Cannot ready shipment."
+      invalid_taxonomy_id: "Invalid taxonomy id."

config/routes/api.rb

@@ -16,8 +16,6 @@ Openfoodnetwork::Application.routes.draw do
     resources :variants, :only => [:index]
 
     resources :orders, only: [:index, :show] do
-      get :managed, on: :collection
-
       resources :shipments, :only => [:create, :update] do
         member do
           put :ready
@@ -30,8 +28,6 @@ Openfoodnetwork::Application.routes.draw do
 
     resources :enterprises do
       post :update_image, on: :member
-      get :managed, on: :collection
-      get :accessible, on: :collection
 
       resource :logo, only: [:destroy]
       resource :promo_image, only: [:destroy]
@@ -42,9 +38,6 @@ Openfoodnetwork::Application.routes.draw do
     end
 
     resources :order_cycles do
-      get :managed, on: :collection
-      get :accessible, on: :collection
-
       get :products, on: :member
       get :taxons, on: :member
       get :properties, on: :member

config/routes/spree.rb

@@ -51,12 +51,6 @@ Spree::Core::Engine.routes.prepend do
 
   resources :credit_cards
 
-  namespace :api, :defaults => { :format => 'json' } do
-    resources :users do
-      get :authorise_api, on: :collection
-    end
-  end
-
   namespace :admin do
     get '/search/known_users' => "search#known_users", :as => :search_known_users
     get '/search/customers' => 'search#customers', :as => :search_customers

lib/spree/api/controller_setup.rb

@@ -1,5 +1,3 @@
-require 'spree/api/responders'
-
 module Spree
   module Api
     module ControllerSetup
@@ -24,7 +22,6 @@ module Spree
           prepend_view_path Rails.root + "app/views"
           append_view_path File.expand_path("../../../app/views", File.dirname(__FILE__))
 
-          self.responder = Spree::Api::Responders::AppResponder
           respond_to :json
         end
       end

spec/controllers/api/base_controller_spec.rb

@@ -1,8 +1,10 @@
 require 'spec_helper'
 
-describe Spree::Api::BaseController do
+describe Api::BaseController do
   render_views
-  controller(Spree::Api::BaseController) do
+  controller(Api::BaseController) do
+    skip_authorization_check only: :index
+
     def index
       render text: { "products" => [] }.to_json
     end
@@ -23,13 +25,15 @@ describe Spree::Api::BaseController do
     end
   end
 
-  context "cannot make a request to the API" do
+  context "can make an anonymous request to the API" do
     it "without an API key" do
       api_get :index
-      expect(json_response).to eq( "error" => "You must specify an API key." )
-      expect(response.status).to eq(401)
+      expect(json_response["products"]).to eq []
+      expect(response.status).to eq(200)
     end
+  end
 
+  context "cannot make a request to the API" do
     it "with an invalid API key" do
       request.env["X-Spree-Token"] = "fake_key"
       get :index, {}
@@ -46,19 +50,15 @@ describe Spree::Api::BaseController do
   it 'handles exceptions' do
     expect(subject).to receive(:authenticate_user).and_return(true)
     expect(subject).to receive(:index).and_raise(Exception.new("no joy"))
-    get :index, token: "fake_key"
+    get :index
     expect(json_response).to eq( "exception" => "no joy" )
   end
 
-  it "maps symantec keys to nested_attributes keys" do
-    klass = double(nested_attributes_options: { line_items: {},
-                                                bill_address: {} })
-    attributes = { 'line_items' => { id: 1 },
-                   'bill_address' => { id: 2 },
-                   'name' => 'test order' }
-
-    mapped = subject.map_nested_attributes_keys(klass, attributes)
-    expect(mapped.key?('line_items_attributes')).to be_truthy
-    expect(mapped.key?('name')).to be_truthy
+  it 'handles record not found' do
+    expect(subject).to receive(:authenticate_user).and_return(true)
+    expect(subject).to receive(:index).and_raise(ActiveRecord::RecordNotFound.new)
+    get :index
+    expect(json_response).to eq( "error" => "The resource you were looking for could not be found." )
+    expect(response.status).to eq(404)
   end
 end

spec/controllers/user_registrations_controller_spec.rb

@@ -49,12 +49,14 @@ describe UserRegistrationsController, type: :controller do
     end
 
     it "sets user.locale from cookie on create" do
+      original_i18n_locale = I18n.locale
       original_locale_cookie = cookies[:locale]
+
       cookies[:locale] = "pt"
-
       xhr :post, :create, spree_user: user_params, use_route: :spree
-
       expect(assigns[:user].locale).to eq("pt")
+
+      I18n.locale = original_i18n_locale
       cookies[:locale] = original_locale_cookie
     end
   end

spec/features/admin/users_spec.rb

@@ -24,7 +24,7 @@ feature "Managing users" do
           click_link "users_email_title"
         end
 
-        it "should be able to list users with order email asc" do
+        it "should list users with order email asc" do
           expect(page).to have_css('table#listing_users')
           within("table#listing_users") do
             expect(page).to have_content("a@example.com")
@@ -32,7 +32,7 @@ feature "Managing users" do
           end
         end
 
-        it "should be able to list users with order email desc" do
+        it "should list users with order email desc" do
           click_link "users_email_title"
           within("table#listing_users") do
             expect(page).to have_content("a@example.com")
@@ -57,7 +57,7 @@ feature "Managing users" do
           click_link("a@example.com")
         end
 
-        it "should let me edit the user password" do
+        it "should allow editing the user password" do
           fill_in "user_password", :with => "welcome"
           fill_in "user_password_confirmation", :with => "welcome"
           click_button "Update"
@@ -71,6 +71,23 @@ feature "Managing users" do
 
           expect(page).to have_content("The account will be updated once the new email is confirmed.")
         end
+
+        it "should allow to generate, regenarate and clear the user api key", js: true do
+          user = Spree::User.find_by_email("a@example.com")
+          expect(page).to have_content "NO KEY"
+
+          click_button "Generate API key"
+          first_user_api_key = user.reload.spree_api_key
+          expect(page).to have_content first_user_api_key
+
+          click_button "Regenerate Key"
+          second_user_api_key = user.reload.spree_api_key
+          expect(page).to have_content second_user_api_key
+          expect(second_user_api_key).not_to eq first_user_api_key
+
+          click_button "Clear key"
+          expect(page).to have_content "NO KEY"
+        end
       end
     end
 

spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee

@@ -272,13 +272,9 @@ describe "AdminProductEditCtrl", ->
 
   describe "loading data upon initialisation", ->
     it "gets a list of producers and then resets products with a list of data", ->
-      $httpBackend.expectGET("/api/users/authorise_api?token=API_KEY").respond success: "Use of API Authorised"
       spyOn($scope, "fetchProducts").and.returnValue "nothing"
       $scope.initialise()
-      $httpBackend.flush()
       expect($scope.fetchProducts.calls.count()).toBe 1
-      expect($scope.spree_api_key_ok).toEqual true
-
 
   describe "fetching products", ->
     $q = null

spec/services/product_tag_rules_filterer_spec.rb

@@ -79,7 +79,7 @@ describe ProductTagRulesFilterer do
           variant_hidden_for_another_customer.update_attribute(:tag_list, non_applicable_rule.preferred_variant_tags)
 
           overrides_to_hide = filterer.__send__(:overrides_to_hide)
-          expect(overrides_to_hide).to eq [variant_hidden_by_default.id, variant_hidden_by_rule.id]
+          expect(overrides_to_hide).to include variant_hidden_by_default.id, variant_hidden_by_rule.id
         end
       end