Merge pull request #15385 from CartoDB/feature/kuviz-name-unique

kuviz has unique name

NEWS.md

@@ -24,6 +24,7 @@ sudo make install
 - Add default delivery days for data observatory metadata ([#15362](https://github.com/CartoDB/cartodb/pull/15362))
 - Add required tips parameter to fix street geocoding in advanced mode ([CartoDB/support#2265](https://github.com/CartoDB/support/issues/2265))
 - Use plpython3u for PG12+ ([#15228](https://github.com/CartoDB/cartodb/pull/15228))
+- Unique name for Kuvizs [#15385](https://github.com/CartoDB/cartodb/pull/15385)
 
 
 4.32.0 (2019-12-27)

app/controllers/carto/api/public/custom_visualizations_controller.rb

@@ -13,6 +13,10 @@ class Carto::Api::Public::CustomVisualizationsController < Carto::Api::Public::A
     Carto::Visualization::PRIVACY_PROTECTED
   ].freeze
 
+  IF_EXISTS_FAIL = 'fail'.freeze
+  IF_EXISTS_REPLACE = 'replace'.freeze
+  VALID_IF_EXISTS = [IF_EXISTS_FAIL, IF_EXISTS_REPLACE].freeze
+
   ssl_required
 
   before_action :validate_mandatory_creation_params, only: [:create]
@@ -21,8 +25,12 @@ class Carto::Api::Public::CustomVisualizationsController < Carto::Api::Public::A
   before_action :get_user, only: [:create, :update, :delete]
   before_action :check_edition_permission, only: [:update, :delete]
   before_action :check_master_api_key
+  before_action :validate_if_exists, only: [:create, :update]
+  before_action :get_last_one, only: [:create]
+  before_action :remove_duplicates, only: [:update]
 
   rescue_from Carto::UnauthorizedError, with: :rescue_from_carto_error
+  rescue_from Carto::ParamInvalidError, with: :rescue_from_carto_error
 
   def index
     opts = { valid_order_combinations: VALID_ORDER_PARAMS }
@@ -47,13 +55,18 @@ def index
   end
 
   def create
+    return update if @kuviz
+
     kuviz = create_visualization_metadata(@logged_user)
     asset = Carto::Asset.for_visualization(visualization: kuviz,
                                            resource: StringIO.new(Base64.decode64(params[:data])))
     asset.save
     Carto::Tracking::Events::CreatedMap.new(@logged_user.id, event_properties(kuviz).merge(origin: 'custom')).report
 
     render_jsonp(Carto::Api::Public::KuvizPresenter.new(self, @logged_user, kuviz).to_hash, 200)
+  rescue ActiveRecord::RecordInvalid => e
+    CartoDB::Logger.error(message: 'Error creating kuviz', params: params, exception: e)
+    render_jsonp({ error: e.message }, 400)
   rescue StandardError => e
     CartoDB::Logger.error(message: 'Error creating kuviz', params: params, exception: e)
     render_jsonp({ error: 'cant create the kuviz' }, 500)
@@ -70,6 +83,8 @@ def update
     Carto::Tracking::Events::ModifiedMap.new(@logged_user.id, event_properties(@kuviz)).report
 
     render_jsonp(Carto::Api::Public::KuvizPresenter.new(self, @logged_user, @kuviz).to_hash, 200)
+  rescue ActiveRecord::RecordInvalid => e
+    render_jsonp({ error: e.message }, 400)
   end
 
   def delete
@@ -90,7 +105,7 @@ def create_visualization_metadata(user)
     kuviz.password = params[:password]
     kuviz.type = Carto::Visualization::TYPE_KUVIZ
     kuviz.user = user
-    kuviz.save
+    kuviz.save!
     kuviz
   end
 
@@ -160,4 +175,26 @@ def get_kuviz
     end
   end
 
+  def validate_if_exists
+    @if_exists = params[:if_exists]
+    if @if_exists.nil?
+      @if_exists = @kuviz.present? ? IF_EXISTS_REPLACE : IF_EXISTS_FAIL
+    end
+
+    raise Carto::ParamInvalidError.new(:if_exists, VALID_IF_EXISTS.join(', ')) unless VALID_IF_EXISTS.include?(@if_exists)
+  end
+
+  def get_last_one
+    if @if_exists == IF_EXISTS_REPLACE
+      @kuviz = Carto::Visualization.where(user: @logged_user, name: params[:name]).order(updated_at: :desc).first
+    end
+  end
+
+  def remove_duplicates
+    if @if_exists == IF_EXISTS_REPLACE
+      existing_kuvizs = Carto::Visualization.where(user: @logged_user, name: params[:name]) - [@kuviz]
+      existing_kuvizs.each(&:destroy!)
+    end
+  end
+
 end

app/models/carto/visualization.rb

@@ -95,6 +95,7 @@ class Carto::Visualization < ActiveRecord::Base
   validates :name, :privacy, :type, :user_id, :version, presence: true
   validates :privacy, inclusion: { in: PRIVACIES }
   validates :type, inclusion: { in: VALID_TYPES }
+  validates :name, uniqueness: { scope: :user_id }, if: :kuviz?
   validate :validate_password_presence
   validate :validate_privacy_changes
   validate :validate_user_not_viewer, on: :create

spec/requests/carto/api/custom_visualizations_controller_spec.rb

@@ -22,12 +22,12 @@
 
   describe '#index' do
     before(:each) do
-      @kuviz = FactoryGirl.create(:kuviz_visualization, user: @user)
+      @kuviz = FactoryGirl.create(:kuviz_visualization, user: @user, name: 'kuviz')
       @kuviz.save
       @asset = Carto::Asset.for_visualization(visualization: @kuviz,
                                               resource: StringIO.new('<html><body>test</body></html>'))
       @asset.save
-      @kuviz_password = FactoryGirl.create(:kuviz_protected_visualization, user: @user)
+      @kuviz_password = FactoryGirl.create(:kuviz_protected_visualization, user: @user, name: 'kuviz password')
       @kuviz_password.save
       @asset_password = Carto::Asset.for_visualization(visualization: @kuviz_password,
                                                        resource: StringIO.new('<html><body>test</body></html>'))
@@ -160,28 +160,34 @@
   describe '#create' do
     before(:all) do
       @valid_html_base64 = Base64.strict_encode64('<html><head><title>test</title></head><body>test</body></html>')
+      @kuviz_name = 'kuviz_name'
+    end
+
+    after(:each) do
+      kuvizs = Carto::Visualization.where(user: @user)
+      kuvizs.each(&:destroy!)
     end
 
     it 'returns 403 wih default_public api_key' do
       token = 'default_public'
 
-      post_json api_v4_kuviz_create_viz_url(api_key: token), data: @valid_html_base64, name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: token), data: @valid_html_base64, name: @kuviz_name do |response|
         expect(response.status).to eq(403)
       end
     end
 
     it 'returns 403 with oauth api_key' do
       api_key = FactoryGirl.create(:oauth_api_key, user_id: @user.id)
 
-      post_json api_v4_kuviz_create_viz_url(api_key: api_key.token), data: @valid_html_base64, name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: api_key.token), data: @valid_html_base64, name: @kuviz_name do |response|
         expect(response.status).to eq(403)
       end
     end
 
     it 'returns 403 wih regular api_key' do
       api_key = FactoryGirl.create(:api_key_apis, user_id: @user.id)
 
-      post_json api_v4_kuviz_create_viz_url(api_key: api_key.token), data: @valid_html_base64, name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: api_key.token), data: @valid_html_base64, name: @kuviz_name do |response|
         expect(response.status).to eq(403)
       end
     end
@@ -199,57 +205,125 @@
     end
 
     it 'rejects if data parameter is not send in the request' do
-      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: nil, name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: nil, name: @kuviz_name do |response|
         expect(response.status).to eq(400)
         expect(response.body[:error]).to eq('missing data parameter')
       end
-      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), name: @kuviz_name do |response|
         expect(response.status).to eq(400)
         expect(response.body[:error]).to eq('missing data parameter')
       end
     end
 
     it 'rejects if data parameter is not encoded in base64' do
-      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: 'non-base64 test', name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: 'non-base64 test', name: @kuviz_name do |response|
         expect(response.status).to eq(400)
         expect(response.body[:error]).to eq('data parameter must be encoded in base64')
       end
     end
 
     it 'rejects non html content' do
       string_base64 = Base64.strict_encode64('test string non-html')
-      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: string_base64, name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: string_base64, name: @kuviz_name do |response|
         expect(response.status).to eq(400)
         expect(response.body[:error]).to eq('data parameter must be HTML')
       end
       pixel_base64 = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=='
-      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: pixel_base64, name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: pixel_base64, name: @kuviz_name do |response|
         expect(response.status).to eq(400)
         expect(response.body[:error]).to eq('data parameter must be HTML')
       end
     end
 
     it 'stores html content' do
-      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: 'test' do |response|
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name do |response|
         expect(response.status).to eq(200)
         expect(response.body[:visualizations]).present?.should be true
         expect(response.body[:url]).present?.should be true
       end
     end
+
+    it 'rejects if if_exists parameter is not a valid one' do
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name, if_exists: 'wrong-option' do |response|
+        expect(response.status).to eq(400)
+        expect(response.body[:errors]).to eq("Wrong 'if_exists' parameter value. Valid values are one of fail, replace")
+      end
+    end
+
+    it 'fails if if_exists is fail and exists a kuviz with the same name' do
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+      end
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name do |response|
+        expect(response.status).to eq(400)
+        expect(response.body[:error]).to eq("Validation failed: Name has already been taken")
+      end
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name, if_exists: 'fail' do |response|
+        expect(response.status).to eq(400)
+        expect(response.body[:error]).to eq("Validation failed: Name has already been taken")
+      end
+    end
+
+    it 'works if if_exists is fail and does not exists a kuviz with the same name' do
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+      end
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: 'another name' do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+      end
+    end
+
+    it 'works if if_exists is replace and exists a kuviz with the same name' do
+      kuviz1 = nil
+
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+
+        kuvizs = Carto::Visualization.where(user: @user, name: @kuviz_name)
+        expect(kuvizs.length).to be 1
+        kuviz1 = kuvizs.first
+      end
+      post_json api_v4_kuviz_create_viz_url(api_key: @user.api_key), data: @valid_html_base64, name: @kuviz_name, if_exists: 'replace' do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+
+        kuvizs = Carto::Visualization.where(user: @user, name: @kuviz_name)
+        expect(kuvizs.length).to be 1
+        kuviz2 = kuvizs.first
+        expect(kuviz1.id).to eq(kuviz2.id)
+      end
+    end
   end
 
   describe '#update' do
     before(:each) do
       @kuviz = FactoryGirl.create(:kuviz_visualization, user: @user)
-      @kuviz.save
+      @kuviz.save!
       @asset = Carto::Asset.for_visualization(visualization: @kuviz,
                                               resource: StringIO.new('<html><body>test</body></html>'))
       @asset.save
-      @kuviz2 = FactoryGirl.create(:kuviz_visualization)
-      @kuviz2.save
-      @asset2 = Carto::Asset.for_visualization(visualization: @kuviz2,
-                                               resource: StringIO.new('<html><body>test</body></html>'))
+
+      @kuviz2 = FactoryGirl.create(:kuviz_visualization, user: @user, name: 'kuviz2')
+      @kuviz2.save!
+      @asset2 = Carto::Asset.for_visualization(visualization: @kuviz,
+                                              resource: StringIO.new('<html><body>test</body></html>'))
       @asset2.save
+
+      @kuviz_other_user = FactoryGirl.create(:kuviz_visualization)
+      @kuviz_other_user.save!
+      @asset_other_user = Carto::Asset.for_visualization(visualization: @kuviz_other_user,
+                                               resource: StringIO.new('<html><body>test</body></html>'))
+      @asset_other_user.save
+    end
+
+    after(:each) do
+      @kuviz.destroy!
+      @kuviz2.destroy!
+      @kuviz_other_user.destroy!
     end
 
     it 'returns 403 wih default_public api_key' do
@@ -327,7 +401,7 @@
     end
 
     it 'shouldn\'t update an existing kuviz if the user doesn\'t have permission' do
-      put_json api_v4_kuviz_update_viz_url(api_key: @user.api_key, id: @kuviz2.id), name: 'test' do |response|
+      put_json api_v4_kuviz_update_viz_url(api_key: @user.api_key, id: @kuviz_other_user.id), name: 'test' do |response|
         expect(response.status).to eq(403)
       end
     end
@@ -337,6 +411,64 @@
         expect(response.status).to eq(404)
       end
     end
+
+    it 'rejects if if_exists parameter is not a valid one' do
+      put_json api_v4_kuviz_update_viz_url(api_key: @user.api_key, id: @kuviz.id), name: 'test', if_exists: 'wrong-option' do |response|
+        expect(response.status).to eq(400)
+        expect(response.body[:errors]).to eq("Wrong 'if_exists' parameter value. Valid values are one of fail, replace")
+      end
+    end
+
+    it 'works if name already exists and if_exists is replace by default' do
+      put_json api_v4_kuviz_update_viz_url(api_key: @user.api_key, id: @kuviz.id), name: @kuviz2.name do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+
+        kuvizs = Carto::Visualization.where(user: @user)
+        expect(kuvizs.length).to be [@kuviz].length
+
+        kuviz_updated = Carto::Visualization.find(@kuviz.id)
+        expect(kuviz_updated.id).to eq @kuviz.id
+        expect(kuviz_updated.name).to eq @kuviz2.name
+      end
+    end
+
+    it 'works if name already exists and if_exists is replace' do
+        put_json api_v4_kuviz_update_viz_url(api_key: @user.api_key, id: @kuviz.id), name: @kuviz2.name, if_exists: 'replace' do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+
+        kuvizs = Carto::Visualization.where(user: @user)
+        expect(kuvizs.length).to be [@kuviz].length
+
+        kuviz_updated = Carto::Visualization.find(@kuviz.id)
+        expect(kuviz_updated.id).to eq @kuviz.id
+        expect(kuviz_updated.name).to eq @kuviz2.name
+      end
+    end
+
+    it 'rejects if name already exists and if_exists is fail' do
+      put_json api_v4_kuviz_update_viz_url(api_key: @user.api_key, id: @kuviz.id), name: @kuviz2.name, if_exists: 'fail' do |response|
+        expect(response.status).to eq(400)
+        expect(response.body[:error]).to eq("Validation failed: Name has already been taken")
+      end
+    end
+
+    it 'works if if_exists is fail and name does not exists' do
+      new_name = 'other_name'
+      put_json api_v4_kuviz_update_viz_url(api_key: @user.api_key, id: @kuviz.id), name: new_name, if_exists: 'fail' do |response|
+        expect(response.status).to eq(200)
+        expect(response.body[:url].present?).to be true
+
+        kuvizs = Carto::Visualization.where(user: @user)
+        expect(kuvizs.length).to be [@kuviz, @kuviz2].length
+
+        kuviz_updated = Carto::Visualization.find(@kuviz.id)
+        expect(kuviz_updated.id).to eq @kuviz.id
+        expect(kuviz_updated.name).to eq new_name
+        expect(kuviz_updated.name).not_to eq @kuviz.name
+      end
+    end
   end
 
   describe '#delete' do
@@ -346,11 +478,16 @@
       @asset = Carto::Asset.for_visualization(visualization: @kuviz,
                                               resource: StringIO.new('<html><body>test</body></html>'))
       @asset.save
-      @kuviz2 = FactoryGirl.create(:kuviz_visualization)
-      @kuviz2.save
-      @asset2 = Carto::Asset.for_visualization(visualization: @kuviz2,
+      @kuviz_other_user = FactoryGirl.create(:kuviz_visualization)
+      @kuviz_other_user.save
+      @asset_other_user = Carto::Asset.for_visualization(visualization: @kuviz_other_user,
                                                resource: StringIO.new('<html><body>test</body></html>'))
-      @asset2.save
+      @asset_other_user.save
+    end
+
+    after(:each) do
+      @kuviz.destroy!
+      @kuviz_other_user.destroy!
     end
 
     it 'returns 403 wih default_public api_key' do
@@ -386,7 +523,7 @@
     end
 
     it 'shouldn\'t delete a kuviz for which the user doesn\'t have permissions' do
-      delete_json api_v4_kuviz_delete_viz_url(api_key: @user.api_key, id: @kuviz2.id) do |response|
+      delete_json api_v4_kuviz_delete_viz_url(api_key: @user.api_key, id: @kuviz_other_user.id) do |response|
         expect(response.status).to eq(403)
       end
     end