First step towards visualization destruction with AR models #11912

app/controllers/api/json/visualizations_controller.rb

@@ -175,49 +175,6 @@ class Api::Json::VisualizationsController < Api::ApplicationController
     end
   end
 
-  def destroy
-    @stats_aggregator.timing('visualizations.destroy') do
-
-      begin
-        vis,  = @stats_aggregator.timing('locate') do
-          locator.get(@table_id, CartoDB.extract_subdomain(request))
-        end
-
-        return head(404) unless vis
-        return head(403) unless vis.is_owner?(current_user)
-
-        current_viewer_id = current_viewer.id
-        properties = { user_id: current_viewer_id, visualization_id: vis.id }
-        if vis.derived?
-          Carto::Tracking::Events::DeletedMap.new(current_viewer_id, properties).report
-        else
-          Carto::Tracking::Events::DeletedDataset.new(current_viewer_id, properties).report
-        end
-
-        unless vis.table.nil?
-          vis.table.fully_dependent_visualizations.each do |dependent_vis|
-            properties = { user_id: current_viewer_id, visualization_id: dependent_vis.id }
-            if dependent_vis.derived?
-              Carto::Tracking::Events::DeletedMap.new(current_viewer_id, properties).report
-            else
-              Carto::Tracking::Events::DeletedDataset.new(current_viewer_id, properties).report
-            end
-          end
-        end
-
-        @stats_aggregator.timing('delete') do
-          vis.delete
-        end
-
-        return head 204
-      rescue KeyError
-        head(404)
-      rescue Sequel::DatabaseError => e
-        render_jsonp({ errors: [e.message] }, 400)
-      end
-    end
-  end
-
   def notify_watching
     vis = Visualization::Member.new(id: @table_id).fetch
     return(head 403) unless vis.has_permission?(current_user, Visualization::Member::PERMISSION_READONLY)

app/controllers/carto/api/visualizations_controller.rb

@@ -121,6 +121,42 @@ module Carto
         redirect_to Carto::StaticMapsURLHelper.new.url_for_static_map(request, @visualization, map_width, map_height)
       end
 
+      def destroy
+        begin
+          vis = get_priority_visualization(params[:id], user_id: current_user.id)
+
+          return head(404) unless vis
+          return head(403) unless vis.is_owner?(current_user)
+
+          current_viewer_id = current_viewer.id
+          properties = {user_id: current_viewer_id, visualization_id: vis.id}
+          if vis.derived?
+            Carto::Tracking::Events::DeletedMap.new(current_viewer_id, properties).report
+          else
+            Carto::Tracking::Events::DeletedDataset.new(current_viewer_id, properties).report
+          end
+
+          unless vis.table.nil?
+            vis.table.fully_dependent_visualizations.each do |dependent_vis|
+              properties = {user_id: current_viewer_id, visualization_id: dependent_vis.id}
+              if dependent_vis.derived?
+                Carto::Tracking::Events::DeletedMap.new(current_viewer_id, properties).report
+              else
+                Carto::Tracking::Events::DeletedDataset.new(current_viewer_id, properties).report
+              end
+            end
+          end
+
+          vis.delete
+
+          return head 204
+        rescue KeyError
+          head(404)
+        rescue Sequel::DatabaseError => e
+          render_jsonp({errors: [e.message]}, 400)
+        end
+      end
+
       private
 
       def generate_vizjson2

config/routes.rb

@@ -343,6 +343,8 @@ CartoDB::Application.routes.draw do
     match '(/user/:user_domain)(/u/:user_domain)/api/v1/viz/:id/like' => 'visualizations#is_liked', as: :api_v1_visualizations_is_liked, constraints: {method: 'OPTIONS'}
     get '(/user/:user_domain)(/u/:user_domain)/api/v1/viz/:id/related_templates'          => 'templates#related_templates_by_visualization', as: :api_v1_visualizations_related_templates, constraints: { id: /[^\/]+/ }
 
+    delete '(/user/:user_domain)(/u/:user_domain)/api/v1/viz/:id'                         => 'visualizations#destroy',         as: :api_v1_visualizations_destroy,         constraints: { id: /[^\/]+/ }
+
     # Tables
     get '(/user/:user_domain)(/u/:user_domain)/api/v1/tables/:id'                     => 'tables#show',   as: :api_v1_tables_show, constraints: { id: /[^\/]+/ }
     get '(/user/:user_domain)(/u/:user_domain)/api/v1/tables/:id/related_templates'   => 'templates#related_templates_by_table', as: :api_v1_tables_related_templates, constraints: { id: /[^\/]+/ }
@@ -501,7 +503,6 @@ CartoDB::Application.routes.draw do
     # Visualizations
     post    '(/user/:user_domain)(/u/:user_domain)/api/v1/viz'                                => 'visualizations#create',          as: :api_v1_visualizations_create
     put     '(/user/:user_domain)(/u/:user_domain)/api/v1/viz/:id'                            => 'visualizations#update',          as: :api_v1_visualizations_update,          constraints: { id: /[^\/]+/ }
-    delete  '(/user/:user_domain)(/u/:user_domain)/api/v1/viz/:id'                            => 'visualizations#destroy',         as: :api_v1_visualizations_destroy,         constraints: { id: /[^\/]+/ }
     # TODO: deprecate?
     put     '(/user/:user_domain)(/u/:user_domain)/api/v1/viz/:id/watching'                   => 'visualizations#notify_watching', as: :api_v1_visualizations_list_watching,   constraints: { id: /[^\/]+/ }
     put     '(/user/:user_domain)(/u/:user_domain)/api/v1/viz/:id/next_id'                    => 'visualizations#set_next_id',     as: :api_v1_visualizations_set_next_id,     constraints: { id: /[^\/]+/ }

spec/requests/carto/api/visualizations_controller_spec.rb

@@ -1560,6 +1560,13 @@ describe Carto::Api::VisualizationsController do
       end
     end
 
+    describe '#destroy' do
+      it 'returns 404 for nonexisting visualizations' do
+        id = random_uuid
+        delete api_v1_visualizations_destroy_url(id: random_uuid, api_key: @api_key), { id: random_uuid }.to_json, @headers
+        last_response.status.should == 404
+      end
+    end
   end
 
   describe 'index' do