Merge pull request opf#9609 from opf/user-preferences-as-json

Migrate UserPreference to JSON storage

spec_legacy/fixtures/user_preferences.yml → app/models/serializers/indifferent_hash_serializer.rb

@@ -1,3 +1,5 @@
+#-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) 2012-2021 the OpenProject GmbH
@@ -25,39 +27,15 @@
 #
 # See docs/COPYRIGHT.rdoc for more details.
 #++
+module Serializers
+  class IndifferentHashSerializer
+    def self.dump(hash)
+      hash
+    end
 
----
-user_preferences_001:
-  others: |
-    ---
-    :my_page_layout:
-      left:
-      - latestnews
-      right:
-      - issuesassignedtome
-      top:
-      - calendar
-
-  id: 1
-  user_id: 1
-  hide_mail: true
-user_preferences_002:
-  others: |
-    ---
-    :my_page_layout:
-      left:
-      - latestnews
-      right:
-      - issuesassignedtome
-      top:
-      - calendar
-
-  id: 2
-  user_id: 3
-  hide_mail: false
-user_preferences_003:
-  others: |
-    ---
-  id: 3
-  user_id: 2
-  hide_mail: false
+    def self.load(value)
+      hash = value.is_a?(Hash) ? value : {}
+      hash.with_indifferent_access
+    end
+  end
+end

app/models/user_preference.rb

@@ -30,50 +30,79 @@
 
 class UserPreference < ApplicationRecord
   belongs_to :user
-  serialize :others
-
   delegate :notification_settings, to: :user
+  serialize :settings, ::Serializers::IndifferentHashSerializer
 
   validates_presence_of :user
   validate :time_zone_correctness, if: -> { time_zone.present? }
 
-  after_initialize :init_other_preferences
+  ##
+  # Retrieve keys from settings, and allow accessing
+  # as boolean with ? suffix
+  def method_missing(method_name, *args)
+    key = method_name.to_s
+    action = key[-1]
+
+    case action
+    when '?'
+      to_boolean send(key[..-2])
+    when '='
+      settings[key[..-2]] = args.first
+    else
+      settings[key]
+    end
+  end
+
+  ##
+  # We respond to all methods as we retrieve
+  # the key from settings
+  def respond_to_missing?(*)
+    true
+  end
 
   def [](attr_name)
-    attribute?(attr_name) ? super : others[attr_name]
+    if attribute?(attr_name)
+      super
+    else
+      send attr_name
+    end
   end
 
   def []=(attr_name, value)
-    attribute?(attr_name) ? super : others[attr_name] = value
+    if attribute?(attr_name)
+      super
+    else
+      send :"#{attr_name}=", value
+    end
   end
 
   def comments_sorting
-    others.fetch(:comments_sorting, OpenProject::Configuration.default_comment_sort_order)
-  end
-
-  def comments_sorting=(order)
-    others[:comments_sorting] = order
+    settings.fetch(:comments_sorting, OpenProject::Configuration.default_comment_sort_order)
   end
 
   def comments_in_reverse_order?
     comments_sorting == 'desc'
   end
 
+  def hide_mail
+    settings.fetch(:hide_mail, true)
+  end
+
   def auto_hide_popups=(value)
-    others[:auto_hide_popups] = to_boolean(value)
+    settings[:auto_hide_popups] = to_boolean(value)
   end
 
   def auto_hide_popups?
-    others.fetch(:auto_hide_popups) { Setting.default_auto_hide_popups? }
+    settings.fetch(:auto_hide_popups) { Setting.default_auto_hide_popups? }
   end
 
   def warn_on_leaving_unsaved?
     # Need to cast here as previous values were '0' / '1'
-    to_boolean(others.fetch(:warn_on_leaving_unsaved) { true })
+    to_boolean(settings.fetch(:warn_on_leaving_unsaved, true))
   end
 
   def warn_on_leaving_unsaved=(value)
-    others[:warn_on_leaving_unsaved] = to_boolean(value)
+    settings[:warn_on_leaving_unsaved] = to_boolean(value)
   end
 
   # Provide an alias to form builders
@@ -82,11 +111,11 @@ def warn_on_leaving_unsaved=(value)
   alias :auto_hide_popups :auto_hide_popups?
 
   def comments_in_reverse_order=(value)
-    others[:comments_sorting] = to_boolean(value) ? 'desc' : 'asc'
+    settings[:comments_sorting] = to_boolean(value) ? 'desc' : 'asc'
   end
 
   def time_zone
-    self[:time_zone].presence || Setting.user_default_timezone.presence
+    super.presence || Setting.user_default_timezone.presence
   end
 
   def canonical_time_zone
