diff --git a/app/controllers/quantities_controller.rb b/app/controllers/quantities_controller.rb index 47bce3b..e76b8a6 100644 --- a/app/controllers/quantities_controller.rb +++ b/app/controllers/quantities_controller.rb @@ -19,7 +19,7 @@ class QuantitiesController < ApplicationController def create @quantity = current_user.quantities.new(quantity_params) if @quantity.save - @before = @quantity.successive + @before = @quantity.succeeding @ancestors = @quantity.ancestors flash.now.notice = t('.success', quantity: @quantity) else @@ -45,8 +45,9 @@ class QuantitiesController < ApplicationController if @quantity.update(permitted) @ancestors = @quantity.ancestors - @self_and_progenies = @quantity.with_progenies - @before = @self_and_progenies.last.successive + # TODO: simplify into 1 query (see #succeeding comment) + @quantities = @quantity.with_progenies + @before = @quantities.last.succeeding else # Until UI blocks all disallowed reparents, render error messages if present. flash.now.alert = @quantity.errors.full_messages diff --git a/app/controllers/units_controller.rb b/app/controllers/units_controller.rb index 97f116f..ecd2ab2 100644 --- a/app/controllers/units_controller.rb +++ b/app/controllers/units_controller.rb @@ -18,7 +18,7 @@ class UnitsController < ApplicationController def create @unit = current_user.units.new(params.expect(Unit::ATTRIBUTES)) if @unit.save - @before = @unit.successive + @before = @unit.succeeding flash.now.notice = t('.success', unit: @unit) else render_errors @unit @@ -45,7 +45,7 @@ class UnitsController < ApplicationController @previous_base = @unit.base @unit.update!(unit_params) - @before = @unit.successive + @before = @unit.succeeding if @unit.multiplier_previously_changed? flash.now.notice = t(".multiplier_reset", unit: @unit) end diff --git a/app/models/quantity.rb b/app/models/quantity.rb index f1bcbef..56b4ced 100644 --- a/app/models/quantity.rb +++ b/app/models/quantity.rb @@ -18,6 +18,9 @@ class Quantity < ApplicationRecord end validates :name, presence: true, uniqueness: {scope: [:user_id, :parent_id]}, length: {maximum: type_for_attribute(:name).limit} + # FIXME: length validation for :text type column is probably wrong; reported + # limit is in bytes (at least for MySQL) and check is for #length (i.e. + # characters); this doesn't seem to apply to :string (implemented as varchar) validates :description, length: {maximum: type_for_attribute(:description).limit} # Update :depths of progenies after parent change @@ -62,26 +65,42 @@ class Quantity < ApplicationRecord scope :defaults, ->{ where(user: nil) } # Return: ordered [sub]hierarchy - scope :ordered, ->(root: nil, include_root: true) { - numbered = Arel::Table.new('numbered') + scope :ordered, ->(**kwargs) { + with_path(**kwargs).order(arel_table[:path]) + } - self.model.with(numbered: numbered(:parent_id, :name)).with_recursive(arel_table.name => [ - numbered.project( - numbered[Arel.star], - numbered.cast(numbered[:child_number], 'BINARY').as('path') - ).where(numbered[root && include_root ? :id : :parent_id].eq(root)), - numbered.project( - numbered[Arel.star], - arel_table[:path].concat(numbered[:child_number]) - ).join(arel_table).on(numbered[:parent_id].eq(arel_table[:id])) - ]).order(arel_table[:path]) + # TODO: sort recursive CTE using ORDER BY; it's supported by SQLite, so + # drop MySQL until it is available (?) + # Only ORDER BY allows proper i.e. not placing limits on user data and + # efficient with regard to paging (LIMIT/OFFSET) depth-first search. + # Sorting based on numbering is less elegant/efficient (MySQL: ROW_NUMBER cannot + # be used inside recursive CTE and needs separate SELECT, + # https://bugs.mysql.com/bug.php?id=96538). + # Sorting based on pathname requires I) padding to maximum column length or + # II) delimiter which a) cannot be used in sorted column and b) sorts before + # anything else (this adds complexity; NULL cannot be used when sorting). + # Sorting based on ARRAY is only available on PostgreSQL. + scope :with_path, ->(root: nil, include_root: true) { + with_number = Arel::Table.new('with_number') + + self.model.with(with_number: with_number(:parent_id, :name)) + .with_recursive(arel_table.name => [ + with_number.project( + with_number[Arel.star], + with_number.cast(with_number[:number], 'BINARY').as('path') + ).where(with_number[root && include_root ? :id : :parent_id].eq(root)), + with_number.project( + with_number[Arel.star], + arel_table[:path].concat(with_number[:number]) + ).join(arel_table).on(with_number[:parent_id].eq(arel_table[:id])) + ]) } # TODO: extract named functions to custom Arel extension # NOTE: once recursive queries allow use of window functions, this scope can # be merged with :ordered # https://gist.github.com/ProGM/c6df08da14708dcc28b5ca325df37ceb#extending-arel - scope :numbered, ->(parent_column, order_column) { + scope :with_number, ->(parent_column, order_column) { select( arel_table[Arel.star], Arel::Nodes::NamedFunction.new( @@ -94,7 +113,7 @@ class Quantity < ApplicationRecord ).from(arel_table), Arel::Nodes.build_quoted('0') ], - ).as('child_number') + ).as('number') ) } @@ -132,15 +151,17 @@ class Quantity < ApplicationRecord .order(arel_table[:depth].desc) } - # Return: successive record in order of appearance; used for partial view reload - def successive + # Return: succeeding record in order of appearance, used for partial view reload. + # TODO: add scope returning succeeding element for multiple records + def succeeding quantities = Quantity.arel_table - Quantity.with( - quantities: user.quantities.ordered.select( - quantities[Arel.star], - Arel::Nodes::NamedFunction.new('LAG', [quantities[:id]]).over.as('lag_id') - ) - ).where(quantities[:lag_id].eq(id)).first + Quantity.with(lagging_quantities: user.quantities.with_path.select( + quantities[Arel.star], + # For SQLite 'OVER () ORDER BY x' is not equivalent to 'OVER (ORDER BY x)'. + Arel::Nodes::NamedFunction.new('LAG', [quantities[:id]]) + .over(Arel::Nodes::Window.new.order(Quantity.ordered.order_values)).as('lag_id') + )).from(Arel::Table.new(:lagging_quantities).as(:quantities)) + .where(quantities[:lag_id].eq(id)).take end def with_progenies diff --git a/app/models/unit.rb b/app/models/unit.rb index bc82353..64e7836 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -3,6 +3,7 @@ class Unit < ApplicationRecord belongs_to :user, optional: true belongs_to :base, optional: true, class_name: "Unit" + # TODO: use #alias_attribute after Rails upgrade? alias_method :parent, :base has_many :subunits, class_name: "Unit", inverse_of: :base, dependent: :restrict_with_error @@ -111,13 +112,13 @@ class Unit < ApplicationRecord .update!(base: recipient_base, **params) end - def successive + def succeeding units = Unit.arel_table - Unit.with(units_with_lag: user.units.left_outer_joins(:base).select( + Unit.with(lagging_units: user.units.left_outer_joins(:base).select( units[Arel.star], Arel::Nodes::NamedFunction.new('LAG', [units[:id]]) .over(Arel::Nodes::Window.new.order(Unit.ordered.order_values)).as('lag_id') - )).from(Arel::Table.new(:units_with_lag).as(:units)) + )).from(Arel::Table.new(:lagging_units).as(:units)) .where(units[:lag_id].eq(id)).take end end diff --git a/app/views/quantities/reparent.turbo_stream.erb b/app/views/quantities/reparent.turbo_stream.erb index 05eea39..b3756be 100644 --- a/app/views/quantities/reparent.turbo_stream.erb +++ b/app/views/quantities/reparent.turbo_stream.erb @@ -1,7 +1,7 @@ <% @previous_ancestors.union(@ancestors).each do |ancestor| %> <%= turbo_stream.replace ancestor %> <% end %> -<% @self_and_progenies.each do |quantity| %> +<% @quantities.each do |quantity| %> <%= turbo_stream.remove quantity %> <%= turbo_stream.before @before || :endmark, quantity %> <% end %>