From 53949733460dd80481a419e280d32f1da7317c47 Mon Sep 17 00:00:00 2001 From: Jyri-Petteri Paloposki Date: Tue, 25 Jun 2019 03:15:23 +0300 Subject: [PATCH] #1955: Migrate tags to belong to named users for enhanced privacy. --- app/controllers/todos_controller.rb | 11 ++- .../abstract_recurring_todos_builder.rb | 2 +- app/models/tag.rb | 4 +- app/models/todo.rb | 2 +- app/models/user.rb | 1 + app/services/todo_from_rich_message.rb | 2 +- .../20190618202817_add_user_id_to_tag.rb | 72 +++++++++++++++++++ db/schema.rb | 3 +- lib/is_taggable.rb | 24 +++---- 9 files changed, 97 insertions(+), 24 deletions(-) create mode 100644 db/migrate/20190618202817_add_user_id_to_tag.rb diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index 7a3fa8b4..e1cbcb7c 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -106,7 +106,7 @@ class TodosController < ApplicationController if @todo.errors.empty? @todo.add_predecessor_list(p.predecessor_list) @saved = @todo.save - @todo.tag_with(tag_list) if @saved && tag_list.present? + @todo.tag_with(tag_list, current_user) if @saved && tag_list.present? @todo.block! if @todo.uncompleted_predecessors? else @saved = false @@ -194,7 +194,7 @@ class TodosController < ApplicationController todo.block! end - todo.tag_with(tag_list) if @saved && tag_list.present? + todo.tag_with(tag_list, current_user) if @saved && tag_list.present? @todos << todo @not_done_todos << todo if p.new_context_created || p.new_project_created @@ -712,9 +712,8 @@ class TodosController < ApplicationController def tags - # TODO: limit to current_user - tags_beginning = Tag.where(Tag.arel_table[:name].matches("#{params[:term]}%")) - tags_all = Tag.where(Tag.arel_table[:name].matches("%#{params[:term]}%")) + tags_beginning = current_user.tags.where(Tag.arel_table[:name].matches("#{params[:term]}%")) + tags_all = current_user.tags.where(Tag.arel_table[:name].matches("%#{params[:term]}%")) tags_all = tags_all - tags_beginning respond_to do |format| @@ -1219,7 +1218,7 @@ end def update_tags if params[:tag_list] - @todo.tag_with(params[:tag_list]) + @todo.tag_with(params[:tag_list], current_user) @todo.tags.reload #force a reload for proper rendering end end diff --git a/app/models/recurring_todos/abstract_recurring_todos_builder.rb b/app/models/recurring_todos/abstract_recurring_todos_builder.rb index 5c367d14..46331acf 100644 --- a/app/models/recurring_todos/abstract_recurring_todos_builder.rb +++ b/app/models/recurring_todos/abstract_recurring_todos_builder.rb @@ -129,7 +129,7 @@ module RecurringTodos end def save_tags - @recurring_todo.tag_with(@filterred_attributes[:tag_list]) if @filterred_attributes[:tag_list].present? + @recurring_todo.tag_with(@filterred_attributes[:tag_list], @user) if @filterred_attributes[:tag_list].present? @recurring_todo.reload end diff --git a/app/models/tag.rb b/app/models/tag.rb index aa193c20..2c710dad 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -3,13 +3,15 @@ class Tag < ApplicationRecord has_many :taggings has_many :taggable, :through => :taggings + belongs_to :user + DELIMITER = ",".freeze # Controls how to split and join tagnames from strings. You may need to change the validates_format_of parameters if you change this. JOIN_DELIMITER = ", ".freeze # If database speed becomes an issue, you could remove these validations and # rescue the ActiveRecord database constraint errors instead. validates_presence_of :name - validates_uniqueness_of :name, :case_sensitive => false + validates_uniqueness_of :name, :scope => "user_id", :case_sensitive => false before_create :before_create diff --git a/app/models/todo.rb b/app/models/todo.rb index c69bb69c..e31e5f6d 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -349,7 +349,7 @@ class Todo < ApplicationRecord def add_tags=(params) unless params[:tag].nil? tag_list = params[:tag].inject([]) { |list, value| list << value[:name] } - tag_with tag_list.join(", ") + tag_with tag_list.join(", "), user end end diff --git a/app/models/user.rb b/app/models/user.rb index 99a54528..736db95e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -94,6 +94,7 @@ class User < ApplicationRecord end end + has_many :tags, dependent: :delete_all has_many :notes, -> { order "created_at DESC" }, dependent: :delete_all has_one :preference, dependent: :destroy has_many :attachments, through: :todos diff --git a/app/services/todo_from_rich_message.rb b/app/services/todo_from_rich_message.rb index bf567797..50668c86 100644 --- a/app/services/todo_from_rich_message.rb +++ b/app/services/todo_from_rich_message.rb @@ -50,7 +50,7 @@ class TodoFromRichMessage todo.project_id = project_id unless project_id.nil? todo.show_from = show_from if show_from.is_a? Time todo.due = due if due.is_a? Time - todo.tag_with tags unless tags.nil? || tags.empty? + todo.tag_with tags, @user unless tags.nil? || tags.empty? todo.starred = star todo end diff --git a/db/migrate/20190618202817_add_user_id_to_tag.rb b/db/migrate/20190618202817_add_user_id_to_tag.rb new file mode 100644 index 00000000..627e75f4 --- /dev/null +++ b/db/migrate/20190618202817_add_user_id_to_tag.rb @@ -0,0 +1,72 @@ +class AddUserIdToTag < ActiveRecord::Migration[5.2] + def self.up + add_column :tags, :user_id, :integer + + # Find uses of each tag for both Todos and RecurringTodos to + # figure out which users use which tags. + @tags = execute <<-EOQ + SELECT t.id AS tid, tds.user_id AS todo_uid, rt.user_id AS rtodo_uid + FROM tags t + JOIN taggings tgs ON tgs.tag_id = t.id + LEFT OUTER JOIN todos tds + ON tgs.taggable_type = "Todo" AND tds.id = tgs.taggable_id + LEFT OUTER JOIN recurring_todos rt + ON tgs.taggable_type = "RecurringTodo" AND rt.id = tgs.taggable_id + WHERE rt.id IS NOT NULL OR tds.id IS NOT NULL + GROUP BY t.id, tds.user_id, rt.user_id + EOQ + + # Map each tag to the users using it. + @tag_users = {} + @tags.each do |row| + uid = (row[1] ? row[1] : row[2]) + if not @tag_users[row[0]] + @tag_users[row[0]] = [uid] + elsif not @tag_users[row[0]].include? uid + @tag_users[row[0]] << uid + end + end + + # Go through the tags assigning users and duplicating as necessary. + @tag_users.each do |tid, uids| + tag = Tag.find(tid) + + # One of the users will get the original tag instance, but first + # duplicate their own copy to all the others. + extras = uids.length - 1 + extras.times do |n| + uid = uids[n+1] + + # Create a duplicate of the tag assigned to the user. + new_tag = tag.dup + new_tag.user_id = uid + new_tag.save! + + # Move all the user's regular todos to the new tag. + connection.execute(<<-EOQ) + UPDATE taggings ta + JOIN todos t + ON ta.taggable_type = "Todo" AND t.id = ta.taggable_id + SET ta.tag_id = #{new_tag.id} + WHERE t.user_id = #{uid} + EOQ + + # Move all the user's recurring todos to the new tag. + connection.execute(<<-EOQ) + UPDATE taggings ta + JOIN recurring_todos t + ON ta.taggable_type = "RecurringTodo" AND t.id = ta.taggable_id + SET ta.tag_id = #{new_tag.id} + WHERE t.user_id = #{uid} + EOQ + end + + tag.user_id = uids[0] + tag.save! + end + end + def self.down + remove_column :tags, :user_id + end +end + diff --git a/db/schema.rb b/db/schema.rb index 93a6c96f..d961d407 100644 --- 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: 2016_01_31_233303) do +ActiveRecord::Schema.define(version: 2019_06_18_202817) do create_table "attachments", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8", force: :cascade do |t| t.integer "todo_id" @@ -159,6 +159,7 @@ ActiveRecord::Schema.define(version: 2016_01_31_233303) do t.string "name" t.datetime "created_at" t.datetime "updated_at" + t.integer "user_id" t.index ["name"], name: "index_tags_on_name" end diff --git a/lib/is_taggable.rb b/lib/is_taggable.rb index 122f0409..8b9e79a0 100644 --- a/lib/is_taggable.rb +++ b/lib/is_taggable.rb @@ -15,43 +15,43 @@ module IsTaggable self.to_a.reject{|tag| tag.name == Todo::STARRED_TAG_NAME} end end - + def tag_list tags.reload tags.to_s end - + def tag_list=(value) tag_with(value) end - + # Replace the existing tags on self. Accepts a string of tagnames, an array of tagnames, or an array of Tags. - def tag_with list + def tag_with(list, user) list = tag_cast_to_string(list) - + # Transactions may not be ideal for you here; be aware. Tag.transaction do current = tags.to_a.map(&:name) - _add_tags(list - current) + _add_tags(list - current, user) _remove_tags(current - list) end - + self end def has_tag?(tag_name) return tags.any? {|tag| tag.name == tag_name} end - + # Add tags to self. Accepts a string of tagnames, an array of tagnames, or an array of Tags. # # We need to avoid name conflicts with the built-in ActiveRecord association methods, thus the underscores. - def _add_tags incoming + def _add_tags(incoming, user) tag_cast_to_string(incoming).each do |tag_name| # added following check to prevent empty tags from being saved (which will fail) if tag_name.present? begin - tag = Tag.where(:name => tag_name).first_or_create + tag = user.tags.where(:name => tag_name).first_or_create raise Tag::Error, "tag could not be saved: #{tag_name}" if tag.new_record? tags << tag rescue ActiveRecord::StatementInvalid => e @@ -66,7 +66,7 @@ module IsTaggable outgoing = tag_cast_to_string(outgoing) tags.destroy(*(tags.select{|tag| outgoing.include? tag.name})) end - + def get_tag_name_from_item(item) case item # removed next line as it prevents using numbers as tags @@ -94,8 +94,6 @@ module IsTaggable raise "Invalid object of class #{obj.class} as tagging method parameter" end end - end end - end