Fix repeat deletion Issue by implementing Clear volatiles for tree element

[shubham1g5]

Technical Summary

https://dimagi.atlassian.net/browse/USH-4708 (Deleting a repeat was causing errors in resolving references for remaining repeat nodes)

After deleting a repeat, a node's reference can change due to change in repeat index of the node. Therefore we need to expire the existing cached references for TreeElements.

Safety Assurance

Safety story

• Should only affect repeat deletion which is covered by test

Automated test coverage

Added in the PR plus in dimagi/formplayer#1597

QA Plan

None

Special deploy instructions

• This PR can be deployed after merge with no further considerations.

Rollback instructions

• This PR can be reverted after deploy with no further considerations.

Review

• The set of people pinged as reviewers is appropriate for the level of risk of the change.

Duplicate PR

Automatically duplicate this PR as defined in contributing.md.

[ctsims]

I think we should be moving the logic for dispatching the volatile clearing from the DataInstance into each implementation of the method, where each implementation is required to both clear its own volatile state and dispatch the clear call to its children and other methods.

There are two big reasons why.
The biggest reason is that not all implementations of AbstractTreeElement actually have real child nodes. getChild() sometimes returns real elements, but also sometimes returns templates or virtual elements (say, for Repeat Indexing), so the "Get child count" and "dispatch to each child by multiplicity" approach could either end up triggering unintended consequences (walking the repeat tree and creating repeats, for instance), could return confusing elements, could miss data (say, nodes with a multiplicity of -2), or could just end up being slower than necessary
Additionally, right now there's just something a little awkward about requiring a DataInstance root for dispatching the tree walk in the first place.

I checked, and I don't think this will actually create meaningfully more repetitive implementations. The only thing this change makes a little confusing is that I think the default implementation of clearCache should definitely call getBase().clearVolatiles(), so we either need a second implementation of cleanCache that gives it a different target, or we should just take the hit and clear all of the caches

[ctsims]

Looks good to me after the revisions! Thanks for getting this implemented across the models.

[shubham1g5]

duplicate this branch 00c3d8a 9302ea1

[snopoke]

Just to clarify, this cleanCache function is already being called in the correct way and we're just adding more cleanup code. Is that correct?

[shubham1g5]

yes, for this particular use case of deleting repeat it gets called here

[shubham1g5]

duplicate this PR 00c3d8a 9302ea1

[shubham1g5]

duplicate this PR 00c3d8a 9302ea1