From 9c3bb863da166cdc31c5e56b930762f586a46fb9 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 12 Jul 2018 11:43:10 +0800 Subject: [PATCH] Add endpoints for removing enterprise images --- app/controllers/api/base_controller.rb | 4 + .../api/enterprise_attachment_controller.rb | 37 ++++++++ app/controllers/api/logos_controller.rb | 16 ++++ .../api/promo_images_controller.rb | 16 ++++ app/models/spree/ability_decorator.rb | 2 +- config/locales/en.yml | 8 ++ config/routes.rb | 4 + spec/controllers/api/logos_controller_spec.rb | 90 +++++++++++++++++++ .../api/promo_images_controller_spec.rb | 90 +++++++++++++++++++ spec/models/spree/ability_spec.rb | 4 +- 10 files changed, 268 insertions(+), 3 deletions(-) create mode 100644 app/controllers/api/enterprise_attachment_controller.rb create mode 100644 app/controllers/api/logos_controller.rb create mode 100644 app/controllers/api/promo_images_controller.rb create mode 100644 spec/controllers/api/logos_controller_spec.rb create mode 100644 spec/controllers/api/promo_images_controller_spec.rb diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index f25c47417d..a7f96d01cf 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -9,5 +9,9 @@ module Api include ActionController::UrlFor include Rails.application.routes.url_helpers use_renderers :json + + def respond_with_conflict(json_hash) + render json: json_hash, status: :conflict + end end end diff --git a/app/controllers/api/enterprise_attachment_controller.rb b/app/controllers/api/enterprise_attachment_controller.rb new file mode 100644 index 0000000000..083d6946be --- /dev/null +++ b/app/controllers/api/enterprise_attachment_controller.rb @@ -0,0 +1,37 @@ +module Api + class EnterpriseAttachmentController < BaseController + class MissingImplementationError < StandardError; end + class UnknownEnterpriseAuthorizationActionError < StandardError; end + + before_filter :load_enterprise + + respond_to :json + + def destroy + return respond_with_conflict(error: destroy_attachment_does_not_exist_error_message) unless @enterprise.public_send("#{attachment_name}?") + + @enterprise.update_attributes!(attachment_name => nil) + render json: @enterprise, serializer: Admin::EnterpriseSerializer, spree_current_user: spree_current_user + end + + protected + + def attachment_name + raise MissingImplementationError, "Method attachment_name should be defined" + end + + def enterprise_authorize_action + raise MissingImplementationError, "Method enterprise_authorize_action should be defined" + end + + def load_enterprise + @enterprise = Enterprise.find_by_permalink(params[:enterprise_id].to_s) + raise UnknownEnterpriseAuthorizationActionError if enterprise_authorize_action.blank? + authorize!(enterprise_authorize_action, @enterprise) + end + + def destroy_attachment_does_not_exist_error_message + I18n.t("api.enterprise_#{attachment_name}.destroy_attachment_does_not_exist") + end + end +end diff --git a/app/controllers/api/logos_controller.rb b/app/controllers/api/logos_controller.rb new file mode 100644 index 0000000000..f3e7934724 --- /dev/null +++ b/app/controllers/api/logos_controller.rb @@ -0,0 +1,16 @@ +module Api + class LogosController < EnterpriseAttachmentController + private + + def attachment_name + :logo + end + + def enterprise_authorize_action + case action_name.to_sym + when :destroy + :remove_logo + end + end + end +end diff --git a/app/controllers/api/promo_images_controller.rb b/app/controllers/api/promo_images_controller.rb new file mode 100644 index 0000000000..0e79ebdb93 --- /dev/null +++ b/app/controllers/api/promo_images_controller.rb @@ -0,0 +1,16 @@ +module Api + class PromoImagesController < EnterpriseAttachmentController + private + + def attachment_name + :promo_image + end + + def enterprise_authorize_action + case action_name.to_sym + when :destroy + :remove_promo_image + end + end + end +end diff --git a/app/models/spree/ability_decorator.rb b/app/models/spree/ability_decorator.rb index 53198251b9..9d0941a220 100644 --- a/app/models/spree/ability_decorator.rb +++ b/app/models/spree/ability_decorator.rb @@ -99,7 +99,7 @@ class AbilityDecorator end can [:admin, :index, :create], Enterprise - can [:read, :edit, :update, :bulk_update, :resend_confirmation], Enterprise do |enterprise| + can [:read, :edit, :update, :remove_logo, :remove_promo_image, :bulk_update, :resend_confirmation], Enterprise do |enterprise| OpenFoodNetwork::Permissions.new(user).editable_enterprises.include? enterprise end can [:welcome, :register], Enterprise do |enterprise| diff --git a/config/locales/en.yml b/config/locales/en.yml index 540d306a95..61f74ad2a8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1077,6 +1077,14 @@ en: stripe_connect_settings: resource: Stripe Connect configuration +# API +# + api: + enterprise_logo: + destroy_attachment_does_not_exist: "Logo does not exist" + enterprise_promo_image: + destroy_attachment_does_not_exist: "Promo image does not exist" + # Frontend views # # These keys are referenced relatively like `t('.message')` in diff --git a/config/routes.rb b/config/routes.rb index 574140bd5b..4f6125c8ac 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -95,7 +95,11 @@ Openfoodnetwork::Application.routes.draw do post :update_image, on: :member get :managed, on: :collection get :accessible, on: :collection + + resource :logo, only: [:destroy] + resource :promo_image, only: [:destroy] end + resources :order_cycles do get :managed, on: :collection get :accessible, on: :collection diff --git a/spec/controllers/api/logos_controller_spec.rb b/spec/controllers/api/logos_controller_spec.rb new file mode 100644 index 0000000000..213ac8bc6e --- /dev/null +++ b/spec/controllers/api/logos_controller_spec.rb @@ -0,0 +1,90 @@ +require "spec_helper" + +module Api + describe LogosController, type: :controller do + include AuthenticationWorkflow + + let(:admin_user) { create(:admin_user) } + let(:enterprise_owner) { create(:user) } + let(:enterprise) { create(:enterprise, owner: enterprise_owner ) } + let(:enterprise_manager) { create(:user, enterprise_limit: 10, enterprises: [enterprise]) } + let(:other_enterprise_owner) { create(:user) } + let(:other_enterprise) { create(:enterprise, owner: other_enterprise_owner ) } + let(:other_enterprise_manager) { create(:user, enterprise_limit: 10, enterprises: [other_enterprise]) } + + describe "removing logo" do + image_path = File.open(Rails.root.join("app", "assets", "images", "logo-black.png")) + let(:image) { Rack::Test::UploadedFile.new(image_path, "image/png") } + + let(:enterprise) { create(:enterprise, owner: enterprise_owner, logo: image) } + + before do + allow(controller).to receive(:spree_current_user) { current_user } + end + + context "as manager" do + let(:current_user) { enterprise_manager } + + it "removes logo" do + spree_delete :destroy, enterprise_id: enterprise + + expect(response).to be_success + expect(json_response["id"]).to eq enterprise.id + enterprise.reload + expect(enterprise.logo?).to be false + end + + context "when logo does not exist" do + let(:enterprise) { create(:enterprise, owner: enterprise_owner, logo: nil) } + + it "responds with error" do + spree_delete :destroy, enterprise_id: enterprise + + expect(response.status).to eq(409) + expect(json_response["error"]).to eq I18n.t("api.enterprise_logo.destroy_attachment_does_not_exist") + end + end + end + + context "as owner" do + let(:current_user) { enterprise_owner } + + it "allows removal of logo" do + spree_delete :destroy, enterprise_id: enterprise + expect(response).to be_success + end + end + + context "as super admin" do + let(:current_user) { admin_user } + + it "allows removal of logo" do + spree_delete :destroy, enterprise_id: enterprise + expect(response).to be_success + end + end + + context "as manager of other enterprise" do + let(:current_user) { other_enterprise_manager } + + it "does not allow removal of logo" do + spree_delete :destroy, enterprise_id: enterprise + expect(response.status).to eq(401) + enterprise.reload + expect(enterprise.logo?).to be true + end + end + + context "as owner of other enterprise" do + let(:current_user) { other_enterprise_owner } + + it "does not allow removal of logo" do + spree_delete :destroy, enterprise_id: enterprise + expect(response.status).to eq(401) + enterprise.reload + expect(enterprise.logo?).to be true + end + end + end + end +end diff --git a/spec/controllers/api/promo_images_controller_spec.rb b/spec/controllers/api/promo_images_controller_spec.rb new file mode 100644 index 0000000000..cce6d08f5f --- /dev/null +++ b/spec/controllers/api/promo_images_controller_spec.rb @@ -0,0 +1,90 @@ +require "spec_helper" + +module Api + describe PromoImagesController, type: :controller do + include AuthenticationWorkflow + + let(:admin_user) { create(:admin_user) } + let(:enterprise_owner) { create(:user) } + let(:enterprise) { create(:enterprise, owner: enterprise_owner ) } + let(:enterprise_manager) { create(:user, enterprise_limit: 10, enterprises: [enterprise]) } + let(:other_enterprise_owner) { create(:user) } + let(:other_enterprise) { create(:enterprise, owner: other_enterprise_owner ) } + let(:other_enterprise_manager) { create(:user, enterprise_limit: 10, enterprises: [other_enterprise]) } + + describe "removing promo image" do + image_path = File.open(Rails.root.join("app", "assets", "images", "logo-black.png")) + let(:image) { Rack::Test::UploadedFile.new(image_path, "image/png") } + + let(:enterprise) { create(:enterprise, owner: enterprise_owner, promo_image: image) } + + before do + allow(controller).to receive(:spree_current_user) { current_user } + end + + context "as manager" do + let(:current_user) { enterprise_manager } + + it "removes promo image" do + spree_delete :destroy, enterprise_id: enterprise + + expect(response).to be_success + expect(json_response["id"]).to eq enterprise.id + enterprise.reload + expect(enterprise.promo_image?).to be false + end + + context "when promo image does not exist" do + let(:enterprise) { create(:enterprise, owner: enterprise_owner, promo_image: nil) } + + it "responds with error" do + spree_delete :destroy, enterprise_id: enterprise + + expect(response.status).to eq(409) + expect(json_response["error"]).to eq I18n.t("api.enterprise_promo_image.destroy_attachment_does_not_exist") + end + end + end + + context "as owner" do + let(:current_user) { enterprise_owner } + + it "allows removal of promo image" do + spree_delete :destroy, enterprise_id: enterprise + expect(response).to be_success + end + end + + context "as super admin" do + let(:current_user) { admin_user } + + it "allows removal of promo image" do + spree_delete :destroy, enterprise_id: enterprise + expect(response).to be_success + end + end + + context "as manager of other enterprise" do + let(:current_user) { other_enterprise_manager } + + it "does not allow removal of promo image" do + spree_delete :destroy, enterprise_id: enterprise + expect(response.status).to eq(401) + enterprise.reload + expect(enterprise.promo_image?).to be true + end + end + + context "as owner of other enterprise" do + let(:current_user) { other_enterprise_owner } + + it "does not allow removal of promo image" do + spree_delete :destroy, enterprise_id: enterprise + expect(response.status).to eq(401) + enterprise.reload + expect(enterprise.promo_image?).to be true + end + end + end + end +end diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index 7882665618..dcd290e195 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -297,11 +297,11 @@ module Spree let!(:er_pd) { create(:enterprise_relationship, parent: d_related, child: d1, permissions_list: [:edit_profile]) } it "should be able to edit enterprises it manages" do - should have_ability([:read, :edit, :update, :bulk_update, :resend_confirmation], for: d1) + should have_ability([:read, :edit, :update, :remove_logo, :remove_promo_image, :bulk_update, :resend_confirmation], for: d1) end it "should be able to edit enterprises it has permission to" do - should have_ability([:read, :edit, :update, :bulk_update, :resend_confirmation], for: d_related) + should have_ability([:read, :edit, :update, :remove_logo, :remove_promo_image, :bulk_update, :resend_confirmation], for: d_related) end it "should be able to manage shipping methods, payment methods and enterprise fees for enterprises it manages" do