Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixup: unselection of items from AJAX data sources with non-string identifiers #6241

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Apr 5, 2023

This pull request includes a

  • Bug fix

The following changes were made

  • Adds the jquery-mockjax plugin as a vendored integration test dependency.
  • Provides integration test coverage for selection-then-unselection of items based on an AJAX data source.
  • Performs normalization of result items received from AJAX sources.

Resolves #6128.

@jayaddison jayaddison changed the title Test coverage and fixup: unselection of items from AJAX data sources with non-string identifiers Fixup: unselection of items from AJAX data sources with non-string identifiers Apr 6, 2023
@kevin-brown
Copy link
Member

  • Maybe-controversial: removes some special-case handling for multi-select controls

Yeah those are too controversial to roll in here. We intentionally don't have single-select elements issue unselect because that'd always be paired with a select.

src/js/select2/data/ajax.js Outdated Show resolved Hide resolved
@kevin-brown
Copy link
Member

I am likely going to spin a new ajax-tests.js module out of this and move this test into it, since we don't currently have good tests around AJAX handling and this would be a solid first test to add. More than likely I would switch this to just be testing the AjaxAdapter itself and create a new test around the weirdness with creating <option>s on select events.

@jayaddison
Copy link
Contributor Author

Thanks @kevin-brown - how would you like to co-ordinate the creation of that test file? (should I attempt to create it here, or would you prefer to do that separately and move these changes into it?)

@jayaddison
Copy link
Contributor Author

Yeah those are too controversial to roll in here. We intentionally don't have single-select elements issue unselect because that'd always be paired with a select.

Sounds good; the single-select unselect event change here has been reverted.

@jayaddison
Copy link
Contributor Author

Also a note: the jquery-mockjax component is available under dual MIT-or-GPLv2 licensing terms (it's up to the authors of the codebase where it is included to decide on the applicable terms). I think I did check the license compatibility before choosing it for this branch, but wanted to double-check that again anyway.

@kevin-brown
Copy link
Member

Good call on checking the license, thank you for switching back the unselect changes. This looks good to go from a high level, so I'll mark it for my next sweep.

I'll take care of the testing changes independently, the tests you wrote get the job done for this change set. Anything I add would likely be in addition to the ones here and aren't required.

@jayaddison
Copy link
Contributor Author

Hey @kevin-brown - I'm afraid I'm joining the chorus of 'any updates on this' here - is there anything I can do to help this and/or the 4.1.0 milestone along?

@jayaddison
Copy link
Contributor Author

Note, partly since licensing was discussed here previously: jquery-mockjax v2.6.1 has consolidated from dual MIT-and-GPLv2 licensing to a single MIT license (ref: jakerella/jquery-mockjax@8183c94 and the associated changelog entry).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot unselect by clicking selected when using dataAdapter
2 participants