From 2313237a954149b6bc6735b82264c9e8a796ff27 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Fri, 18 Nov 2022 07:09:50 +1100
Subject: [PATCH] FIX: better handling of edge cases (#187)

- Require query name is present
- Ensure all routes are treated by default as .json, so errors flow correctly
- Remove superflous save/cancel controls from group settings
- Remove group control when item is destroyed
- Disable editing of query when it is deleted

Co-authored-by: Osama Sayegh <asooomaasoooma90@gmail.com>
---
 app/models/data_explorer/query.rb             |  1 +
 .../templates/admin/plugins-explorer.hbs      | 45 +++++++------------
 plugin.rb                                     | 16 ++++---
 spec/requests/query_controller_spec.rb        | 15 ++++++-
 4 files changed, 38 insertions(+), 39 deletions(-)

diff --git a/app/models/data_explorer/query.rb b/app/models/data_explorer/query.rb
index 3e0b07b..f072463 100644
--- a/app/models/data_explorer/query.rb
+++ b/app/models/data_explorer/query.rb
@@ -6,6 +6,7 @@ module DataExplorer
     has_many :query_groups
     has_many :groups, through: :query_groups
     belongs_to :user
+    validates :name, presence: true
 
     scope :for_group, ->(group) do
       where(hidden: false)
diff --git a/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs b/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs
index 9edafe0..afc8090 100644
--- a/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs
+++ b/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs
@@ -88,36 +88,21 @@
               </div>
             {{/if}}
 
-            <div class="pull-left">
-              <div class="groups">
-                <span class="label">{{i18n "explorer.allow_groups"}}</span>
-                <span>
-                  {{multi-select
-                    value=selectedItem.group_ids
-                    content=groupOptions
-                    allowAny=false
-                    onSelect=(action (mut selectedItem.group_ids))
-                  }}
-                </span>
-
-                {{#if runDisabled}}
-                  {{#unless editing}}
-                    <span class="setting-controls">
-                      {{d-button
-                        class="ok"
-                        action=(action "save")
-                        icon="check"
-                      }}
-                      {{d-button
-                        class="cancel"
-                        action=(action "discard")
-                        icon="times"
-                      }}
-                    </span>
-                  {{/unless}}
-                {{/if}}
+            {{#if (not selectedItem.destroyed)}}
+              <div class="pull-left">
+                <div class="groups">
+                  <span class="label">{{i18n "explorer.allow_groups"}}</span>
+                  <span>
+                    {{multi-select
+                      value=selectedItem.group_ids
+                      content=groupOptions
+                      allowAny=false
+                      onSelect=(action (mut selectedItem.group_ids))
+                    }}
+                  </span>
+                </div>
               </div>
-            </div>
+            {{/if}}
 
             <div class="clear"></div>
 
@@ -126,7 +111,7 @@
               <div class="query-editor {{if hideSchema "no-schema"}}">
                 <div class="panels-flex">
                   <div class="editor-panel">
-                    {{ace-editor content=selectedItem.sql mode="sql"}}
+                    {{ace-editor content=selectedItem.sql mode="sql" disabled=selectedItem.destroyed}}
                   </div>
 
                   <div class="right-panel">
diff --git a/plugin.rb b/plugin.rb
index 1118328..61ea23d 100644
--- a/plugin.rb
+++ b/plugin.rb
@@ -886,14 +886,16 @@ SQL
 
   DataExplorer::Engine.routes.draw do
     root to: "query#index"
-    get 'schema' => "query#schema"
     get 'queries' => "query#index"
-    get 'groups' => "query#groups"
-    post 'queries' => "query#create"
-    get 'queries/:id' => "query#show"
-    put 'queries/:id' => "query#update"
-    delete 'queries/:id' => "query#destroy"
-    post 'queries/:id/run' => "query#run", constraints: { format: /(json|csv)/ }
+    scope "/", defaults: { format: :json } do
+      get 'schema' => "query#schema"
+      get 'groups' => "query#groups"
+      post 'queries' => "query#create"
+      get 'queries/:id' => "query#show"
+      put 'queries/:id' => "query#update"
+      delete 'queries/:id' => "query#destroy"
+      post 'queries/:id/run' => "query#run", constraints: { format: /(json|csv)/ }
+    end
   end
 
   Discourse::Application.routes.append do
diff --git a/spec/requests/query_controller_spec.rb b/spec/requests/query_controller_spec.rb
index f322ce9..e89ba83 100644
--- a/spec/requests/query_controller_spec.rb
+++ b/spec/requests/query_controller_spec.rb
@@ -107,6 +107,19 @@ describe DataExplorer::QueryController do
 
         expect(response.status).to eq(200)
       end
+
+      it "returns a proper json error for invalid updates" do
+
+        query = DataExplorer::Query.find(-4)
+        put "/admin/plugins/explorer/queries/#{query.id}", params: {
+          "query" => {
+            "name" => "",
+          },
+          "id" => query.id }
+
+        expect(response.status).to eq(422)
+        expect(response.parsed_body["errors"]).to eq(["Name can't be blank"])
+      end
     end
 
     describe "#run" do
@@ -304,8 +317,6 @@ describe DataExplorer::QueryController do
             DataExplorer.send(:remove_const, "QUERY_RESULT_MAX_LIMIT")
             DataExplorer.const_set("QUERY_RESULT_MAX_LIMIT", 2)
 
-            ids = Post.order(:id).pluck(:id)
-
             query = make_query <<~SQL
             SELECT id FROM posts
             SQL