brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

Why does acts_as_list override rails validation on it's own field? #269

Closed Loremaster closed 3 months ago

Loremaster commented 7 years ago

Example:

class Category < ActiveRecord::Base
  acts_as_list

  belongs_to :account
  has_many :templates, dependent: :nullify

  validates :name, presence: true
  validates :position, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
end

It gives:

Category.new(name: "123", position: -1).valid?
 => true 

If I remove acts_as_list from the model then validation works properly:

c=Category.new(name: "123", position: -1); c.valid?
=> false 
c.errors
 => #<ActiveModel::Errors:0x007f83e98e0610 @base=#<Category id: nil, name: "123", created_at: nil, updated_at: nil, account_id: nil, position: -1>, @messages={:position=>["must be greater than or equal to 0"]}> 

I find such behaviour very confusing. Why does it work like that? Is there any way it can be fixed?

brendon commented 7 years ago

We don't do anything to the validations. However, we do do a lot when position is set via attributes. Perhaps in the process of setting the position we're making it fit your conditions? Have you tried calling Category.new(name: "123", position: -1).position before and after calling valid?.

brendon commented 7 years ago

@Loremaster, check if the master branch solves your problem. I think this is related to https://github.com/swanandp/acts_as_list/pull/272.

Please let me know the outcome.

brendon commented 7 years ago

@Loremaster, can you give me an update?

brendon commented 6 years ago

Closing because of inactivity. Feel free to request that I reopen it @Loremaster.

jeff-free commented 2 years ago

We don't do anything to the validations. However, we do do a lot when position is set via attributes. Perhaps in the process of setting the position we're making it fit your conditions? Have you tried calling Category.new(name: "123", position: -1).position before and after calling valid?.

@brendon I could confirm the issue as well. the validation passed via sending valid? directly with attribute assignment:

# subject:
class Section
  validates_numericality_of :position, greater_than_or_equal_to: 0
end

Section.new(position: -1).valid? #=> true

And I've tried with the context you provided, the validation certainly fails after sending position:

s = Section.new(position: -1)
s.position #=> -1
s.valid? #=> false

But if we create it directly, the validation works:

Section.create!(position: -1) #=> Validation failed: Position must be greater than or equal to 0
brendon commented 2 years ago

Hi @jeff-free, the only thing I can think of is to check what the internal instance variable is prior to calling .valid? (and without calling .position).

.instance_variable_get('@position')

The code reads like this:

      attr_reader :position_changed

      define_method :position_column do
        position_column
      end

      define_method :"#{position_column}=" do |position|
        self[position_column] = position
        @position_changed = true
      end

I honestly don't know why we're even bothering with these methods but it could be a legacy thing from before ActiveRecord::AttributeMethods::Dirty existed to keep track of if the position had changed.

If you feel up to it, I'd be happy to review a PR around removing this and just relying on the built in attribute change functionality since we don't support any Rails versions that don't have this as far as I'm aware :)

jeff-free commented 2 years ago

@brendon I'll check it up, thx for the reply!

brendon commented 2 years ago

Oh I know why we track whether positioned was assigned. It's for the scope change functionality. In the rare case where the scope is change and we want to specifically position the item in the new list at the same position integer as its position in the old scope the dirty tracking won't report that the position value has changed. This method tracks if the position is set as an attribute even if the value hasn't changed.

brendon commented 3 months ago

Closing due to inactivity.