views:

273

answers:

3

Because RoR does not offer a validate_on_destroy, I am essentially implementing one by using the before_destroy callback.

Using before_destory works and prevents a project that has had effort_logged? from being deleted. The below implementation does not work because when no has been logged I want to delete the project and all of its dependents. As long as before_destroy is implemented as it is below I am unable to do so.

If I understand how :dependent => :destroy works in relation to before_destroy the dependent children are deleted before the parent's before_destroy method is called. If my assumption is correct is accessing the children in the effort_logged? method somehow causing them to not be deleted? Is there a better means to check to see if a parent can be deleted based on its children?

Aside from curiosity on how RoR works my goal is to pass the following two tests:

  • when no effort logged project deletion deletes dependents (this test fails)
  • cannot delete project with effort logged (this test passes)

Given everything outlined below I would expect both of these tests to pass.

Project Model

class Project < ActiveRecord::Base
  has_many :project_phases, :dependent => :destroy

  def before_destroy
     if effort_logged?
        errors.add_to_base("A project with effort logged cannot be deleted")
        false
     else
        true
     end
  end

  def effort_logged?
     project_phases.each do |project_phase|
        project_phase.deliverables.each do |deliverable|
           if (deliverable.effort_logged?)
              return true
           end
        end
     end
  end
end

Project Phase Model

class ProjectPhase < ActiveRecord::Base
  belongs_to :project
  has_many :deliverables, :dependent => :destroy
end

Deliverable Model

class Deliverable < ActiveRecord::Base
  has_many :effort_logs, :dependent => :destroy

  def effort_logged?
    total_effort_logged != 0
  end

  def total_effort_logged
    effort_logs.to_a.sum {|log| log.duration}
  end
end

Effort Log Model

class EffortLog < ActiveRecord::Base
  belongs_to :deliverable
end

Test cannot delete project with effort logged

test "cannot delete project with effort logged" do
   project = projects(:ProjectOne)

   assert !project.destroy, "#{project.errors.full_messages.to_sentence}"
end

Test when no effort logged project deletion deletes dependents

test "when no effort logged project deletion deletes dependents" do
   project = projects(:ProjectNoEffort)

   # all phases of the project
   project_phases = project.project_phases

   # all deliverables of all phases of the project
   project_phases_deliverables = {}

   # all effort logs of all deliverables of the project
   deliverables_effort_logs = {}

   project_phases.each do |project_phase|
      project_phases_deliverables[project_phase.name + "-" + project_phase.id.to_s] =
         project_phase.deliverables
   end

   project_phases_deliverables.each { |project_phase, project_phase_deliverables|
      project_phase_deliverables.each do |deliverable|
         deliverables_effort_logs[deliverable.name + "-" + deliverable.id.to_s] =
            deliverable.effort_logs
      end
   }

   project.destroy

   assert_equal(0, project_phases.count,
                "Project phases still exist for the deleted project")

   project_phases_deliverables.each { |project_phase, project_phases_deliverables|
      assert_equal(0, project_phases_deliverables.count,
      "Deliverables still exist for the project phase \"" + project_phase + "\"")
   }

   deliverables_effort_logs.each { |deliverable, deliverables_effort_logs|
      assert_equal(0, deliverables_effort_logs.count,
      "Effort logs still exist for the deliverable \"" + deliverable + "\"")
   }
end
A: 

You are correct, the children are wiped out before you get to before_destroy. Its not elegant but could you do something like this? : (btw, sorry i haven't tested this. it is more a thought than anything else).

in EffortLog, have a before_destroy :ready_to_die?

ready_to_die? would check if it has a zero effort value. If yes, destroy itself. If No, raise an exception (my example is EffortLogError). note: if you wanted to manually destroy something, you would need to zero it out yourself first.

then on your Project have a method with a more descriptive name:

def carefully_destroy
  Begin
    Project.transaction do
       self.destroy
    end 
  rescue EffortLogError
     self.errors.add_to_base("A Project with effort can't be deleted")
     #do some sort of redirect to the right spot.
  end
end
cgr
ActiveRecord already wraps the CRUD methods with transactions, there is no need to add a transaction here as well.
Elad Meidar
Good call about save and destroy being wrapped already. I missed that.
cgr
So with this implementation I would call carefully_destory instead of just calling destroy?
ahsteele
Right. Just mind the useless transaction I put in there, that Elad pointed out.
cgr
A: 

try adding raise ActiveRecord::Rollback right after you add the errors to base on the before_destroy filter.

Elad Meidar
Not deleting the child records works. My problem exists when trying to delete a record and all of its child records when I am able to do so.
ahsteele
A: 

After debugging the tests and keeping a close eye on the values of my methods and variables I was able to determine that the effort_logged? method was having issues. When there was logged effort it would return true. However, when there was no logged effort it would return the array of the project_phases. I modified effort_logged? to utilize a retval and that fixed the issue. The below method could stand for a refactoring.

  def effort_logged?
    retval = false

    project_phases.each do |project_phase|
      project_phase.deliverables.each do |deliverable|
         if (deliverable.effort_logged?)
            retval = true
         end
      end
    end

    return retval
  end
ahsteele