Remove query soft delete (o19s#299)

* harder than expected to get the right incantation to delete old rows * remove code. * rubocop * document change
reid-rigo · Mar 26, 2021 · 4737ba2 · 4737ba2
1 parent dde339f
commit 4737ba2
Show file tree

Hide file tree

Showing 17 changed files with 79 additions and 42 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,9 @@
 
 * _Unit Test_ style custom scorers let you run a unit test that asserted specific things about specific docs at specific ranks in your search results.   This logic however was always convoluted, and only 15 people since 2019 have used it, and I suspect by accident ;-)   We want better ways of handling this type of function, so removing this to pay down some tech debt, simplify the database architecture, and open the door to new approach.  https://github.com/o19s/quepid/pull/296 by @epugh fixes https://github.com/o19s/quepid/issues/290.
 
+* We have removed the Soft Delete for Queries to simplify how Quepid works.  If you delete a query in Quepid it is now fully deleted from the database!  This is a nice bit of paying down tech debt.  Huge thanks to @DmitryKey for testing this PR.  https://github.com/o19s/quepid/pull/299 by @epugh fixes https://github.com/o19s/quepid/issues/298 by @DmitryKey.
+
+
 ### Bugs
 
 * You can export a rating that has no actual rating value chosen! https://github.com/o19s/quepid/pull/266 by @epugh fixes https://github.com/o19s/quepid/issues/265.

diff --git a/NOTES_REMOVING_UNIT_TEST.md b/NOTES_REMOVING_UNIT_TEST.md
@@ -4,3 +4,6 @@ scorerEnbl
 
 
 check teh cases scorers logif fo chte ommucunal scorer versus not..  do we need teams?
