Consistently Persist PD FactoryBot Records in v5

[Hamms]

In preparation for an update to FactoryBot 5, which fixes some unintuitive behavior when building objects with descendants. Specifically, before, when creating records with build, any associated records created as a result would unexpectedly be persisted with create rather than just initialized with build.

In fact, for the School model specifically, that behavior is required, in a couple of ways: in the :pd_teacher_application factory, we pass the id of the School object upstream to the :pd_teacher_application factory which as part of its own persistence logic expects its school_id attribute to reference a persisted object. And in the School model itself, because it doesn't use a standard Rails auto-incrementing integer id, we manually assign an id based on the latest persisted value; that means that we always want to persist any built School record before building another one, or we'll get an ActiveRecord::RecordNotUnique error.

Starting in FactoryBot 5, the default association strategy will now be inherited from the parent factory, so building a hash will build but not persist the associated School object. In most cases this is probably desirable, but here we are relying on the old strange behavior and so now need to explicitly specify that we always want to persist the associated School object.

In a couple other places, we were implicitly (and I think accidentally) depending on this behavior to allow us to build and then validate records which require their associated records to be persisted in order to pass validation. In those cases, the fix was either to create rather than build the original record in the first place, or to explicitly create the associated records we want to be persisted.

Links

• https://github.com/thoughtbot/factory_bot/blob/main/GETTING_STARTED.md#build-strategies-1

Testing story

Without this change, we get the following error when trying to run tests on FactoryBot 5:

ERROR["test_deleting_user_deletes_dependent_pd_applications", "UserTest", 21.59177164407447]
 test_deleting_user_deletes_dependent_pd_applications#UserTest (21.59s)
Minitest::UnexpectedError:         ActiveRecord::RecordNotFound: Couldn't find School with 'id'=60001411747
            app/models/school_info.rb:98:in `sync_from_schools'
            lib/school_info_deduplicator.rb:20:in `get_duplicate_school_info'
            app/models/pd/application/teacher_application.rb:105:in `update_user_school_info!'
            test/models/user_test.rb:3156:in `block in <class:UserTest>'
            test/testing/setup_all_and_teardown_all.rb:36:in `run'

With this change, the test passes.

We also had two tests that were just validating internal FactoryBot persistence logic. One of them is no longer accurate because we actually now are persisting some of these dependencies consistently, and the other one is just a bit unnecessary. Remove them both.

Follow-up Work

Update FactoryBot to v5

[megcrenshaw]

Thanks for doing this, Elijah!

[Hamms]

Upon further investigation, I found one more place where we need to update our use of School factories for FactoryBot v5, and learned about the potential for several more. Un-marking this PR as ready for review until I'm confident I've tracked down all of 'em

[megcrenshaw]

Why are we using create on line 168 but build here?

[Hamms]

Because it this case the existing code cares about the school_info object being built but not created, whereas the other one doesn't have any such restrictions. And also because saving a Rails object will also save its associated records; using create here will also attempt to persist school_info

[megcrenshaw]

What do you mean by "it's too empty to pass validation"?

[Hamms]

This test is explicitly calling build :school_info_us rather than just using the default school_info factory because the default factory also creates associated school data and other stuff that's required for the model to pass validation, but this test explicitly wants to check the serializer behavior when school and teacher info is empty.

[megcrenshaw]

Ah yes, makes sense. Thank you!

[davidsbailey]

moving from build to create when possible makes sense to me!