[WIP] Feat: Search for model instances

[khanxmetu]

• Write JSON view based on AutocompleteJsonView
• Create custom AdminSite, and override default admin site, restructure app config
• Add the new site-wide view to urlpatterns of the custom AdminSite
• Add view tests
• Add selenium tests
  resolves Add a shortcut to jump to any model instance #5

[khanxmetu]

• Simplify by removing the remote_model, source_field and to_field variables.
• Only term, app_label, model_name are required in the request.
• app_label and model_name are used to retrieve the specified model's ModelAdmin for querying.
• The queryset is then serialized into a list of dictionaries with primary key and str representation of the object

[khanxmetu]

I decided to create a custom AdminSite to override and add new methods to it while retaining a similar structure so it could be merged into django easily.

[khanxmetu]

This is added to avoid UnorderedObjectListWarning. I'd like to see deterministic results on each search but I'm not quite sure if we should force the ordering ourselves. For reference, select2's autocomplete view doesn't take care of this.

[knyghty]

@khanxmetu Thanks, this looks interesting. Did you try using the existing view? Is anything here different from the one used by autocomplete_fields? I was kind of hoping we could use the existing view directly somehow.

[khanxmetu]

The existing autocomplete view does not work here as it is.

Autocomplete requires 4 request parameters: term, app_label, model_name, field_name. The difference here is the use field_name which needs to be a ForeignKey or Many-to-Many field in the (source) model describing its relationship with another/remote model on which partial search is to be conducted. Here the ModelAdmin associated with the remote model is needed for get_search_results(). It is necessary to have both source and remote models here for things like ForeignKey.limit_choices_to and to_field which the view uses.

In our case we require only 3 parameters: term, app_label and model_name. We don't need to use field_name and instead directly use the ModelAdmin associated with the source model for get_search_results().

Therefore a request like:

/admin/autocomplete?term=bob&app_label=demo&model_name=artist

Would fail since autocomplete view would expect a field_name to derive remote model on which to conduct partial search as described above.

[knyghty]

Ah I understand, so to merge this it would be required to change the signature and code here a bit. That's probably acceptable. Before really looking to merge this I would want to have the frontend working also, even if it's ugly.

I'd also suggest setting up pre-commit locally so it passes in CI.

[khanxmetu]

For the frontend, I think its okay to use select2 library that the autocomplete_fields already uses for async loading. It has some quirks like dependency on jquery, minor warning and does not seem to play out nicely with dialog by default. But I am not sure if it would be worth trying something else when the solution exists that is already being utilized in django codebase.

If we could agree on using select2 for instance search then I may also convince you to use it for model search too for this feature as well as go to change list feature as select2 allows navigating and selecting rows with keys without any additional javascript.

[khanxmetu]

@knyghty requesting your early feedback on select2-based frontend.

I can follow up with tests afterwards, if this looks in the right direction.

[knyghty]

@khanxmetu select2 is okay I think as long as it'll work with what's already in the admin.

In the future I think it makes sense to move away from select2 for both this and autocomplete_fields but it's probably the right way to go for now.

[CodemanRichard]

The existing autocomplete view does not work here as it is.

Autocomplete requires 4 request parameters: term, app_label, model_name, field_name. The difference here is the use field_name which needs to be a ForeignKey or Many-to-Many field in the (source) model describing its relationship with another/remote model on which partial search is to be conducted. Here the ModelAdmin associated with the remote model is needed for get_search_results(). It is necessary to have both source and remote models here for things like ForeignKey.limit_choices_to and to_field which the view uses.

In our case we require only 3 parameters: term, app_label and model_name. We don't need to use field_name and instead directly use the ModelAdmin associated with the source model for get_search_results().

Therefore a request like:

/admin/autocomplete?term=bob&app_label=demo&model_name=artist

Would fail since autocomplete view would expect a field_name to derive remote model on which to conduct partial search as described above.

