From 819f299d3bdc2485394120583b8a76a67b10b418 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miquel=20Sabat=C3=A9=20Sol=C3=A0?= <msabate@suse.com>
Date: Wed, 20 Mar 2024 16:00:18 +0100
Subject: [PATCH] tags: attempt to modify searches on tag rename
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We don't want to break existing searches upon renaming a tag, since most
of searches will be based on tag names.

NOTE: if the UI of searches is improved, maybe the `tags:""` clause can
contain tag ids instead of names, and hence we won't need all this dance
afterwards.

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
---
 app/controllers/tags_controller.rb | 22 +++++++++++++++++++++-
 config/locales/ca.yml              |  3 +++
 config/locales/en.yml              |  3 +++
 test/fixtures/searches.yml         | 12 +++++++++++-
 test/system/searches_test.rb       |  2 +-
 test/system/tags_test.rb           | 12 ++++++++++++
 6 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb
index 9d87687..ee837ef 100644
--- a/app/controllers/tags_controller.rb
+++ b/app/controllers/tags_controller.rb
@@ -24,11 +24,31 @@ def create
   end
 
   def update
-    if @tag.update(tag_params)
+    updated = false
+
+    # An update on the tag name can also mean that we need to modify existing
+    # saved searches so we don't break them. Thus, everything needs to pass if
+    # we want this action to succeed. For searches it's fine to `#update!` with
+    # a bang and let the `rescue` below throw a generic error message. For
+    # `@tag` itself we can go the usual Rails-way, but since we don't want
+    # errors to be mixed together, we call `#update` without a bang and save the
+    # result on a boolean variable.
+    ActiveRecord::Base.transaction do
+      Search.where('body LIKE ?', "tag:\"#{@tag.name}\"").find_each do |s|
+        body = s.body.gsub("tag:\"#{@tag.name}\"", "tag:\"#{tag_params['name']}\"")
+        s.update!(body:)
+      end
+
+      updated = @tag.update(tag_params)
+    end
+
+    if updated
       redirect_to tags_url, notice: t('tags.update-success')
     else
       render :edit, status: :unprocessable_entity
     end
+  rescue ActiveRecord::RecordInvalid
+    redirect_to edit_tag_path(@tag), alert: t('tags.update-fail')
   end
 
   def destroy
diff --git a/config/locales/ca.yml b/config/locales/ca.yml
index b9f9b7c..0bfe01e 100644
--- a/config/locales/ca.yml
+++ b/config/locales/ca.yml
@@ -30,6 +30,8 @@ ca:
         editors: Editors d'una col·lecció
         bought_at: Data de compra
         where_is_it: Per on para?
+      search:
+        body: Cos de la cerca
       user:
         username: Nom d'usuari
         password: Contrasenya
@@ -113,6 +115,7 @@ ca:
 
     create-success: Etiqueta creada correctament
     update-success: S'ha canviat el nom de l'etiqueta correctament
+    update-fail: "No s'ha pogut canviar el nom de l'etiqueta perquè comportaria problemes en cerques existents. Modifica primer les cerques que tinguin en compte aquest nom d'etiqueta"
     destroy-success: Etiqueta esborrada correctament
 
   things:
diff --git a/config/locales/en.yml b/config/locales/en.yml
index e720a0c..7bfe478 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -30,6 +30,8 @@ en:
         editors: Editors of a collection
         bought_at: Bought at
         where_is_it: Where is it?
+      search:
+        body: Body
       user:
         username: Username
         password: Password
@@ -113,6 +115,7 @@ en:
 
     create-success: Tag was successfully created
     update-success: Tag was successfully updated
+    update-fail: Tag could not be updated because of conflicts with some searches. Update first searches which contain this tag name
     destroy-success: Tag was successfully deleted
 
   things:
diff --git a/test/fixtures/searches.yml b/test/fixtures/searches.yml
index ffc7a28..81af270 100644
--- a/test/fixtures/searches.yml
+++ b/test/fixtures/searches.yml
@@ -1,4 +1,14 @@
 search1:
   name: 'search1'
-  body: "tag:\"tag1\""
+  body: 'tag:"tag1"'
+  user_id: <%= ActiveRecord::FixtureSet.identify(:user) %>
+
+search2:
+  name: 'search2'
+  body: 'tag:"tag2"'
+  user_id: <%= ActiveRecord::FixtureSet.identify(:user) %>
+
+search3:
+  name: 'search3'
+  body: 'tag:"unknown"'
   user_id: <%= ActiveRecord::FixtureSet.identify(:user) %>
diff --git a/test/system/searches_test.rb b/test/system/searches_test.rb
index 309d51c..4920102 100644
--- a/test/system/searches_test.rb
+++ b/test/system/searches_test.rb
@@ -127,6 +127,6 @@ class SearchesTest < ApplicationSystemTestCase
     accept_alert { click_on I18n.t('general.delete') }
 
     assert_selector 'a', text: searches(:search1).name, count: 0
-    assert_predicate Search, :none?
+    assert_equal searches.size - 1, Search.count
   end
 end
diff --git a/test/system/tags_test.rb b/test/system/tags_test.rb
index 7e2100f..15d1c3e 100644
--- a/test/system/tags_test.rb
+++ b/test/system/tags_test.rb
@@ -47,6 +47,9 @@ class SharedSearchesTest < ApplicationSystemTestCase
 
     assert_text I18n.t('tags.update-success')
     assert_text "#{tags(:tag1).name}-updated"
+
+    # Searches are also updated on tag renames.
+    assert_equal "tag:\"#{tags(:tag1).name}-updated\"", searches(:search1).body
   end
 
   test 'gives feedback on update errors' do
@@ -58,6 +61,15 @@ class SharedSearchesTest < ApplicationSystemTestCase
     assert_text "#{I18n.t('activerecord.attributes.tag.name')} #{I18n.t('errors.messages.taken')}"
   end
 
+  test 'gives feedback when any of the searches could not be updated because of the tag rename' do
+    visit edit_tag_url(tags(:tag1))
+
+    fill_in I18n.t('activerecord.attributes.tag.name'), with: 'unknown'
+    click_on I18n.t('helpers.submit.update')
+
+    assert_text I18n.t('tags.update-fail')
+  end
+
   test 'can delete an existing tag' do
     visit tags_url