Models and Tests - Lab 4 - Solution

The problem

From labs/4-regression.rb in your copy of models-and-tests:

Someone logging activities in a hurry keeps leaving the salesperson blank, and the save fails. A developer decides to help: when the salesperson is missing, use the first salesperson in the table.

Code-along 4 did this to Company with a filled-in name, and a test caught it. This lab does it to Activity, and the question is whether yours will.

1 and 2. Before you paste anything

bin/rails test

Everything passes. Now the prediction, on paper: is this a bug?

Yes. The rule in DONE.md is "An activity can't be saved without a salesperson", and the change saves exactly such an activity by inventing a salesperson for it. A note logged by nobody now belongs to whoever happens to be first in the table, and that person gets the credit, or the blame. So you expect the first test under Activity to fail.

Write that down before step 3. The lab is about comparing the prediction with what happens.

3 and 4. The helpful change

Pasted into app/models/activity.rb, under the class line:

class Activity < ApplicationRecord
  before_validation :fill_in_missing_salesperson

  def fill_in_missing_salesperson
    if self["salesperson_id"].blank?
      first_salesperson = Salesperson.all[0]
      if first_salesperson != nil
        self["salesperson_id"] = first_salesperson["id"]
      end
    end
  end

  validates :salesperson_id, presence: true
  validates :contact_id, presence: true
end

before_validation runs the method every time an activity is about to be checked and saved, so by the time validates :salesperson_id, presence: true looks, the blank has been filled. The rule is still in the file, and it's now unreachable.

bin/rails test

9 runs, 9 assertions, 0 failures, 0 errors, 0 skips

Nothing failed. The prediction was wrong, and the bug is in. To prove it's in, a script that does what the hurried person did, run with Ben and Brian in the table from lab 2:

activity = Activity.new
activity["contact_id"] = 1
activity["note"] = "left a voicemail"

if activity.save
  puts "Saved, with salesperson_id #{activity["salesperson_id"]}"
  activity.destroy
else
  puts "Not saved: #{activity.errors.full_messages.join(", ")}"
end

Saved, with salesperson_id 1

The voicemail is Ben's now (whatever his id is in your table).

5. Why the test didn't catch it

Read the test again:

test "an activity can't be saved without a salesperson" do
  activity = Activity.new
  activity["contact_id"] = 1
  activity["note"] = "met at Cupertino"
  activity.save

  assert_equal 0, Activity.all.count
end

The test database is empty when each test starts. There are no salespeople in it, so Salesperson.all[0] is nil, the if first_salesperson != nil does nothing, the salesperson_id stays blank, and the validation refuses the save. The test passes for a reason that has nothing to do with your rule: there was nobody to fill in.

That's the lab's hint, and it's a general lesson about tests. A test proves the rule only in the situation it sets up. "Can't be saved without a salesperson" was proven for an empty table. The change only misbehaves when the table has someone in it, and no test put anyone there.

So the sentence to add under Activity in DONE.md names that situation:

An activity can't be saved without a salesperson, even when there are salespeople.

Ask your agent for its test, with the change still pasted in. It should fail. Something like:

test "an activity can't be saved without a salesperson, even when there are salespeople" do
  ben = Salesperson.new
  ben["first_name"] = "Ben"
  ben["email"] = "ben@example.com"
  ben.save

  activity = Activity.new
  activity["contact_id"] = 1
  activity["note"] = "met at Cupertino"
  activity.save

  assert_equal 0, Activity.all.count
end

Failure:
ActivityTest#test_an_activity_can't_be_saved_without_a_salesperson,_even_when_there_are_salespeople
Expected: 0
  Actual: 1

4 runs, 4 assertions, 1 failures, 0 errors, 0 skips

Now it's caught. If the new test passed with the change still in, it isn't testing the rule yet: check that it saves a salesperson first.

6. Take the change out

Remove the before_validation line and the method, save, and run the tests:

10 runs, 10 assertions, 0 failures, 0 errors, 0 skips

Keep the new test. It's the one that would have caught this, and the next helpful change is usually made by an agent, in a hurry, with the best intentions.

Reflection

A test suite is only as good as the rules behind it, and a rule is only as good as the situations its test sets up. Your first test said "without a salesperson" and meant "with no salespeople anywhere", because that's the only case it built. Nobody wrote the other case down, so nothing protected it. When a change gets through, the fix is rarely in the model first. It's a sentence in DONE.md.