Skip to content

Commit

Permalink
tags: attempt to modify searches on tag rename
Browse files Browse the repository at this point in the history
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à <[email protected]>
  • Loading branch information
mssola committed Mar 20, 2024
1 parent ccf5888 commit 819f299
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 3 deletions.
22 changes: 21 additions & 1 deletion app/controllers/tags_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions config/locales/ca.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
12 changes: 11 additions & 1 deletion test/fixtures/searches.yml
Original file line number Diff line number Diff line change
@@ -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) %>
2 changes: 1 addition & 1 deletion test/system/searches_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 12 additions & 0 deletions test/system/tags_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down

0 comments on commit 819f299

Please sign in to comment.