+
+
+When you delete a query, we nuke it from snapshots.  Do we want tot do that?  I think yes!
diff --git a/app/assets/javascripts/services/queriesSvc.js b/app/assets/javascripts/services/queriesSvc.js
@@ -812,7 +812,6 @@ angular.module('QuepidApp')
       this.createQuery = function(queryText) {
         var queryJson = {
           'query_text': queryText,
-          deleted:      false,
           queryId:      -1
         };
         var newQuery = new Query(queryJson);

diff --git a/app/controllers/api/v1/import/ratings_controller.rb b/app/controllers/api/v1/import/ratings_controller.rb
@@ -15,6 +15,9 @@ def create
           file_format = params[:file_format]
           file_format = 'hash' unless params[:file_format]
 
+          bool = ActiveRecord::Type::Boolean.new
+          clear_queries = bool.deserialize(params[:clear_queries]) || false
+
           case file_format
           when 'hash'
             # convert from ActionController::Parameters to a Hash, symbolize, and
@@ -64,7 +67,7 @@ def create
           options = {
             format:         :hash,
             force:          true,
-            clear_existing: params[:clear_queries] || false,
+            clear_existing: clear_queries,
             show_progress:  false,
           }
 

diff --git a/app/controllers/api/v1/queries_controller.rb b/app/controllers/api/v1/queries_controller.rb
@@ -72,7 +72,7 @@ def update
 
       def destroy
         @query.remove_from_list
-        @query.soft_delete
+        @query.destroy
         Analytics::Tracker.track_query_deleted_event current_user, @query
 
         # Make sure queries have the right `arranged_next` and `arranged_at`

diff --git a/app/models/case.rb b/app/models/case.rb
@@ -196,7 +196,6 @@ def clone_query query, clone_ratings
     new_query = ::Query.new(
       arranged_next:  query.arranged_next,
       arranged_at:    query.arranged_at,
-      deleted:        query.deleted,
       query_text:     query.query_text,
       notes:          query.notes,
       threshold:      query.threshold,

diff --git a/app/models/query.rb b/app/models/query.rb
@@ -7,7 +7,6 @@
 #  id             :integer          not null, primary key
 #  arranged_next  :integer
 #  arranged_at    :integer
-#  deleted        :boolean
 #  query_text     :string(191)
 #  notes          :text(65535)
 #  threshold      :float(24)
@@ -30,20 +29,14 @@ class Query < ApplicationRecord
   has_many    :ratings,
               dependent: :destroy
 
+  has_many    :snapshot_queries,
+              dependent: :destroy
+
   # Validations
   validates :query_text,
             presence: true
 
   # Scopes
-  # Lot of folks say don't use default_scopes since if you do case.queries you down't see deleted queries!
-  default_scope -> { where(deleted: false).or(where(deleted: nil)) }
-
-  # TODO: use the acts_as_paranoid gem instead
-  # Which requires change to the db, that is not going to be done in the
-  # initial scope of work in the rails migration
-  def soft_delete
-    update deleted: true
-  end
 
   def parent_list
     self.case.queries

diff --git a/app/services/ratings_importer.rb b/app/services/ratings_importer.rb
@@ -144,7 +144,7 @@ def import
     print_step 'Clearing unused queries'
 
     @acase.queries.each do |query|
-      query.soft_delete if @queries[query.query_text].blank?
+      query.destroy if @queries[query.query_text].blank?
     end
   end
   # rubocop:enable Metrics/PerceivedComplexity

diff --git a/app/views/api/v1/queries/_query.json.jbuilder b/app/views/api/v1/queries/_query.json.jbuilder
@@ -2,7 +2,6 @@
 
 json.arrangedAt         query.arranged_at
 json.arrangedNext       query.arranged_next
-json.deleted            query.deleted
 json.queryId            query.id
 json.query_text         query.query_text
 json.threshold          query.threshold

diff --git a/db/migrate/20210321113736_remove_soft_deleted_queries.rb b/db/migrate/20210321113736_remove_soft_deleted_queries.rb
@@ -0,0 +1,39 @@
+class RemoveSoftDeletedQueries < ActiveRecord::Migration[5.2]
+  def change
+
+    # Delete from the database any snapshot related data for soft deleted queries.
+    RemoveSoftDeletedQueries.connection.execute(
+      "
+      delete from snapshot_docs
+      using queries inner join snapshot_queries  inner join snapshot_docs
+      where queries.deleted = 1 and queries.id = snapshot_queries.query_id and snapshot_queries.id = snapshot_docs.snapshot_query_id
+      "
+    )
+
+    RemoveSoftDeletedQueries.connection.execute(
+      "
+      delete from snapshot_queries
+      using queries inner join snapshot_queries
+      where queries.deleted = 1 and queries.id = snapshot_queries.query_id
+      "
+    )
+
+    # Delete the soft deleted queries.
+    RemoveSoftDeletedQueries.connection.execute(
+      "
+      delete from ratings
+      using queries inner join ratings
+      where queries.deleted = 1 and queries.id = ratings.query_id
+      "
+    )
+
+    RemoveSoftDeletedQueries.connection.execute(
+      "
+      delete from queries
+      where queries.deleted = 1
+      "
+    )
+
+    remove_column :queries, :deleted
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2021_03_16_172643) do
+ActiveRecord::Schema.define(version: 2021_03_21_113736) do
 
   create_table "annotations", id: :integer, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8", force: :cascade do |t|
     t.text "message"
@@ -90,7 +90,6 @@
   create_table "queries", id: :integer, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin", force: :cascade do |t|
     t.bigint "arranged_next"
     t.bigint "arranged_at"
-    t.boolean "deleted"
     t.string "query_text", limit: 500
     t.text "notes"
     t.float "threshold"

diff --git a/docs/database.md b/docs/database.md
@@ -14,3 +14,15 @@ Or if you have a Zip file:
 ```
 unzip -p quepid_prod_2021_03_02.sql.zip | mysql --host=127.0.0.1 --port=3306 -u root -p quepid_development
 ```
+
+## Emoji Support
+
+Both the `scorers` and `queries` tables have columns that support using emojis.   To do this, they need
+a different set of options when creating the tables:
+
+```
+CHARSET=utf8mb4 COLLATE=utf8mb4_bin
+```
+
+If your migration add/drops a table, you may need to edit `schema.rb` to restore that definition.  Your tests
+will fail fortunately ;-).
diff --git a/test/controllers/api/v1/import/ratings_controller_test.rb b/test/controllers/api/v1/import/ratings_controller_test.rb
@@ -117,15 +117,13 @@ class RatingsControllerTest < ActionController::TestCase
                 { query_text: 'dog', doc_id: '456', rating: 3 }
               ],
             }
