Implicit behavior in WorkflowImporter leads to confusing errors in testing

[dmolesUC]

So I ran into some weird WorkflowImporter behavior in a unit test — the import was failing with a null constraint violation, trying to create a WorkflowAction for a Workflow that hadn't been saved yet.

When I looked at WorkflowImporter.find_or_create_from(), it seemed like it couldn't possibly work -- we initialize a new Workflow with find_or_initialize_by, but then generate_state_diagram! goes off to create a bunch of dependent objects.

def find_or_create_from(configuration:)
  workflow = Sipity::Workflow.find_or_initialize_by(name: configuration.fetch(:name), permission_template: permission_template)
  generate_state_diagram!(workflow: workflow, actions_configuration: configuration.fetch(:actions))


  find_or_create_workflow_permissions!(
    workflow: workflow, workflow_permissions_configuration: configuration.fetch(:workflow_permissions, [])
  )
  workflow.label = configuration.fetch(:label, nil)
  workflow.description = configuration.fetch(:description, nil)
  workflow.allows_access_grant = configuration.fetch(:allows_access_grant, nil)
  workflow.save!
  logger.info(%(Loaded Sipity::Workflow "#{workflow.name}" for #{permission_template.class} ID=#{permission_template.id}))
  workflow
end

As far as I can tell, what happens is that the WorkflowAction belongs_to :workflow gets set to autosave by some side effect — I still haven't figured out what — and due to other problem with the test, that side effect hadn't happened yet by the time it hit this code.

I'm wondering if for clarity it might be better for WorkflowImporter.find_or_create_from to use find_or_create_by!?

It looks like it used to be that way, up till this change: 59ca0f7

but the apparent intended effect of setting label, description, and allows_access_grant before doing anything else was changed in some subsequent refactoring: 1e80387

I don't really understand why things were rearranged in that last commit, but in any case there doesn't seem to be a good reason not to use find_or_create_by! if somewhere in generate_state_diagram a newly created Workflow is going to get autosaved by side effect anyway.

Thoughts?

[dmolesUC]

Discussed in 12/4 tech call — @dmolesUC to work on it as time permits