feat(copy-campaign): copy van mappings

[ajohn25]

Description

This adds the copying of van mappings to the copy campaign mutation

Motivation and Context

Client request

How Has This Been Tested?

This has been tested locally

Screenshots (if appropriate):

Documentation Changes

Checklist:

• My change requires a change to the documentation.
• I have included updates for the documentation accordingly.

[claude]

PR Review: Copy VAN Mappings in Campaign Copy

Summary

This PR adds the ability to copy external system question response mappings when copying a campaign. The implementation adds a new SQL insert in the copyCampaign function.

Code Quality & Best Practices

✅ Positive Points

• Consistent with existing patterns: The implementation follows the same raw SQL pattern used for copying other campaign resources (canned responses, teams, variables)
• Transaction safety: The query runs within the existing transaction context (trx), ensuring atomicity
• Proper JOIN logic: Uses EXISTS subquery to validate that the answer option matches, which is good for data integrity

⚠️ Concerns & Issues

1. Incomplete Child Table Copying (Critical Issue)

The PR only copies the all_external_sync_question_response_configuration table but does not copy the child tables that reference it:

• external_sync_config_question_response_result_code
• external_sync_config_question_response_activist_code
• external_sync_config_question_response_response_option

Based on the schema (migration 20200915113230_van_sync_question_response_configuration.js:55-101), these child tables have ON DELETE CASCADE foreign keys to the parent table and contain the actual VAN result codes, activist codes, and survey response options that should sync.

Without copying these child tables, the VAN mappings will be incomplete. The copied configurations will exist but won't have any actual sync actions configured.

Recommendation: Add three more INSERT statements to copy the child tables, mapping the old question_response_config_id to the new IDs:

-- Copy Result Codes
insert into external_sync_config_question_response_result_code 
  (question_response_config_id, external_result_code_id)
select
  new_config.id,
  old_result.external_result_code_id
from external_sync_config_question_response_result_code old_result
join all_external_sync_question_response_configuration old_config 
  on old_config.id = old_result.question_response_config_id
join all_external_sync_question_response_configuration new_config
  on new_config.campaign_id = ?
  and new_config.system_id = old_config.system_id
  and new_config.question_response_value = old_config.question_response_value
  and new_config.interaction_step_id in (
    select new_step.id 
    from interaction_step new_step
    join interaction_step old_step
      on old_step.campaign_id = ?
      and new_step.campaign_id = ?
      and new_step.answer_option = old_step.answer_option
    where old_step.id = old_config.interaction_step_id
  )
where old_config.campaign_id = ?

-- Similar inserts needed for activist_code and response_option tables

2. Interaction Step ID Mapping Logic May Be Incorrect

The current query joins on parent_step.campaign_id = ? (the NEW campaign ID) and validates with an EXISTS check. However:

join interaction_step parent_step on parent_step.campaign_id = ? and exists (
  select 1 from interaction_step child_step 
  where child_step.parent_interaction_id = parent_step.id
  and answer_option = question_response_value
)

Issue: This assumes the interaction steps have already been copied to the new campaign (which they have at line 269-298), but the logic for mapping old step IDs to new step IDs is fragile. The EXISTS check validates that a child step with the matching answer exists, but doesn't actually use the old interaction_step_id to find the corresponding new one.

Better approach: Since interaction steps are copied earlier in the function (src/server/api/lib/campaign.ts:269-298), you should map old step IDs to new step IDs more explicitly, similar to how other campaign copy operations work. The current approach might work but could create incorrect mappings if multiple steps have the same answer_option value.

Recommendation: Use a more explicit mapping:

join interaction_step new_step on new_step.campaign_id = ?
join interaction_step old_step 
  on old_step.campaign_id = ?
  and old_step.id = ext.interaction_step_id
  and new_step.answer_option = old_step.answer_option
  and coalesce(new_step.parent_interaction_id, -1) = coalesce(old_step.parent_interaction_id, -1)