Hi, I have a little suggestion here @khanxmetu : can we let InstanceSearchJsonView inherit from AutocompleteJsonView? I have two reasons for this:

1. Three functions in these two classes are the same: serialize_result, get_paginator, has_perm. Inheritance means we don't have to write the implementations of these functions again in our class, which will make the code more readable.

2. InstanceSearchJsonView is based on AutocompleteJsonView, inheritance makes this logic more clear so that we don't have to illustrate this with comment.

[khanxmetu]

@CodemanRichard Although this view was written with reference to AutocompleteJsonView, I don't think inheritance should be used here. InstanceSearchJsonView expects only a subset of request parameters with different semantics. That is, this view searches using model_name's ModelAdmin vs the other searches using field_name's ModelAdmin. Therefore the core logic is somewhat different.

Only two trivial methods get_paginator and has_perm are the same. If there were many more helper methods in common, refactoring would have made sense in my opinion.

[CodemanRichard]

@khanxmetu Yes you're right, the core logic is different, then let's just don't use inheritance here. Thanks for your explanation.

[khanxmetu]

@knyghty I believe this PR is ready for another look. I had to make some changes unrelated to this PR specifically to get the urls and tests working. I think this is quite a big changeset, if you want me to split it into multiple PRs, let me know.

[coveralls]

Pull Request Test Coverage Report for Build 14251904300

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 194 of 272 (71.32%) changed or added relevant lines in 8 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.5%) to 73.856%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
django_admin_keyboard_shortcuts/sites.py	17	18	94.44%
django_admin_keyboard_shortcuts/views/instance_search.py	50	57	87.72%
runtests.py	15	26	57.69%
tests/test_instance_search_view.py	96	155	61.94%

Totals
Change from base Build 14103661975:	0.5%
Covered Lines:	219
Relevant Lines:	297

---

💛 - Coveralls

[knyghty]

@khanxmetu can you give me some instructions how to get this branch working? I'm using my package https://github.com/knyghty/django-admin-demo and I can't seem to get the settings right to test manually. If I remove django.contrib.admin and set django_admin_keyboard_shortcuts.apps.KSAdminConfig in INSTALLED_APPS then I can get it to load but I don't see any of the shortcuts code loading anywhere.

[khanxmetu]

@knyghty In INSTALLED_APPS, django_admin_keyboard_shortcuts.apps.KSAdminConfig replacesdjango.contrib.admin, and django_admin_keyboard_shortcuts.apps.AdminKeyboardShortcutsConfig replaces django_admin_keyboard_shortcuts, while retaining the same order mentioned in the original README.

Here's how my INSTALLED_APPS look like for django-admin-demo:

INSTALLED_APPS = [
    "demo",
    "django_admin_keyboard_shortcuts.apps.AdminKeyboardShortcutsConfig",
    "django_admin_keyboard_shortcuts.apps.KSAdminConfig",
    "django.contrib.auth",
    "django.contrib.contenttypes",
    "django.contrib.sessions",
    "django.contrib.messages",
    "django.contrib.staticfiles",
    "django.contrib.redirects",
    "django.contrib.sites",
    "django.contrib.admindocs",
    "django.contrib.flatpages",
]

Alternatively, I suggest you to try running selenium tests since they have their own settings.py configured and they simulate shortcut keys to open and test instance search dialog:

./runtests --selenium=chrome

[knyghty]

Sorry, it's a bit embarrassing how long it took me to get to this. Now that we're in the context of GSoC (though also aware we've not really started yet)...

It works! That's amazing. We could merge this here, once it's ready. But what I'm considering is if there's part of this that we could merge into Django first? I'm not sure what that would be, exactly, perhaps the changes to the AutocompleteJsonView?

I also wonder about the future viability of jQuery. We're already trying to reduce its usage in the admin. However I don't know if there's a good select2 alternative out there. It would be a shame to add more jQuery into Django I think, but also not the end of the world.