Revert "Unselect events: cache lookup key fix (#6179)"

[jayaddison]

This reverts commit d961613 (#6179).

Re-opens #6128.

[kevin-brown]

Reverting this and taking a deeper look into the underlying issue.

[jayaddison]

Thanks, @kevin-brown - I'm taking another look at it as well.

From the commit-log for the original pull request: I do seem to remember that something about the data object during unselection events didn't seem correct (mismatched key, I think -- not an incorrect datatype (string vs int), but a different value) - and looking it up on the option element seemed to provide an alternative path.

[jayaddison]

My best guess at the moment is that it could be something to do with the item._resultId value (as assigned here). Unconfirmed so far, though.

[jayaddison]

Currently stuck trying to get grunt test to run as expected (even if tests fail). I don't think that the phantomjs webserver is starting as it should do (there's an error message about being unable to read a property pid from an undefined object).

[jayaddison]

I've been spending attention on other tasks recently but I do intend to take a look at this again soon. It's good to see that continuous integration is working again, and that might help to solve some of the test environment problems I was running into.

[jayaddison]

Ok, I spent some time yesterday on this. With #6234 in place I'm able to run the grunt tests locally, and that provides a lot more confidence and support.

I've found it difficult to decide exactly what to test, and where to place those tests.

• Are we testing the bug around int vs string ID fields?
  • Currently, I think not -- that's quite a well-known issue and should be easy to describe and for people to fix. Yes it'd be nice to somehow magically sort out, but the fact is that if someone provides an integer key, then it's difficult to use that throughout the library's API, because at some level the key has to become a string attribute on the relevant HTML DOM entities
• Are we testing the multi-select combined with option grouping behaviour? (as described in Cannot deselect items in grouped multiple select when data ids are not strings #3882, Bug: unselect an item doesn't change the state.selected in function templateResult(state) #6129)
  • This seems like a good candidate to begin with, yes - it seems like a relatively rare use case (so a fix should be incremental and not too disruptive), and @ikhvjs has provided a repro case that it should be possible to translate to a test case.
• Are we testing the behaviour of AJAX sources that respond with non-string keys? (such as Cannot unselect by clicking selected when using dataAdapter #6128)
  • Maybe - this was the original goal of Unselect events: cache lookup key fix #6179 to fix, but the fix wasn't proven and didn't have test coverage to prevent regressions.

After we choose what to test, deciding where to place the test coverage is tricky.

• An easy way to generate options without native IDs is to use autogenerated 'tags' on a multi-select. This could imply the data/test-tags unit test module.
• These bugs appear to occur most often in multi-select inputs, and they relate to unselect events, primarily. This could imply the selection/multiple unit test module.
• Is it possible that the bug relates to interactions between multi-selects and the allowClear config setting? If so, maybe an integration test module would be more appropriate (but which one?)

These are the problems I'm considering at the moment. I also realized that the fix in #3882 -- that I only noticed today -- still looks relevant and may be a nice small change (with a corresponding test case to show what it fixes).

[kevin-brown]

Alright, I've been following along so I should finally chime in with some direction of where to head next.

• Are we testing the multi-select combined with option grouping behaviour?

Yes, I think this is one of the core issues.

• Are we testing the behaviour of AJAX sources that respond with non-string keys?

It'd also be nice to cover this too, but, just like the first bullet point, I lean more towards grouping this in with the existing fixes for non-string keys.

• This could imply the data/test-tags unit test module.

Lean pretty strongly as a no here, since in tests we can mock data that has non-string keys (most notably in data/array module). Unless this ends up being a change that hits the tags module (which based on the reports seems unlikely), I'd rather keep the tests out of there.

• This could imply the selection/multiple unit test module.

To me this seems like the most likely candidate if we do trace this down to a multi-select specific issue (which, as you note, seems most likely). Especially if the fix hits the select/unselect event handlers in the multi-select specific portions.

• Is it possible that the bug relates to interactions between multi-selects and the allowClear config setting?

If it does end up being this, then the tests would land in selection/allowClear module.

• If so, maybe an integration test module would be more appropriate (but which one?)

Generally I try to keep the integration tests to the stuff that is jQuery-specific (like it requires full initialization or is called directly by the jQuery integration). Historically this has been a lot of the browser-specific quirks that come from bugs that we've found in jQuery and then had to patch in Select2.

These are the problems I'm considering at the moment. I also realized that the fix in #3882 -- that I only noticed today -- still looks relevant and may be a nice small change (with a corresponding test case to show what it fixes).

If you think that one is still valid, drop a comment in the ticket pinging me and I can reopen it. It was closed automatically before we configured Stale to not close confirmed bugs.

[jayaddison]

Ok, thank you for the guidance - I'll take another look at this soon (perhaps as soon as tomorrow) with all that in mind.

Confirming the relevance of #3882 during the test case writeup and/or fix probably won't be my primary goal, but if the process does provide enough information to verify it, I'll comment on the ticket as suggested.

[jayaddison]

In reply to these two parts of the thread in particular:

• Are we testing the multi-select combined with option grouping behaviour?

Yes, I think this is one of the core issues.

It'd also be nice to cover this too, but, just like the first bullet point, I lean more towards grouping this in with the existing fixes for non-string keys.

The changes in #6239 should provide relevant test coverage and a fix, and is now ready for review.

The root cause uncovered did appear to be non-string keys (in grouped/child data elements).