3. Missing Template Handling

Other copy operations check the template parameter to determine behavior (see line 242-246, 248-267, 335-348). VAN mappings might need similar logic:

• Should VAN mappings be copied when creating a template?
• Should they be copied when creating a campaign from a template?

The current implementation will copy them in both cases, which may or may not be desired.

Security Concerns

✅ No Major Security Issues

• SQL injection: Uses parameterized queries (? placeholders) ✓
• No user input directly in the SQL string ✓

⚠️ Organization Boundary Check

One consideration: The system_id is copied directly from the source campaign. There should be validation that the external system belongs to the target organization when copying across organizations (the targetOrgId parameter at line 332).

Current behavior: The code copies the system_id as-is, which could reference an external system from a different organization if copying cross-org.

Check needed: Does the all_external_sync_question_response_configuration table have constraints that prevent this? Looking at the migration (line 38 of 20200915113230_van_sync_question_response_configuration.js), it references external_system(id) but there's a trigger tg_campaign_check_exteral_system_id that validates organization matching for campaigns.

Recommendation: Add a WHERE clause to only copy mappings where the system belongs to the target organization:

where ext.campaign_id = ?
  and exists (
    select 1 from external_system es
    where es.id = ext.system_id
    and es.organization_id = (select organization_id from campaign where id = ?)
  )

Performance Considerations

⚠️ Query Performance

• The EXISTS subquery in the JOIN clause could be expensive for campaigns with many interaction steps
• Consider adding appropriate indexes if not already present (should check interaction_step(campaign_id, parent_interaction_id, answer_option))

Minor Impact

Since campaign copying is not a high-frequency operation, performance is likely acceptable, but the query could be optimized as suggested above.

Test Coverage

❌ Missing Tests

The PR description states "tested locally" but there are no new automated tests added. Given the complexity of the VAN integration and the critical issue identified above, this needs test coverage.

Required tests:

1. Test that VAN mappings are copied when copying a campaign
2. Test that child configuration tables (result codes, activist codes, response options) are also copied
3. Test that interaction step mappings are correctly maintained
4. Test cross-organization copy behavior (if supported)
5. Test template → campaign creation with VAN mappings

See existing test patterns in src/server/api/lib/campaign.spec.ts:410-444 for reference.

Overall Assessment

Status: ⚠️ Needs Revision - The implementation is incomplete

Critical Issues:

1. Child tables are not copied (missing result codes, activist codes, response options)
2. No automated test coverage

Recommendations:

1. Add INSERT statements for the three child tables
2. Review and potentially improve the interaction step ID mapping logic
3. Add test coverage for this functionality
4. Consider organization boundary validation
5. Consider whether template campaigns should include VAN mappings

The approach is sound and follows existing patterns, but the implementation needs to be completed to fully copy the VAN configuration.

[ajohn25]

@claude 2 questions from your review:

1. Can you explain the child table relationships in more detail?
2. The records that system_id links to are org specific correct?

[claude]

Claude finished @ajohn25's task —— View job

---

Answering Your Questions

Great questions! Let me investigate the database schema and relationships to give you detailed answers.

Todo List

• Read VAN sync migration to understand table relationships
• Read external_system schema to verify org-level scoping
• Explain child table relationships in detail
• Verify and explain system_id organization specificity

---

Answer 1: Child Table Relationships Explained

The VAN sync question response configuration uses a parent-child table pattern to represent the actions that should be taken when a texter selects a specific answer in a campaign. Here's how the tables relate:

Parent Table: all_external_sync_question_response_configuration

This is the main configuration table that links:

• A specific campaign (campaign_id)
• A specific interaction step/question (interaction_step_id)
• A specific answer value (question_response_value)
• An external system like VAN (system_id)

Reference: migrations/20200915113230_van_sync_question_response_configuration.js:36-47

Child Tables (The Actual Sync Actions)

