Fix transitive data-option-for updates in dynamic forms (#3242)

[bstepanovski]

Fixes #3242

• Enable bidirectional data-option-for when a dependent option opts in with data-option-for-inverse: true
• Skip registering reverse option-for unless that flag is set, so existing Batch Connect forms keep their current behavior
• Add a system test for classroom / classroom_size with inverse enabled on the large row

[Bubballoo3]

I am still looking into it to get a better plan, but I feel like there must be a way to get this behavior without adding as much new code.

From the principle of the issue,

select1:
  - options: 
    - option1: 'name1'
    - option2: 'name2'
select2:
  - options:
    - option1: ['name1', data-option-for-select1-option1: 'false', data-option-for-inverse: true]
    - option2: 'name2'

Should have identical behavior to

select1:
  - options: 
    - option1: ['name1', data-option-for-select2-option1: 'false']
    - option2: 'name2'
select2:
  - options:
    - option1: ['name1', data-option-for-select1-option1: 'false']
    - option2: 'name2'

Meaning that we should be able to handle this higher up in the logic by detecting the inverse flag and inverting the function call, something like

function addOptionForHandler(causeId, targetId) {
  sharedOptionForHandler(causeId, targetId, 'optionFor');
  if (inverseEnabled){
    sharedOptionForHandler(targetId, causeId, 'optionFor');
  }
};

I'm not sure on the details of the logic, but it just seems that if we could achieve the same effect with the currently available flags, then we should be able to simulate those flags without re-implementing so many pieces (like the cache and mapping and stuff)

[Bubballoo3]

You mentioned there were some tricky errors with cascading effects here, are those covered in your test cases? That is, does the current solution solve those mentioned issues, or have you found gaps in the tests?

[bstepanovski]

Right now the test only covers a simple two field inverse case (classroom and classroom_size). It doesn’t cover chained dependencies, since I added the inverse flag specifically to handle that behavior directly instead of extending the existing option-for logic. However, after going more in depth in testing when trying to get it to work without adding a flag mainly because I didn’t want existing forms to start behaving unexpectedly, it seems that the current approach may also fail. Chained dependencies can still get into weird states with the current solution and after a while it started feeling like fixing one case would break another and the code changes were getting out of hand for this kind of fix.

[Bubballoo3]

Chained dependencies can still get into weird states with the current solution and after a while it started feeling like fixing one case would break another and the code changes were getting out of hand for this kind of fix.

That makes a lot of sense. I think this is still something that would be nice to add, so if you can add a test case demonstrating where the current solution falls short that will hopefully make it easier to fix up.