From 037d43111f0dfc10ab6dd83db5cbb29c5d5e1918 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Tue, 6 Dec 2022 18:24:39 +0100 Subject: [PATCH] Allow LIKE searches for email in user admin, drop search by username (#20057) * Allow LIKE searches for email in user admin, drop search by username * Add test --- src/olympia/amo/models.py | 18 +++++++++- src/olympia/amo/tests/test_models.py | 17 +++++++++ src/olympia/users/admin.py | 15 +++++--- src/olympia/users/tests/test_admin.py | 52 +++++++++++++++++++++++++-- 4 files changed, 94 insertions(+), 8 deletions(-) diff --git a/src/olympia/amo/models.py b/src/olympia/amo/models.py index 5c56ab2457..405f899c52 100644 --- a/src/olympia/amo/models.py +++ b/src/olympia/amo/models.py @@ -7,8 +7,9 @@ from urllib.parse import urljoin from django.conf import settings from django.core.files.storage import default_storage as storage from django.db import models +from django.db.models import Lookup from django.db.models.expressions import Func -from django.db.models.fields import CharField +from django.db.models.fields import CharField, Field from django.db.models.fields.related_descriptors import ManyToManyDescriptor from django.db.models.query import ModelIterable from django.urls import resolve, reverse @@ -26,6 +27,21 @@ from olympia.translations.hold import save_translations log = olympia.core.logger.getLogger('z.addons') +@Field.register_lookup +class Like(Lookup): + lookup_name = 'like' + + def as_sql(self, compiler, connection): + lhs_sql, params = self.process_lhs(compiler, connection) + rhs_sql, rhs_params = self.process_rhs(compiler, connection) + params.extend(rhs_params) + # This looks scarier than it is: rhs_sql should to resolve to '%s', + # lhs_sql to the query before this part. The params are isolated and + # will be passed to the database client code separately, ensuring + # everything is escaped correctly. + return '%s LIKE %s' % (lhs_sql, rhs_sql), params + + @contextlib.contextmanager def use_primary_db(): """Within this context, all queries go to the master.""" diff --git a/src/olympia/amo/tests/test_models.py b/src/olympia/amo/tests/test_models.py index 732a7e8467..29f6ff65dd 100644 --- a/src/olympia/amo/tests/test_models.py +++ b/src/olympia/amo/tests/test_models.py @@ -470,3 +470,20 @@ class TestFilterableManyToManyField(TestCase): assert list(self.twinkle_twinkle.performers.all()) == [self.bob] # But Joe is still on the other song assert list(self.humpty_dumpty.performers.all()) == [self.sue, self.joe] + + +class TestLikeLookup(TestCase): + def test_basic(self): + song = Song.objects.create(name='ThisIsAName') + qs = Song.objects.filter(name__like='This%aname') + assert qs.get() == song + assert qs.query.sql_with_params() == ( + # 2 different kinds of `%`: + # - the first one, `%s` in the query itself, is where mysqlclient + # will insert the parameter + # - the second one, in the params, is the wildcard for the LIKE + # query + 'SELECT `m2m_testapp_song`.`id`, `m2m_testapp_song`.`name` ' + 'FROM `m2m_testapp_song` WHERE `m2m_testapp_song`.`name` LIKE %s', + ('This%aname',), + ) diff --git a/src/olympia/users/admin.py b/src/olympia/users/admin.py index c3545525d8..fb6e141a3a 100644 --- a/src/olympia/users/admin.py +++ b/src/olympia/users/admin.py @@ -63,11 +63,14 @@ class GroupUserInline(admin.TabularInline): @admin.register(UserProfile) class UserAdmin(CommaSearchInAdminMixin, admin.ModelAdmin): list_display = ('__str__', 'email', 'last_login', 'is_public', 'deleted') - search_fields = ('=id', '^email', '^username') - # A custom ip address search is implemented in get_search_results() using - # IPLog. It sets an annotation that we can then use in the - # custom `known_ip_adresses` method referenced in the line below, which - # is added to the list_display fields for IP searches. + # pk and IP address search are supported without needing to specify them in + # search_fields (see `CommaSearchInAdminMixin.get_search_results()` and + # `get_search_id_field()` as well as `get_search_results()` below) + search_fields = ('email__like',) + # get_search_results() below searches using `IPLog`. It sets an annotation + # that we can then use in the custom `known_ip_adresses` method referenced + # in the line below, which is added to the` list_display` fields for IP + # searches. extra_list_display_for_ip_searches = ('known_ip_adresses',) # A custom field used in search json in zadmin, not django.admin. search_fields_response = 'email' @@ -264,6 +267,8 @@ class UserAdmin(CommaSearchInAdminMixin, admin.ModelAdmin): # duplicates and avoid doing a DISTINCT. may_have_duplicates = False else: + # We support `*` as a wildcard character for `email__like` lookup. + search_term = search_term.replace('*', '%') queryset, may_have_duplicates = super().get_search_results( request, queryset, diff --git a/src/olympia/users/tests/test_admin.py b/src/olympia/users/tests/test_admin.py index d4116c1c7a..239ee98e79 100644 --- a/src/olympia/users/tests/test_admin.py +++ b/src/olympia/users/tests/test_admin.py @@ -47,20 +47,68 @@ class TestUserAdmin(TestCase): 'admin:users_userprofile_delete', args=(self.user.pk,) ) - def test_search_for_multiple_users(self): + def test_search_by_email_simple(self): user = user_factory(email='someone@mozilla.com') self.grant_permission(user, 'Users:Edit') self.client.force_login(user) another_user = user_factory() response = self.client.get( self.list_url, - {'q': f'{self.user.pk},{another_user.pk},foobaa'}, + {'q': self.user.email}, + follow=True, + ) + assert response.status_code == 200 + doc = pq(response.content) + assert str(self.user.pk) in doc('#result_list').text() + assert str(another_user.pk) not in doc('#result_list').text() + + def test_search_by_email_like(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Users:Edit') + self.client.force_login(user) + another_user = user_factory(email='someone@notzilla.org') + response = self.client.get( + self.list_url, + {'q': 'some*@notzilla.org'}, + follow=True, + ) + assert response.status_code == 200 + doc = pq(response.content) + assert str(another_user.pk) in doc('#result_list').text() + assert str(self.user.pk) not in doc('#result_list').text() + assert str(user.pk) not in doc('#result_list').text() + + def test_search_by_email_multiple(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Users:Edit') + self.client.force_login(user) + another_user = user_factory() + response = self.client.get( + self.list_url, + {'q': f'{self.user.email},{another_user.email},foobaa'}, follow=True, ) assert response.status_code == 200 doc = pq(response.content) assert str(self.user.pk) in doc('#result_list').text() assert str(another_user.pk) in doc('#result_list').text() + assert str(user.pk) not in doc('#result_list').text() + + def test_search_by_email_multiple_like(self): + user = user_factory(email='someone@mozilla.com') + self.grant_permission(user, 'Users:Edit') + self.client.force_login(user) + another_user = user_factory(email='someone@notzilla.com') + response = self.client.get( + self.list_url, + {'q': 'some*@mozilla.com,*@notzilla.*,foobaa'}, + follow=True, + ) + assert response.status_code == 200 + doc = pq(response.content) + assert str(user.pk) in doc('#result_list').text() + assert str(another_user.pk) in doc('#result_list').text() + assert str(self.user.pk) not in doc('#result_list').text() def test_search_for_multiple_user_ids(self): """Test the optimization when just searching for matching ids."""