When a texter selects the configured answer, the system can perform one or more of these VAN sync actions:

1. external_sync_config_question_response_result_code (lines 55-63)

   • Links to VAN Result Codes (e.g., "Canvassed", "Not Home", "Refused")
   • Multiple result codes can be applied for a single answer
   • Foreign key: question_response_config_id → parent table's id
   • Has ON DELETE CASCADE - if parent is deleted, these are automatically deleted

2. external_sync_config_question_response_activist_code (lines 71-79)

   • Links to VAN Activist Codes (e.g., "Volunteer", "Supporter")
   • Multiple activist codes can be applied for a single answer
   • Foreign key: question_response_config_id → parent table's id
   • Has ON DELETE CASCADE

3. external_sync_config_question_response_response_option (lines 87-95)

   • Links to VAN Survey Question Response Options
   • Syncs the answer to a VAN survey question
   • Foreign key: question_response_config_id → parent table's id
   • Has ON DELETE CASCADE

Why This Matters for Your PR

Your current implementation only copies the parent table, which creates the configuration shell but doesn't copy the actual sync actions. It's like copying a recipe title without the ingredients and instructions.

Example scenario:

• Original campaign: When texter selects "Yes" → Apply VAN Result Code "Canvassed" + Activist Code "Supporter"
• After your current copy: When texter selects "Yes" → (nothing happens - no actions configured)

What needs to be added:
Three additional INSERT statements to copy each child table, mapping the old question_response_config_id to the newly created configuration IDs. The challenge is mapping old parent IDs to new parent IDs - this requires joining through the interaction steps to find the corresponding new configuration records.

---

Answer 2: Yes, system_id is Organization-Specific

Confirmed: external_system records ARE organization-specific.

Evidence from the Schema

1. The external_system table has an organization_id column (migrations/20200510204235_external_list.js:10)

2. Database trigger enforces organization boundary (migrations/20200915113230_van_sync_question_response_configuration.js:8-30):

   create function tg_campaign_check_exteral_system_id()
     returns trigger as $$
   declare
     v_system_organization_id integer;
   begin
     select organization_id
     from public.external_system
     where id = NEW.external_system_id
     into v_system_organization_id;
   
     if NEW.organization_id <> v_system_organization_id
     then
       raise exception 'External system referenced by [external_system_id:%] must belong to the same organization!',
         NEW.external_system_id;
     end if;
   
     return NEW;
   end;
   $$ language plpgsql;

This trigger fires on campaign table inserts/updates and enforces that campaigns can only reference external systems from their own organization.

Implications for Campaign Copying

Within the same organization: Your current code is safe - the system_id will always belong to the organization since it's copied from a campaign in the same org.

Cross-organization copying (if the targetOrgId parameter allows this): The copied system_id might reference an external system from a different organization. However:

• The campaign trigger would prevent the new campaign from being created with a mismatched external_system_id
• But your VAN mapping copy would try to insert records with a system_id that doesn't belong to the target org

Recommendation: Add a WHERE clause to your query to filter out mappings for external systems that don't belong to the target organization:

where ext.campaign_id = ?
  and exists (
    select 1 from external_system es
    where es.id = ext.system_id
    and es.organization_id = (select organization_id from campaign where id = ?)
  )

This ensures that only VAN mappings for external systems that belong to the target campaign's organization are copied.

---

Summary

1. Child tables contain the actual VAN sync actions - your PR needs to copy these too, not just the parent configuration table
2. External systems are org-specific and enforced by database triggers - but you should add defensive filtering when copying to handle cross-org scenarios safely

Let me know if you'd like me to help implement the complete solution with the child table copying!

---

[claude]

Code Review: Copy VAN Mappings Feature

I've reviewed PR #87 which adds the ability to copy external system (VAN) question response mappings when copying campaigns. Here's my analysis:

✅ Positives

1. Correct Placement: The new code is logically placed after copying campaign variables and before the return statement, consistent with the other copy operations.

