Add documentation and improve dev ex for CE Workflow Templates - #6787
Add documentation and improve dev ex for CE Workflow Templates#6787martha wants to merge 6 commits into
Conversation
| identifier = 'mc_direct_referral' | ||
| template_name = 'Direct Referral' | ||
| CeWorkflows::Shared::CeBuilderUtils.delete_template_and_associated_data(identifier, data_source: @data_source) unless @unsafe_run_in_production | ||
| CeWorkflows::Shared::CeBuilderUtils.delete_template_and_associated_data(identifier, data_source: @data_source, delete_opportunities: false) unless @unsafe_run_in_production |
There was a problem hiding this comment.
Added this to test/prove the new behavior locally
| template.destroy! | ||
| end | ||
|
|
||
| def self.delete_form_definitions(form_definition_identifiers, data_source_id) |
There was a problem hiding this comment.
This method is only called in code by the Standard workflow builder rake task (the non-preferred pattern), but I was still finding it useful to run in dev when iterating on workflow template forms, so I've included the local updates that have been helpful.
|
|
||
| - In practice, devs used `FORCE_RECREATE` anyway because drafts can't be used for referrals. | ||
| - Idempotent `connect_if_not_connected`-style guards add complexity and cruft. | ||
| - Implies that we support changing a prod template *without* incrementing the version number, which we don't want to support. |
There was a problem hiding this comment.
@gigxz in this bullet I was trying to articulate the concern you expressed on our huddle about this pattern, but I'm not sure I quite captured it - let me know if I should rearticulate this, or there is another reason I should add to this list?
|
|
||
| ## Updates | ||
|
|
||
| Updates to Workflow Templates fall into two categories. See [https://github.com/open-path/Green-River/issues/8515](https://github.com/open-path/Green-River/issues/8515). |
There was a problem hiding this comment.
It's good that we're linking 8515 somewhere, but I'm not sure this is the right place. The issue doesn't explain the two categories. Maybe we can reference this on line 91?
| - We don't yet have an established pattern for handling this in production. | ||
| - The non-preferred draft-idempotent pattern was partly built to handle this, but due to the downsides listed above, we're retiring that pattern and will cross this bridge when we re-encounter it. |
There was a problem hiding this comment.
| - We don't yet have an established pattern for handling this in production. | |
| - The non-preferred draft-idempotent pattern was partly built to handle this, but due to the downsides listed above, we're retiring that pattern and will cross this bridge when we re-encounter it. | |
| - We don't yet have an established pattern for handling workflow changes in production. The outstanding ticket for it is https://github.com/open-path/Green-River/issues/8515 | |
| - In local development and staging environments, we use a destructive operation to delete all old referrals when we need to update a workflow template. This should never be done in production. |
| | ------------------------------------------- | --------------------------------------------------------------------- | --------------------------------------------------------------------------- | | ||
| | **Template lifecycle** | `delete_template_and_associated_data` → `create_template` (published) | `find_or_create_draft_template` → mutate in place → optional `PUBLISH=true` | | ||
| | **Strategy for changing existing template** | Wipes existing template and starts anew | Idempotent on hard-coded version while draft; errors once published | | ||
| | **Prod setup** | One-time via `UNSAFE_RUN_IN_PRODUCTION=true` (builder + rake guard) | `PUBLISH=true`; no destroy in prod | |
There was a problem hiding this comment.
This is not an env var that exists
The documented UNSAFE_RUN_IN_PRODUCTION=true env var does not exist anywhere in the codebase. Both AC and AZ rake tasks raise unconditionally in production (ce_define_workflows.rake:8, ce_define_az_workflows.rake:10), and unsafe_run_in_production: is only a Ruby keyword argument on WorkflowBuilder#initialize. Anyone following this doc for initial prod setup would hit the rake guard with no way past it.
| def build_my_workflow | ||
| identifier = 'my_workflow' | ||
| CeWorkflows::Shared::CeBuilderUtils.delete_template_and_associated_data( | ||
| identifier, data_source: @data_source, delete_opportunities: false |
There was a problem hiding this comment.
So this is suggesting that even when setting up a workflow for the first time in production, we should be passing delete_opportunities: false which could potentially lead to dangling references and opportunities without a workflow template? but it's okay because that's guaranteed not to happen as long as the template identifier name didn't change, because it immediately recreates it right after (and is run in a transaction as long as its invoked via rake task..)?
It might be helpful to add a comment somewhere documenting the reasoning behind the delete_opportunities flag.
Merging this PR
mainDescription
README_FOR_CE_WORKFLOW_BUILDERS.mddocumenting the two CE workflow builder patterns and trimming the now-redundant "Updates" sections from the PH/Standard per-client READMEs in favor of a single shared doc.delete_template_and_associated_dataanddelete_form_definitionsto include soft-deleted (with_deleted) instances/steps/referrals and to destroy associatedHmis::Form::FormProcessorrows before destroying steps/form definitions, so repeated local rebuilds don't leave orphaned records that block form definition deletion.delete_opportunities:option todelete_template_and_associated_data. Set it tofalsefor the AZ direct referral workflow to test/prove the new option. Rebuilding that template no longer wipes associated opportunities.dependent: :nullifyfromHmis::WorkflowDefinition::Template#unit_groups/#direct_referral_unit_groupsso destroying a template in dev no longer clears unit groups' template identifier references, easing local iteration.delete_template_and_associated_dataunless the newdelete_opportunitiesoption isfalse, in order to maintain the previous behavior except when otherwise specified. I didn't pursue that since I thought it would be more cruft than needed for a low-risk change.Type of change
Checklist before requesting review