FEATURE: Add ability to soft delete (hide) queries and revert deletion with rake tasks (#54)

* FEATURE: Add hide button (toggleable) for all queries (frontend only)

* Switches between hide/unhide on click
* Works almost like the delete button, but toggles between the query's
hidden attribute instead
* So far this is only a frontend feature, the backend implementation
still needs work

* Revert "FEATURE: Add hide button (toggleable) for all queries (frontend only)"

This reverts commit a8771d2ad57083a91b7130df807fa54c26205d11.

REVERT: Remove button that hides queries (frontend)

* Prepare for migration of old frontend logic to backend
* We are going to reuse the existing delete button, but change its
backend logic to enable soft deletion. From the user's perspective
nothing will change, but any deletion mistakes can be reverted.

* DEV: Hide user queries upon deletion, but keep them in store

* Creating a new query will set its hidden attribute to false by
default
* Deleting a user-made query will not delete it from the store, but
set its hidden attribute to true
* User queries will not be indexed if they are hidden
* Undeleting a query will unhide it, and will be indexed
* Updating a hidden query will unhide it, and will be indexed

* SPEC: Add spec for hidden/deleted queries

* Hidden queries should not be shown

* FEATURE: Add ability to delete/hide system queries

* System queries are now able to be deleted from view, but will remain
in the backend for retrieval, if necessary

* FEATURE/DEV: Add rake commands for query soft deletion

* query:list_hidden - Shows a list of hidden queries
* query:hide_all[only_default] - Hides all queries, w/ boolean arg to
hide only default ones
* query:unhide[id] - Unhides a query by id
* query:unhide_all[exclude_default] - Unhides all hidden queries,
w/ boolean arg to exclude default ones

* Remove rails loggers

* UX/DEV: Update query rake tasks to be more user friendly

* Split query:hide_all[only_default] into two tasks:
    * query:hide_all - Hides all queries
    * query:hide_all:only_default - Hide only default queries
* Split query:unhide_all[exclude_default] into two tasks:
    * query:unhide_all - Unhides all hidden queries
    * query:unhide_all:exclude_default - Unhides all non-default
    queries
* Rename file to match task name

* UX: query:unhide can accept multiple arguments

* Example: rake query:unhide[-5,-6,-7,3,5,6,-13,-14,-10]

* UX: Update query rake tasks to output cleaner messages

* Remove unneeded comment

* DEV: Keep only necessary rake tasks, use more specific naming

* UX/DEV: Add rake task for hard deletion, better console logs

* User is able to hard delete a query only if it is hidden, otherwise
output a message stating so
* Add commented examples above each task
* Add rainbow support for more readable console logs
* Successful messages will display green, failures display red,
additional info displays yellow
* Separate multiple queries with spaces instead of lines

* DEV: Remove rainbow colorizing in console logs

* Rainbow is a dependency of rubocop and it may go away in the future
* Rainbow is only used for dev and test environments

* DEV: Add Rails engine to enable rake tasks to be loaded at runtime

* DEV: Favor require - load files only if they are not already loaded

* SPEC: Add tests for data_explorer[id] rake command

* Test if a single query is hidden correctly
    * Expect length of query list to not be modified
    * Expect array of hidden queries to have exactly 1 element
    * Expect that one element to have the same ID as the one invoked to
    be hidden
* Test if multiple queries are hidden correctly
    * Expect length of query list to not be modified
    * Expect array of hidden queries to have the number of elements
    equal to the number invoked to be hidden
    * Expect the elements to have the same ID as the ones invoked to be
    hidden
* Test if a query exists in PluginStore
    * Expect query list to be empty

* DEV: Clear pre-existing tasks before redefining

* This prevents double invocation when user invokes the task once

* SPEC: Add tests for unhide_query rake task

* Test if a single query unhides correctly
    * Expect length of query list to not be modified
    * Expect array of hidden queries to have exactly 1 element after
    unhiding 1 of 2 queries
    * Expect remaining element to be hidden
* Test if multiple queries unhide correctly
    * Expect length of query list to not be modified
    * Expect array of hidden queries to have exactly 1 element after
    unhiding 3 of 4 queries
    * Expect remaining element to be hidden
* Test if a query exists in PluginStore
    * Expect query list to not be modified

* SPEC: Add tests for hard_delete rake task

* Test if a single query hard deletes correctly
    * Expect length of query list to be shorter by 1
    * Expect array of hidden queries to have exactly 1 element after
    hard deleting 1 of 2 queries
    * Expect 1 remaining hidden element
* Test if multiple queries hard delete correctly
    * Expect length of query list to be shorter by 3 after hard deleting
    3 of 4 queries
    * Expect array of hidden queries to have exactly 1 element after
    hard deleting 3 of 4 queries
    * Expect 1 remaining hidden element
* Test if a query exists in PluginStore
    * Expect hidden query list to not be modified
* Test if a query is not hidden
    * Expect query list to not be modified

* UX: Favor newline char in place of puts for logs

* Condensed console logs to output newline char instead of another puts
statement (reduces number of lines used significantly)

.gitignore

@@ -6,3 +6,4 @@ auto_generated
 *.swp
 *.swo
 node_modules/
+.idea/

assets/javascripts/discourse/controllers/admin-plugins-explorer.js.es6

@@ -52,10 +52,6 @@ export default Ember.Controller.extend({
       ? this.set("showRecentQueries", false)
       : this.set("showRecentQueries", true);
 
-    if (id < 0) {
-      this.set("editDisabled", true);
-    }
-
     return item || NoQuery;
   },
 

lib/discourse_data_explorer/engine.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+module DiscourseDataExplorer
+  class Engine < ::Rails::Engine
+    isolate_namespace DiscourseDataExplorer
+  end
+end

lib/tasks/data_explorer.rake

@@ -0,0 +1,95 @@
+# frozen_string_literal: true
+
+# rake data_explorer:list_hidden_queries
+desc "Shows a list of hidden queries"
+task('data_explorer:list_hidden_queries').clear
+task 'data_explorer:list_hidden_queries' => :environment do |t|
+  hidden_queries = []
+  puts "\nHidden Queries\n\n"
+
+  DataExplorer::Query.all.each do |query|
+    hidden_queries.push(query) if query.hidden
+  end
+
+  hidden_queries.each do |query|
+    puts "Name: #{query.name}"
+    puts "Description: #{query.description}"
+    puts "ID: #{query.id}\n\n"
+  end
+end
+
+# rake data_explorer[-1]
+# rake data_explorer[1,-2,3,-4,5]
+desc "Hides one or multiple queries by ID"
+task('data_explorer').clear
+task 'data_explorer' => :environment do |t, args|
+  args.extras.each do |arg|
+    id = arg.to_i
+
+    if DataExplorer.pstore_get("q:#{id}").nil?
+      puts "\nError finding query with id #{id}"
+    else
+      q = DataExplorer::Query.find(id)
+      if q
+        puts "\nFound query with id #{id}"
+      end
+
+      q.hidden = true
+      q.save
+      puts "Query no.#{id} is now hidden" if q.hidden
+    end
+  end
+  puts ""
+end
+
+# rake data_explorer:unhide_query[-1]
+# rake data_explorer:unhide_query[1,-2,3,-4,5]
+desc "Unhides one or multiple queries by ID"
+task('data_explorer:unhide_query').clear
+task 'data_explorer:unhide_query' => :environment do |t, args|
+  args.extras.each do |arg|
+    id = arg.to_i
+
+    if DataExplorer.pstore_get("q:#{id}").nil?
+      puts "\nError finding query with id #{id}"
+    else
+      q = DataExplorer::Query.find(id)
+      if q
+        puts "\nFound query with id #{id}"
+      end
+
+      q.hidden = false
+      q.save
+      puts "Query no.#{id} is now visible" unless q.hidden
+    end
+  end
+  puts ""
+end
+
+# rake data_explorer:hard_delete[-1]
+# rake data_explorer:hard_delete[1,-2,3,-4,5]
+desc "Hard deletes one or multiple queries by ID"
+task('data_explorer:hard_delete').clear
+task 'data_explorer:hard_delete' => :environment do |t, args|
+  args.extras.each do |arg|
+    id = arg.to_i
+
+    if DataExplorer.pstore_get("q:#{id}").nil?
+      puts "\nError finding query with id #{id}"
+    else
+      q = DataExplorer::Query.find(id)
+      if q
+        puts "\nFound query with id #{id}"
+      end
+
+      if q.hidden
+        DataExplorer.pstore_delete "q:#{id}"
+        puts "Query no.#{id} has been deleted"
+      else
+        puts "Query no.#{id} must be hidden in order to hard delete"
+        puts "To hide the query, run: " + "rake data_explorer[#{id}]"
+      end
+    end
+  end
+  puts ""
+end

plugin.rb

@@ -7,6 +7,8 @@
 # url: https://github.com/discourse/discourse-data-explorer
 
 enabled_site_setting :data_explorer_enabled
+
+require File.expand_path('../lib/discourse_data_explorer/engine.rb', __FILE__)
 register_asset 'stylesheets/explorer.scss'
 
 if respond_to?(:register_svg_icon)
@@ -634,13 +636,14 @@ SQL
   # Reimplement a couple ActiveRecord methods, but use PluginStore for storage instead
   require_dependency File.expand_path('../lib/queries.rb', __FILE__)
   class DataExplorer::Query
-    attr_accessor :id, :name, :description, :sql, :created_by, :created_at, :group_ids, :last_run_at
+    attr_accessor :id, :name, :description, :sql, :created_by, :created_at, :group_ids, :last_run_at, :hidden
 
     def initialize
       @name = 'Unnamed Query'
       @description = ''
       @sql = 'SELECT 1'
       @group_ids = []
+      @hidden = false
     end
 
     def slug
@@ -687,6 +690,7 @@ SQL
       group_ids = (h[:group_ids] == "" || !h[:group_ids]) ? [] : h[:group_ids]
       query.group_ids = group_ids
       query.id = h[:id].to_i if h[:id]
+      query.hidden = h[:hidden]
       query
     end
 
@@ -699,7 +703,8 @@ SQL
         created_by: @created_by,
         created_at: @created_at,
         group_ids: @group_ids,
-        last_run_at: @last_run_at
+        last_run_at: @last_run_at,
+        hidden: @hidden
       }
     end
 
@@ -739,7 +744,10 @@ SQL
     end
 
     def destroy
-      DataExplorer.pstore_delete "q:#{id}"
+      # Instead of deleting the query from the store, we can set
+      # it to be hidden and not send it to the frontend
+      @hidden = true
+      DataExplorer.pstore_set "q:#{id}", to_hash
     end
 
     def read_attribute_for_serialization(attr)
@@ -1027,7 +1035,11 @@ SQL
 
     def index
       # guardian.ensure_can_use_data_explorer!
-      queries = DataExplorer::Query.all
+      queries = []
+      DataExplorer::Query.all.each do |query|
+        queries.push(query) unless query.hidden
+      end
+
       Queries.default.each do |params|
         query = DataExplorer::Query.new
         query.id = params.second["id"]
@@ -1120,11 +1132,12 @@ SQL
         end
       end
 
-      [:name, :sql, :description, :created_by, :created_at, :group_ids, :last_run_at].each do |sym|
+      [:name, :sql, :description, :created_by, :created_at, :group_ids, :last_run_at, :hidden].each do |sym|
         query.send("#{sym}=", hash[sym]) if hash[sym]
       end
 
       query.check_params!
+      query.hidden = false
       query.save
 
       render_serialized query, DataExplorer::QuerySerializer, root: 'query'
@@ -1261,7 +1274,7 @@ SQL
   end
 
   class DataExplorer::QuerySerializer < ActiveModel::Serializer
-    attributes :id, :sql, :name, :description, :param_info, :created_by, :created_at, :username, :group_ids, :last_run_at
+    attributes :id, :sql, :name, :description, :param_info, :created_by, :created_at, :username, :group_ids, :last_run_at, :hidden
 
     def param_info
       object.params.map(&:to_hash) rescue nil

spec/controllers/queries_controller_spec.rb

@@ -18,6 +18,7 @@ describe DataExplorer::QueryController do
     q.description = "A description for query number #{q.id}"
     q.group_ids = group_ids
     q.sql = sql
+    q.hidden = opts[:hidden] || false
     q.save
     q
   end
@@ -80,6 +81,16 @@ describe DataExplorer::QueryController do
         expect(response_json['queries'][0]['name']).to eq('A')
         expect(response_json['queries'][1]['name']).to eq('B')
       end
+
+      it "doesn't show hidden/deleted queries" do
+        DataExplorer::Query.destroy_all
+        make_query('SELECT 1 as value', name: 'A', hidden: false)
+        make_query('SELECT 1 as value', name: 'B', hidden: true)
+        make_query('SELECT 1 as value', name: 'C', hidden: true)
+        get :index, format: :json
+        expect(response.status).to eq(200)
+        expect(response_json['queries'].length).to eq(Queries.default.count + 1)
+      end
     end
 
     describe "#run" do

spec/lib/tasks/data_explorer_spec.rb

@@ -0,0 +1,197 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'Data Explorer rake tasks' do
+  before :all do
+    $stdout = File.open(File::NULL, 'w')
+  end
+
+  before do
+    Rake::Task.clear
+    Discourse::Application.load_tasks
+  end
+
+  def make_query(sql, opts = {}, group_ids = [])
+    q = DataExplorer::Query.new
+    q.id = opts[:id] || Fabrication::Sequencer.sequence("query-id", 1)
+    q.name = opts[:name] || "Query number #{q.id}"
+    q.description = "A description for query number #{q.id}"
+    q.group_ids = group_ids
+    q.sql = sql
+    q.hidden = opts[:hidden] || false
+    q.save
+    q
+  end
+
+  def hidden_queries
+    hidden_queries = []
+
+    DataExplorer::Query.all.each do |query|
+      hidden_queries.push(query) if query.hidden
+    end
+
+    hidden_queries
+  end
+
+  describe 'data_explorer' do
+    it 'hides a single query' do
+      DataExplorer::Query.destroy_all
+      make_query('SELECT 1 as value', id: 1, name: 'A')
+      make_query('SELECT 1 as value', id: 2, name: 'B')
+      # rake data_explorer[1] => hide query with ID 1
+      Rake::Task['data_explorer'].invoke(1)
+
+      # Soft deletion: PluginStoreRow should not be modified
+      expect(DataExplorer::Query.all.length).to eq(2)
+      # Array of hidden queries should have exactly 1 element
+      expect(hidden_queries.length).to eq(1)
+      # That one element should have the same ID as the one invoked to be hidden
+      expect(hidden_queries[0].id).to eq(1)
+    end
+
+    it 'hides multiple queries' do
+      DataExplorer::Query.destroy_all
+      make_query('SELECT 1 as value', id: 1, name: 'A')
+      make_query('SELECT 1 as value', id: 2, name: 'B')
+      make_query('SELECT 1 as value', id: 3, name: 'C')
+      make_query('SELECT 1 as value', id: 4, name: 'D')
+      # rake data_explorer[1,2,4] => hide queries with IDs 1, 2 and 4
+      Rake::Task['data_explorer'].invoke(1, 2, 4)
+
+      # Soft deletion: PluginStoreRow should not be modified
+      expect(DataExplorer::Query.all.length).to eq(4)
+      # Array of hidden queries should have the same number of elements invoked to be hidden
+      expect(hidden_queries.length).to eq(3)
+      # The elements should have the same ID as the ones invoked to be hidden
+      expect(hidden_queries[0].id).to eq(1)
+      expect(hidden_queries[1].id).to eq(2)
+      expect(hidden_queries[2].id).to eq(4)
+    end
+
+    context 'query does not exist in PluginStore' do
+      it 'should not hide the query' do
+        DataExplorer::Query.destroy_all
+        make_query('SELECT 1 as value', id: 1, name: 'A')
+        make_query('SELECT 1 as value', id: 2, name: 'B')
+        # rake data_explorer[3] => try to hide query with ID 3
+        Rake::Task['data_explorer'].invoke(3)
+        # rake data_explorer[3,4,5] => try to hide queries with IDs 3, 4 and 5
+        Rake::Task['data_explorer'].invoke(3, 4, 5)
+
+        # Array of hidden queries should be empty
+        expect(hidden_queries.length).to eq(0)
+      end
+    end
+  end
+
+  describe '#unhide_query' do
+    it 'unhides a single query' do
+      DataExplorer::Query.destroy_all
+      make_query('SELECT 1 as value', id: 1, name: 'A', hidden: true)
+      make_query('SELECT 1 as value', id: 2, name: 'B', hidden: true)
+      # rake data_explorer:unhide_query[1] => unhide query with ID 1
+      Rake::Task['data_explorer:unhide_query'].invoke(1)
+
+      # Soft deletion: PluginStoreRow should not be modified
+      expect(DataExplorer::Query.all.length).to eq(2)
+      # Array of hidden queries should have exactly 1 element
+      expect(hidden_queries.length).to eq(1)
+      # There should be one remaining element that is still hidden
+      expect(hidden_queries[0].id).to eq(2)
+    end
+
+    it 'unhides multiple queries' do
+      DataExplorer::Query.destroy_all
+      make_query('SELECT 1 as value', id: 1, name: 'A', hidden: true)
+      make_query('SELECT 1 as value', id: 2, name: 'B', hidden: true)
+      make_query('SELECT 1 as value', id: 3, name: 'C', hidden: true)
+      make_query('SELECT 1 as value', id: 4, name: 'D', hidden: true)
+      # rake data_explorer:unhide_query[1,2,4] => unhide queries with IDs 1, 2 and 4
+      Rake::Task['data_explorer:unhide_query'].invoke(1, 2, 4)
+
+      # Soft deletion: PluginStoreRow should not be modified
+      expect(DataExplorer::Query.all.length).to eq(4)
+      # Array of hidden queries should have exactly 1 element
+      expect(hidden_queries.length).to eq(1)
+      # There should be one remaining element that is still hidden
+      expect(hidden_queries[0].id).to eq(3)
+    end
+
+    context 'query does not exist in PluginStore' do
+      it 'should not unhide the query' do
+        DataExplorer::Query.destroy_all
+        make_query('SELECT 1 as value', id: 1, name: 'A', hidden: true)
+        make_query('SELECT 1 as value', id: 2, name: 'B', hidden: true)
+        # rake data_explorer:unhide_query[3] => try to unhide query with ID 3
+        Rake::Task['data_explorer:unhide_query'].invoke(3)
+        # rake data_explorer:unhide_query[3,4,5] => try to unhide queries with IDs 3, 4 and 5
+        Rake::Task['data_explorer:unhide_query'].invoke(3, 4, 5)
+
+        # Array of hidden queries shouldn't change
+        expect(hidden_queries.length).to eq(2)
+      end
+    end
+  end
+
+  describe '#hard_delete' do
+    it 'hard deletes a single query' do
+      DataExplorer::Query.destroy_all
+      make_query('SELECT 1 as value', id: 1, name: 'A', hidden: true)
+      make_query('SELECT 1 as value', id: 2, name: 'B', hidden: true)
+      # rake data_explorer:hard_delete[1] => hard delete query with ID 1
+      Rake::Task['data_explorer:hard_delete'].invoke(1)
+
+      # Hard deletion: query list should be shorter by 1
+      expect(DataExplorer::Query.all.length).to eq(1)
+      # Array of hidden queries should have exactly 1 element
+      expect(hidden_queries.length).to eq(1)
+      # There should be one remaining hidden element
+      expect(hidden_queries[0].id).to eq(2)
+    end
+
+    it 'hard deletes multiple queries' do
+      DataExplorer::Query.destroy_all
+      make_query('SELECT 1 as value', id: 1, name: 'A', hidden: true)
+      make_query('SELECT 1 as value', id: 2, name: 'B', hidden: true)
+      make_query('SELECT 1 as value', id: 3, name: 'C', hidden: true)
+      make_query('SELECT 1 as value', id: 4, name: 'D', hidden: true)
+      # rake data_explorer:hard_delete[1,2,4] => hard delete queries with IDs 1, 2 and 4
+      Rake::Task['data_explorer:hard_delete'].invoke(1, 2, 4)
+
+      # Hard deletion: query list should be shorter by 3
+      expect(DataExplorer::Query.all.length).to eq(1)
+      # Array of hidden queries should have exactly 1 element
+      expect(hidden_queries.length).to eq(1)
+      # There should be one remaining hidden element
+      expect(hidden_queries[0].id).to eq(3)
+    end
+
+    context 'query does not exist in PluginStore' do
+      it 'should not hard delete the query' do
+        DataExplorer::Query.destroy_all
+        make_query('SELECT 1 as value', id: 1, name: 'A', hidden: true)
+        make_query('SELECT 1 as value', id: 2, name: 'B', hidden: true)
+        # rake data_explorer:hard_delete[3] => try to hard delete query with ID 3
+        Rake::Task['data_explorer:hard_delete'].invoke(3)
+        # rake data_explorer:hard_delete[3,4,5] => try to hard delete queries with IDs 3, 4 and 5
+        Rake::Task['data_explorer:hard_delete'].invoke(3, 4, 5)
+
+        # Array of hidden queries shouldn't change
+        expect(hidden_queries.length).to eq(2)
+      end
+    end
+
+    context 'query is not hidden' do
+      it 'should not hard delete the query' do
+        DataExplorer::Query.destroy_all
+        make_query('SELECT 1 as value', id: 1, name: 'A')
+        # rake data_explorer:hard_delete[1] => try to hard delete query with ID 1
+        Rake::Task['data_explorer:hard_delete'].invoke(1)
+
+        # List of queries shouldn't change
+        expect(DataExplorer::Query.all.length).to eq(1)
+      end
+    end
+  end
+end