Use fixture references instead of active record queries in test setups and assertions

[kasugaijin]

In multiple cases throughout the test files the setup variables and some assertions are using active record queries e.g., searching for a record by a name. This is a brittle approach, because if the fixture property changes, a query on this property may return nil and break the test. Therefore, the tests should make use of fixture references (e.g., dogs(:dog_two) instead of Dog.find_by(name: "rover") ) as much as makes sense. This allows fixture attributes to change without unintended side effects.

Acceptance criteria:

• where it makes sense, change active record queries to fixture references
• all tests should pass
• changing a fixture attribute should not break the tests

[Eduardo06sp]

I would like to be assigned to this!

[kasugaijin]

Done!

[Eduardo06sp]

Question: You have a test (integration/user_account_test) where you test the following behavior:

"Staff user can sign up with an unverified staff account belonging to organization id 1 and see success flash"

https://github.com/kasugaijin/baja-pet-rescue/blob/main/test/integration/user_account_test.rb#L36

Here is part of the test I'm referring to (that I linked):

    # set org id to 1 to match db default value. Cannot pass org_id as it's not permitted param.
    organization = Organization.find_by(name: 'for_staff_sign_up')
    organization.id = 1
    organization.save

You then proceed to create a new user.

I'm going to move on from trying to figure this out, but if I delete the lines:

     organization.id = 1
     organization.save

then the User is no longer saved.

My question is: at what point is that organization ID used in the user registration process?

I tried following along the flow in which users are registered. But I don't see the point at which a staff account is created and being assigned an organization ID (though I did notice it defaults to 1 in the db). I also noticed that even in the registrations controller, there is no create action (so I assume it uses the default Devise one).

I don't understand why it would prevent a user account from being saved, and the model validations don't point to anything. ------------------------------------- And small update: I expect to finish this later today. I am still combing through the files, but I only see a few lines that need a change.

[kasugaijin]

Thanks Eduardo. Yes, you found one of the nuanced pieces of logic (probably could be improve but that's for another day).

So, there is the Users table, and there is the staff_accounts table. Users accepts nested attributes for staff_accounts, so when signing up as a staff, it creates a User and a staff account. This app is built to accept multiple organizations with authorization keeping users only able to perform CRUD on their own organization's resources.

I set the database up such that the StaffAccount.organization_id is null: false, and default: 1. In the db, the Organization with ID == 1 is a placeholder/default organization that all unverified staff belong to when they sign up, until I verify them and change the StaffAccount.organization_id to the appropriate one.

So, to test the flow of a staff user signing up, given the default value for StaffAccount.organization_id is 1, the default organization in the test database also needs an id of 1. As Rails fixtures work, the Ids on records don't start at 1, they are some random 5, 6 or more digits long. Therefore, those steps to find the organization and set the ID to 1 are carried out so that when the staff signs up there is an Organization with id == 1.

I hope that makes sense! I would probably do it differently now, but it works.

[Eduardo06sp]

I really appreciate the explanation! So now that I understand it much better, I realized that we can still go ahead and refactor this test.

I'll update my PR to explain the change, but essentially: you can still explicitly assign IDs in fixtures. If you don't, you get those super random, long numbers (these are simply autogenerated IDs).

Please let me know if I misunderstood the concept or what you intended to do with that particular test. I am still learning, so thank you for your patience!

[kasugaijin]

That sounds like a great idea. I never even thought of setting the ID in the fixture. So go ahead and give that a try!

[Eduardo06sp]

Perfect! I did push the change just yesterday.

So my PR is fully up to date as of now!