Skip to content

update saves associations even when validation fails #92

Closed
clyfe opened this Issue Feb 9, 2011 · 9 comments

2 participants

@clyfe
clyfe commented Feb 9, 2011

This is a strange and nasty one.

User habtm :roles
validates :there_is at least_a_user_with_admin_role
  1. uncheck admin role, submit update form
  2. validation fails, because there is no other user with admin role

= the form is re rendered with the red error messages

BUT the admin association was deleted

This happens because the update_record_from_params method in AS/lib/active_scaffold/attribute_params.rb +62 assigns to the relation.

parent_record.send("#{column.name}=", value) unless parent_record.send(column.name) == value

Assigning to the relation fires the SQL to DB instantly, and what we want is something like .build that only connects the models, doing the saving later, in the "save" call. I heard this behavior is as such for quite some time.

parent_record.send("#{column.name}").build(value.attributes) unless parent_record.send(column.name) == value

Actually, I'm not sure that .build is useful, as according to the API it "Returns a new object [...]"
Maybe have the do_update in a transaction ?
This might affect other parts like create.

[Open for pondering ...]

@clyfe
clyfe commented Feb 9, 2011

I think the following fixes it.
( I moved update_record_from_params inside the transaction and I raise ActiveRecord::Rollback unless successful? )

def do_update
  do_edit
  update_save
end

def update_save
  begin
    active_scaffold_config.model.transaction do
      @record = update_record_from_params(@record, active_scaffold_config.update.columns, params[:record])
      before_update_save(@record)
      self.successful = [@record.valid?, @record.associated_valid?].all? {|v| v == true} # this syntax avoids a short-circuit
      if successful?
        @record.save! and @record.save_associated!
        after_update_save(@record)
      else
        raise ActiveRecord::Rollback, "don't save associations unless record is valid"
      end
    end
  rescue ActiveRecord::RecordInvalid
  rescue ActiveRecord::StaleObjectError
    @record.errors.add(:base, as_(:version_inconsistency))
    self.successful=false
  rescue ActiveRecord::RecordNotSaved
    @record.errors.add(:base, as_(:record_not_saved)) if @record.errors.empty?
    self.successful = false
  end
end
@vhochstein
Owner

That s indeed an ugly one... Do you think you can create a github repository with an example app?

thanks a lot in advance.

@vhochstein
Owner

Great thanks a lot.

@clyfe
clyfe commented Feb 22, 2011

Any official plans on this issue ?
The proposed solution is pretty straightforward and the issue effects are pretty nasty!
Should I submit a pull request ?

@vhochstein
Owner

Nope, unfortuently not so far, lack of time... :-(

If your patch works for you you can monkey path, however in general it will not work, for example it will break batch_create and batch_update
In addition I m not sure what will be shown in case of an validation error. Will user see the same data he has entered in the error form?

@clyfe
clyfe commented Feb 23, 2011

Here's the monkeypatch (rails initializer) if anyone else is interested
https://gist.github.com/840267

@vhochstein
Owner

Finally I found some time to take a look at it... I did nt found a better solution so far. Just changed it a little bit to still support batch actions.

Thanks a lot. Hopefully we do not see any negative side effects

@clyfe
clyfe commented Mar 3, 2011

Great

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.