Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Loremaster opened this issue Apr 10, 2017 · 9 comments
Closed

Comments

@Loremaster
Copy link

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?

@Loremaster Loremaster changed the title Why does acts_as_list overrides rails validation on it's own field? Why does acts_as_list override rails validation on it's own field? Apr 10, 2017
@brendon
Copy link
Owner

brendon commented Apr 10, 2017

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
Copy link
Owner

brendon commented May 10, 2017

@Loremaster, check if the master branch solves your problem. I think this is related to #272.

Please let me know the outcome.

@brendon
Copy link
Owner

brendon commented Aug 10, 2017

@Loremaster, can you give me an update?

@brendon
Copy link
Owner

brendon commented Oct 3, 2017

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

@brendon brendon closed this as completed Oct 3, 2017
@jeff-free
Copy link

jeff-free commented Apr 3, 2022

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
Copy link
Owner

brendon commented Apr 3, 2022

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 :)

@brendon brendon reopened this Apr 3, 2022
@jeff-free
Copy link

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

@brendon
Copy link
Owner

brendon commented Apr 5, 2022

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
Copy link
Owner

brendon commented Jun 4, 2024

Closing due to inactivity.

@brendon brendon closed this as completed Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants