Fix AggregatorObserver sync status

[qmeister]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Related Issue	Fix #285 and #301
Need Doc update	no

Describe your change

Currently, the deleted and forceDeleted event handlers are disabled in AggregatorObserver if disableSearchSyncing has been set for an aggregator model. This should only disable the event handlers for the aggregator models and not the event handlers for aggregator class itself. For this disableSearchSyncing should only be handled by the ModelObserver and not by the AggregatorObserver like it is currently implemented with the saved event handler in ModelObserver and AggregatorObserver.

What problem is this fixing?

Currently, if multiple models are used in one index by using an aggregator and you don't want these to sync also by using disableSearchSyncing, these models won't be removed from the index on a delete, because the AggregatorObserver respects the disableSearchSyncing from its models while this should be respected by the ModelObserver only.

See Multiple models in one index article for feature reference.

[kellerjmrtn]

@qmeister Thanks for making this, I'd love to see this merged in

Seems like some tests are failing to connect to a test environment? Unsure what's going on there but I guess there's not much we can do about it

[qmeister]

@qmeister Thanks for making this, I'd love to see this merged in

Seems like some tests are failing to connect to a test environment? Unsure what's going on there but I guess there's not much we can do about it

Thanks @kellerjmrtn! Looks like these tests are good now though. @DevinCodes could you review this PR and/or let me know if you need anything from me to get this merged in?

[DevinCodes]

Hey @qmeister ,

Thanks for the PR! Were you able to confirm on your end this behaves as expected? The code changes look good to me in general, but we're missing a test that confirms the expected behaviour 🙂

[qmeister]

Hey @qmeister ,

Thanks for the PR! Were you able to confirm on your end this behaves as expected? The code changes look good to me in general, but we're missing a test that confirms the expected behaviour 🙂

Hi @DevinCodes ,

Thanks for the review and good call!! - Just added tests to confirm this :)

And yes, I can confirm this works as it should ... But ... Actually, while writing these tests I saw that my implementation was not correct. It works as expected, because I set disableSyncingFor only for aggregator models and not for the aggregator class itself. So actually the AggregatorObserver should not even respect this setting, because this is handled by the ModelObserver and it should only respect this for its models - as this already was correctly implemented for the saved event handlers in both observer classes.

So, I've removed the disableSyncingFor check from the AggregatorObserver, and additionally, I've created a separate PR to implement disableSyncFor for aggregator classes, see #359.

Further notes (FYI):

• I do see some other tests failing due to a missing Algolia application id, which looks like it's not connected to this PR.
• I've squashed both commits in one, so all changes are in one commit including the tests.
• Also, I've modified the PR's description to match the actual fix corresponding my explanation.

[DevinCodes]

Thank you for the PR @qmeister 🙇‍♂️ I'll release this tomorrow in the morning!

[DevinCodes]

This is out now in https://github.com/algolia/scout-extended/releases/tag/v3.2.2 ! Thanks again for the contribution 😄

[qmeister]

This is out now in https://github.com/algolia/scout-extended/releases/tag/v3.2.2 ! Thanks again for the contribution 😄

Awesome, thank you!