2. Transaction Safety: The operation correctly uses the existing transaction context (trx), ensuring atomicity with other copy operations.

3. Proper Table Usage: Uses all_external_sync_question_response_configuration which is the base table, not a view.

⚠️ Critical Issues

1. Incomplete Copy - Missing Child Tables

The code only copies the parent configuration table but does not copy the associated child tables. According to the schema (migration 20200915113230_van_sync_question_response_configuration.js), there are three child tables with cascade delete relationships:

• external_sync_config_question_response_result_code - Maps to result codes
• external_sync_config_question_response_activist_code - Maps to activist codes
• external_sync_config_question_response_response_option - Maps to survey response options

Impact: The copied campaign will have VAN mappings but they will be incomplete/empty, missing the actual result codes, activist codes, and response options that make the mappings functional.

Fix Required: Add three additional INSERT statements to copy these child tables, referencing the newly generated UUIDs from the parent table copy.

2. Potential Logic Error in JOIN Condition

The query uses this JOIN logic:

join interaction_step parent_step on parent_step.campaign_id = ? and exists (
  select 1 from interaction_step child_step 
  where child_step.parent_interaction_id = parent_step.id
  and answer_option = question_response_value
)

Issue: This assumes that question_response_value always matches an answer_option on a child interaction step. However, looking at the schema view missing_external_sync_question_response_configuration (lines 108-129 in the migration), there are TWO sources of valid question responses:

1. Answer options from child interaction steps (what you're checking)
2. Question responses (question_response table) - which may exist independently

Potential Impact: If the source campaign has VAN mappings for question responses that don't have corresponding child interaction steps (i.e., they're from the question_response table), those mappings won't be copied.

Recommendation: Review whether this filtering is intentional or if all VAN mappings should be copied regardless of interaction step structure.

3. Missing UUID Regeneration

The all_external_sync_question_response_configuration table uses UUIDs as primary keys (id uuid not null default uuid_generate_v1mc()). Your INSERT doesn't include the id column, which means PostgreSQL will auto-generate new UUIDs (correct behavior).

However, when you copy the child tables (which you need to add), you'll need to map old UUIDs to new ones. Consider using a CTE to capture the old→new ID mapping:

with new_configs as (
  insert into all_external_sync_question_response_configuration (...)
  select ...
  returning id as new_id, question_response_value, interaction_step_id, system_id
)
-- Then use this to copy child tables

📋 Additional Considerations

4. Missing Tests

There are no tests for the copyCampaign function at all (searched for *.test.ts files). This is a complex function that would greatly benefit from integration tests, especially for new functionality.

Recommendation: Add tests that verify VAN mappings are correctly copied, including the child tables.

5. Error Handling

The query will silently skip mappings that don't match the JOIN conditions. Consider whether this is the desired behavior or if there should be logging/warnings when mappings can't be copied.

6. Performance

The nested EXISTS subquery could be expensive for campaigns with many interaction steps. Consider if an INNER JOIN would be more performant:

join interaction_step parent_step on parent_step.campaign_id = ?
join interaction_step child_step 
  on child_step.parent_interaction_id = parent_step.id
  and child_step.answer_option = ext.question_response_value

Though this would require DISTINCT to avoid duplicates if multiple child steps have the same answer_option.

🔒 Security

No security concerns identified. The operation uses parameterized queries and respects the existing campaign ID constraints.

📝 Code Style

Follows the existing code conventions in the file. SQL formatting is consistent with other copy operations.

Recommendation

Do not merge until:

1. ✅ Child table copying is implemented (result codes, activist codes, response options)
2. ✅ Clarify and validate the JOIN logic for question response matching
3. ✅ Add integration tests
4. ⚠️ Consider performance optimization (optional)

The feature is a good addition but incomplete in its current form. The VAN mappings won't function correctly without the associated child table data.

---

Review generated by Claude Code