@@ -98,17 +127,12 @@ def canonical_time_zone
 
   private
 
-  def attribute?(name)
-    attr = name.to_sym
-    has_attribute?(attr) || attr == :user || attr == :user_id
-  end
-
   def to_boolean(value)
     ActiveRecord::Type::Boolean.new.cast(value)
   end
 
-  def init_other_preferences
-    self.others ||= {}
+  def attribute?(name)
+    %i[user user_id].include?(name.to_sym)
   end
 
   def time_zone_correctness

db/migrate/20210615150558_aggregate_journals.rb

@@ -1,6 +1,9 @@
 require_relative './20200924085508_cleanup_orphaned_journal_data'
+require_relative './migration_utils/utils'
 
 class AggregateJournals < ActiveRecord::Migration[6.1]
+  include ::Migration::Utils
+
   def up
     [Attachment,
      Changeset,
@@ -25,7 +28,7 @@ def up
   # The change is irreversible (aggregated journals cannot be broken down) but down will not cause database inconsistencies.
 
   def aggregate_journals(klass)
-    klass.in_batches(of: ENV["OPENPROJECT_MIGRATION_AGGREGATE_JOURNALS_BATCH_SIZE"]&.to_i || 1000) do |instances|
+    in_configurable_batches(klass) do |instances|
       # Instantiating is faster than calculating the aggregated journals multiple times.
       aggregated_journals = aggregated_journals_of(klass, instances).to_a
 

db/migrate/20210825183540_make_user_preferences_json.rb

@@ -0,0 +1,55 @@
+require_relative './migration_utils/utils'
+
+class MakeUserPreferencesJson < ActiveRecord::Migration[6.1]
+  include ::Migration::Utils
+
+  class UserPreferenceWithOthers < ::UserPreference
+    self.table_name = 'user_preferences'
+    serialize :others, Hash
+    serialize :settings, ::Serializers::IndifferentHashSerializer
+  end
+
+  def up
+    add_column :user_preferences, :settings, :jsonb, default: {}
+
+    UserPreferenceWithOthers.reset_column_information
+    in_configurable_batches(UserPreferenceWithOthers).each_record do |pref|
+      migrate_yaml_to_json(pref)
+      pref.save!(validate: false)
+    end
+
+    change_table :user_preferences, bulk: true do |t|
+      t.remove :others, :hide_mail, :time_zone
+    end
+  end
+
+  def down
+    change_table :user_preferences, bulk: true do |t|
+      t.text :others
+      t.boolean :hide_mail, default: true
+      t.text :time_zone
+    end
+
+    UserPreferenceWithOthers.reset_column_information
+    in_configurable_batches(UserPreferenceWithOthers).each_record do |pref|
+      migrate_json_to_yaml(pref)
+      pref.save!(validate: false)
+    end
+
+    remove_column :user_preferences, :settings, :jsonb
+  end
+
+  private
+
+  def migrate_yaml_to_json(pref)
+    pref.settings = pref.others
+    pref.settings[:hide_mail] = pref.hide_mail
+    pref.settings[:time_zone] = pref.time_zone
+  end
+
+  def migrate_json_to_yaml(pref)
+    pref.others = pref.settings
+    pref.hide_mail = pref.settings[:hide_mail]
+    pref.time_zone = pref.settings[:time_zone]
+  end
+end

db/migrate/migration_utils/utils.rb

@@ -38,6 +38,12 @@ def say_with_time_silently(message, &block)
       end
     end
 
+    def in_configurable_batches(klass, default_batch_size: 1000)
+      batches = ENV["OPENPROJECT_MIGRATION_BATCH_SIZE"]&.to_i || default_batch_size
+
+      klass.in_batches(of: batches)
+    end
+
     def remove_index_if_exists(table_name, index_name)
       if index_name_exists? table_name, index_name
         remove_index table_name, name: index_name

spec/models/user_preference_spec.rb

@@ -99,6 +99,27 @@
                     :auto_hide_popups?
   end
 
+  describe 'hide_mail' do
+    it_behaves_like 'accepts real and false booleans',
+                    :hide_mail=,
+                    :hide_mail?
+
+    context 'when a new pref instance' do
+      subject { described_class.new }
+
+      it 'defaults to true' do
+        expect(subject.settings[:hide_mail]).to be_nil
+        expect(subject.hide_mail).to eq true
+        expect(subject.hide_mail?).to eq true
+
+        subject.hide_mail = false
+        expect(subject.settings[:hide_mail]).to eq false
+        expect(subject.hide_mail).to eq false
+        expect(subject.hide_mail?).to eq false
+      end
+    end
+  end
+
   describe 'time_zone' do
     it 'allows to save short time zones' do
       subject.time_zone = 'Berlin'