+            assert_not query.destroyed?
             assert_no_difference 'acase.queries.count' do
               post :create, params: data
 
               assert_response :ok
             end
-
-            query.reload
-
-            assert query.deleted
+            assert_not Query.exists?(query.id)
           end
         end
         describe '#create from RRE' do

diff --git a/test/controllers/api/v1/snapshots_controller_test.rb b/test/controllers/api/v1/snapshots_controller_test.rb
@@ -209,7 +209,7 @@ class SnapshotsControllerTest < ActionController::TestCase
 
         test 'returns snapshot when a query is deleted' do
           query_count = acase.queries.size
-          acase.queries.first.soft_delete
+          acase.queries.first.destroy
           acase.save!
 
           get :show, params: { case_id: acase.id, id: snapshot.id }

diff --git a/test/fixtures/queries.yml b/test/fixtures/queries.yml
@@ -5,7 +5,6 @@
 #  id             :integer          not null, primary key
 #  arranged_next  :integer
 #  arranged_at    :integer
-#  deleted        :boolean
 #  query_text     :string(191)
 #  notes          :text(65535)
 #  threshold      :float(24)

diff --git a/test/models/case_test.rb b/test/models/case_test.rb
@@ -237,10 +237,10 @@ class CaseTest < ActiveSupport::TestCase
       assert_not user.destroyed?
     end
 
-    it 'handles soft deleted queries' do
+    it 'handles destroyed queries' do
       assert_difference 'Query.count', -2 do
         assert_equal 2, the_case.queries.size
-        the_case.queries.first.soft_delete
+        the_case.queries.first.destroy
         assert_equal 1, the_case.queries.size
         the_case.really_destroy
         assert the_case.destroyed?

diff --git a/test/models/query_test.rb b/test/models/query_test.rb
@@ -7,7 +7,6 @@
 #  id             :integer          not null, primary key
 #  arranged_next  :integer
 #  arranged_at    :integer
-#  deleted        :boolean
 #  query_text     :string(191)
 #  notes          :text(65535)
 #  threshold      :float(24)
@@ -174,20 +173,20 @@ class QueryTest < ActiveSupport::TestCase
     end
   end
 
-  describe 'Soft deletion' do
+  describe 'Deletion' do
     let(:query) { queries(:one) }
 
     test 'marks query as deleted but does not actually delete query' do
-      assert_difference 'Query.unscoped.where(deleted: true).count' do
-        query.soft_delete
-        query.reload
+      assert_difference 'Query.count', -1 do
+        query.destroy
 
-        assert_equal query.deleted, true
+        assert query.destroyed?
       end
     end
 
-    test 'does not fetch queries marked as deleted by default' do
-      query.soft_delete
+    test 'does not fetch queries destroyed' do
+      query.destroy
+      assert query.destroyed?
 
       queries = Query.all
       ids     = queries.map(&:id)
@@ -201,14 +200,6 @@ class QueryTest < ActiveSupport::TestCase
 
       assert_includes ids, query.id
     end
-
-    test 'returns query if deleted is marked as false' do
-      query.update deleted: false
-      queries = Query.all
-      ids     = queries.map(&:id)
-
-      assert_includes ids, query.id
-    end
   end
 
   describe 'query scoping with ratings and with ratings' do