Enforce taxonomy.allow_multiple across relations

[lukearndt]

Context

https://github.com/impactoss/impactoss-server/issues/379

From the linked issue:

instead of returning an error when a previous association exists, the server should simply remove the previous association and replace it with the new one

now there may be cases where the client is requesting the removal of an existing association at the same time as requesting the creation of new one, which -when implementing the above- may result in the request to remove a link that was already removed by the server. thus to prevent returning an error the request to delete a non-existing association should yield a positive response

Additional context from Timo via Slack:

we should make sure that only ONE category can be associated with other entities (i.e. recommendations, measures and users) when its taxonomy only allows ONE association (taxonomy.allow_multiple = false)

now when the client requests to delete an existing association and create a new one concurrently (as would be the case when the user changes the categorisation) AND when the server processes the CREATE request before the DELETE one it will fail as another association already exists.

to work around this I would suggest to

• also delete the existing association (if present) when processing the CREATE request (for categories from these taxonomies) to ensure allow_multiple is enforced

• return a positive response when the association of a subsequent DELETE request does not (or no longer) exist

Changes

This commit enforces the allow_multiple setting on Taxonomies for the following relation models:

• MeasureCategory
• RecommendationCategory
• UserCategory

In each case, it achieves this using an around_save hook that triggers when we create or update one of these models.

This commit also makes the controllers for these relations respond to destroy requests on missing records as though they had succeeded.

Considerations

There are some downsides to this solution:

• Using an ActiveRecord callback makes the behaviour apply as a hidden side effect, which may be surprising.

• Updating allow_multiple on a Taxonomy will not trigger the callback for each of the linked relationships.

[tmfrnz]

have deployed to nz-dev but it does not appear to work as intended/expected:

having disabled the client-side check (that normally ensures that only one category is assigned when multiple are not allowed), I tried to assign multiple categories (progress status taxonomy where allow_multiple = false) to an action which is triggering as many CREATE requests.

expected outcome: the action will have only one category assigned, i.e. from the latest request

observed outcome: the action appeared to (a) have all categories assigned on the client (as all requests yielded a positive response with the newly created association) however (b) it appears that sometimes NONE of the associations were actually stored in the database. further (c) it appears that assigning a single-association-category that ALL previous category associations including from other taxonomies are being removed

before assignment:

after assignment:

after page reload all associations are gone (illustrating (b) and (c))

after page reload (illustrating (c)):

ad a) although I didn't expect this, it makes sense the way we specified it, i.e. if each request replaces the former connection, the previous responses will yield a positive result (not knowing that it will be removed the next moment). unless we change the implementation (i.e. use one request to create multiple connections), this is mostly a client side issue (which would be prevented from only requesting to create one association at a time). could leave as is for now as long as the database will be valid

ad b) not sure why this is sometimes happening

ad c) please make sure that only categories from the same taxonomy are destroyed (think this is where the issue lies https://github.com/impactoss/impactoss-server/pull/417/files#diff-98cc269f45b5a8127b196083487b9fcfc6357763c8f4568057c5ff0c30ca0b39R35)

[lukearndt]

@tmfrnz I have completed an update that applies the rules as I now understand them, with extensive specs.

Could you please verify that it now works as intended?

[tmfrnz]

it appears that only categories of the same taxonomy are deleted now for those taxonomies. however it also looks like that still sometimes no category assignment remains (of the same taxonomy) when assigning multiple categories at once (in reality 3 separate, concurrent requests)

my suspicion is that the after_commit hooks sometimes run after all PUT requests (commits) are processed and then deleting all respective other categories

[lukearndt]

I've changed the callback to around_save and wrapped it in a transaction. I expect that this will prevent multiple updates in quick succession from leading to incorrect data.

To verify that, I've updated the specs to apply multiple changes quickly across different threads. These new specs failed with the old after_commit implementation, and pass with the new around_save version.