diff --git a/docs/expert-admin/README.md b/docs/expert-admin/README.md new file mode 100644 index 00000000..5fdebbb0 --- /dev/null +++ b/docs/expert-admin/README.md @@ -0,0 +1,132 @@ +# DEV: поиск и фильтр программ для Experts + +Exact base: `11f1e66a85e266d57fc6b0f8c080ee8cd0683f19` (`origin/dev` после fetch). +Ветка: `feature/dev-expert-admin-search`; exact head указан в Draft PR. + +## Изменение + +В `users/admin.py`, `ExpertAdmin`, добавлены три стандартные настройки: + +```python +search_fields = ("user__first_name", "user__last_name", "user__email") +list_filter = ("user__partner_program_profiles__partner_program",) +list_select_related = ("user",) +``` + +`list_display = ("id", "user")` сохранён. Кастомного frontend, шаблона или +`SimpleListFilter` нет. Black также перенёс строки существующего выражения +`CustomUserAdmin.formfield_for_manytomany`; его поведение не изменено. + +**Search:** стандартный Django Admin `icontains` по трём полям. Отдельные слова +соединяются через AND, поля для каждого слова — через OR. Поэтому полное имя +ищется по нескольким полям без собственной нормализации/логики. Это не точное +сравнение ФИО: запрос «Иван Иванов» также может найти «Пётр Иванов», поскольку оба +слова входят в фамилию. Такое стандартное поведение явно покрыто тестом. + +Регистронезависимость зависит от базы данных, а не от дополнительной логики Admin: + +- **PostgreSQL:** поиск кириллицы без учёта регистра проверяется отдельным тестом + `test_search_cyrillic_is_case_insensitive_on_postgresql` в Backend PostgreSQL CI. +- **SQLite CI:** portable-проверка использует ASCII email — `anna@example.com` / + `ANNA@EXAMPLE.COM`. SQLite LIKE не обеспечивает полноценную регистронезависимость + Unicode/кириллицы; кириллический тест явно пропускается при + `connection.vendor != "postgresql"`. + +Production `ExpertAdmin`, `search_fields` и workflows этой доработкой не меняются. + +**Filter:** `RelatedFieldListFilter` по legacy пути +`Expert.user → CustomUser.partner_program_profiles → PartnerProgramUserProfile.partner_program`. +Choices формируются из существующих PartnerProgram через их `__str__`, включая +программы без экспертов. Справа стандартные `FILTER`, `By partner program`, `All`. +Без выбранного значения остаются все Experts, в том числе без программы. Search и +program filter работают одновременно; `All` снимает программу, сохраняя search. + +**Duplicates:** явный `.distinct()` не добавлен. В установленном из lockfile Django +4.2.11 `ChangeList.get_queryset()` распознаёт reverse FK как потенциальный источник +дублей и использует коррелированный SQL `EXISTS`. Эксперт в двух программах +отображается один раз без фильтра и один раз в каждой программе. Тесты сравнивают +список ID и количество строк, а не множества, чтобы не скрыть дубли. + +Несуществующий числовой program ID даёт пустой список; нечисловой ID проходит +стандартную обработку Admin — redirect на `?e=1` без HTTP 500. + +**Performance:** `list_select_related = ("user",)` загружает строки и пользователей +одним SQL-запросом. Проверено на queryset changelist с фильтром и без фильтра: +получение всех строк и чтение `str(expert.user)` укладывается в один запрос. +У самого changelist остаются стандартные запросы counts, choices, session/permissions; +утверждение об одном запросе относится к строкам таблицы вместе с пользователями. + +Модели, schema, migrations, staff/superuser permissions и бизнес-правила не менялись. +Angular, React, Application/Team/Submission/current_application не затронуты. + +## Первоначальные проверки (head `381afde`) + +Локальная среда: Python 3.11.15, Django 4.2.11, PostgreSQL 18.1 (UTF-8, ICU ru-RU), +Black 22.12.0. Отдельный временный кластер PostgreSQL на loopback, отдельные базы +для тестов и browser QA, `procollab.settings_ci`. DEV/PROD данные не использовались. + +- `python manage.py test users.tests.test_expert_admin --noinput --verbosity 2`: + **18 tests OK**, exit 0. +- `python manage.py test --noinput --verbosity 1 --keepdb` (режим существующего CI): + **919 tests OK**, 458.293 s, exit 0; test failures/errors 0. +- `python manage.py check`: **0 issues**, exit 0. +- `python manage.py makemigrations --check --dry-run`: **No changes detected**, exit 0. +- Black / Flake8 для `users/admin.py` и `users/tests/test_expert_admin.py`: exit 0. +- `git diff --check`: exit 0. + +Первый полный запуск без `--keepdb`: **919 tests OK**, 502.401 s, но **exit 1** +на teardown — оставшиеся 21 соединение помешали `DROP DATABASE`. Ошибок тестовых +сценариев не было. Повторный запуск использует существующий режим CI `--keepdb` +с отдельной очисткой изолированной локальной БД; test harness не изменён. + +18 тестов покрывают search по имени/фамилии/email/словам полного имени, регистр, +пустую выдачу, All, программы, отсутствие регистрации, двойную регистрацию, +search+filter, отсутствующий/невалидный ID, запросы user column, запрет доступа +для non-staff и staff без Expert permissions. + +## Проверки после разделения SQLite / PostgreSQL + +В наборе 19 admin-тестов: + +| БД | Targeted `users.tests.test_expert_admin` | Case-insensitive | +| --- | --- | --- | +| PostgreSQL | 19 выполнены, 0 skipped, exit 0 | ASCII email и кириллица | +| SQLite | 18 выполнены, 1 skipped, exit 0 | ASCII email; кириллица явно skipped | + +Обычный SQLite suite запускается с `DJANGO_SETTINGS_MODULE=procollab.settings`, +`DEBUG=True`: `python manage.py test --noinput --verbosity 2`, как в обычном CI. +Полный PostgreSQL suite: `DJANGO_SETTINGS_MODULE=procollab.settings_ci`, +`python manage.py test --noinput --verbosity 1 --keepdb`, как в Backend PostgreSQL CI. +Проверки используют разные локальные рабочие копии/БД, без изменения workflows +или настроек приложения. Django check, Black, Flake8 и diff-check проходят. +Итоги полных прогонов и обоих GitHub CI на актуальном exact head зафиксированы +в [PR #758](https://github.com/PROCOLLAB-github/api/pull/758). + +## Browser QA + +Настоящий `/admin/users/expert/` с обычным входом superuser на изолированной +локальной PostgreSQL-базе. API/HTML не подменялись. Chrome 154.0.8037.57, 1440 px: +**17 assertions, 11 состояний выдачи, 0 JS errors / HTTP 500**, exit 0. +Проверены поиск по имени, фамилии, email, полному имени, отсутствие результата, +filter, search+filter, All, двойная регистрация и невалидный/несуществующий ID. + +- [Все эксперты: search и sidebar filter](all-experts.png). +- [Иванов + FinFor25-26](search-and-program.png). +- [Численные результаты](browser-results.json). +- [Browser script](browser-acceptance.cjs). + +Для воспроизведения нужен отдельный PostgreSQL, зависимости из lockfile и +`procollab.settings_ci`. После `manage.py migrate` на пустой QA-базе вызовите +`ExpertAdminTests.setUpTestData()` из `users.tests.test_expert_admin`, сохранив +PK программы и экспертов в `.cache/expert-admin-qa/fixtures.json` с ключами +`program`, `otherProgram`, `ivan`, `anna`, `otherIvanov`, `noProgram`. +Стандартный Django staticfiles runserver можно вызвать через +`call_command(Command(), "127.0.0.1:8340", use_reloader=False, insecure_serving=True)`; +`Command` импортируется из `django.contrib.staticfiles.management.commands.runserver`. +Это обходит только локальное ограничение Daphne runserver на `--insecure`; +настройки приложения, cookies, auth и permissions не менялись. +Запустите `node docs/expert-admin/browser-acceptance.cjs` из корня репозитория; +`PLAYWRIGHT_MODULE` и `CHROME_PATH` могут указывать на внешние Playwright/Chrome. +Login script использует только известную тестовую учётную запись из fixtures. + +Merge и deploy не выполнялись. diff --git a/docs/expert-admin/all-experts.png b/docs/expert-admin/all-experts.png new file mode 100644 index 00000000..562ff3ca Binary files /dev/null and b/docs/expert-admin/all-experts.png differ diff --git a/docs/expert-admin/browser-acceptance.cjs b/docs/expert-admin/browser-acceptance.cjs new file mode 100644 index 00000000..63afc778 --- /dev/null +++ b/docs/expert-admin/browser-acceptance.cjs @@ -0,0 +1,159 @@ +const { chromium } = require(process.env.PLAYWRIGHT_MODULE || "playwright"); +const assert = require("assert/strict"); +const fs = require("fs"); + +// Run only against the isolated local QA database seeded with ExpertAdminTests.setUpTestData(). +const fixture = JSON.parse( + fs.readFileSync(".cache/expert-admin-qa/fixtures.json", "utf8"), +); +const baseUrl = "http://127.0.0.1:8340/admin/users/expert/"; +const parameter = "user__partner_program_profiles__partner_program__id__exact"; +const output = "docs/expert-admin"; +const findings = []; +let assertions = 0; +function check(actual, expected, message) { + assert.deepEqual(actual, expected, message); + assertions++; +} + +(async () => { + const browser = await chromium.launch({ + headless: true, + executablePath: process.env.CHROME_PATH, + }); + const page = await browser.newPage({ + viewport: { width: 1440, height: 1000 }, + }); + const errors = []; + page.on("pageerror", (error) => errors.push(error.message)); + page.on("response", (response) => { + if (response.status() >= 500) + errors.push(`${response.status()} ${response.url()}`); + }); + await page.goto(baseUrl); + await page.locator("#id_username").fill("admin@example.com"); + await page.locator("#id_password").fill("very_strong_password"); + await Promise.all([ + page.waitForNavigation(), + page.locator('input[type="submit"]').click(), + ]); + await page.locator("#searchbar").waitFor(); + check( + await page.locator("#searchbar").isVisible(), + true, + "standard search visible", + ); + check( + (await page.locator("#changelist-filter").innerText()).includes( + "By partner program", + ), + true, + "standard filter title", + ); + check( + await page.locator("#changelist-filter a").count(), + 4, + "All and three programs", + ); + + async function rows(scenario, expected) { + const ids = await page + .locator("#result_list input.action-select") + .evaluateAll((nodes) => + nodes.map((node) => Number(node.value)).sort((a, b) => a - b), + ); + check( + ids, + [...expected].sort((a, b) => a - b), + scenario, + ); + findings.push({ scenario, expertIds: ids, url: page.url() }); + } + async function search(query) { + await page.locator("#searchbar").fill(query); + await Promise.all([ + page.waitForNavigation(), + page.locator('#changelist-search input[type="submit"]').click(), + ]); + } + await rows("all", [ + fixture.ivan, + fixture.anna, + fixture.otherIvanov, + fixture.noProgram, + ]); + await page.screenshot({ path: `${output}/all-experts.png`, fullPage: true }); + for (const [query, expected] of [ + ["Анна", [fixture.anna]], + ["Иванов", [fixture.ivan, fixture.otherIvanov]], + ["иван@example.com", [fixture.ivan]], + ["Иван Иванов", [fixture.ivan, fixture.otherIvanov]], + ["Несуществующий", []], + ]) { + await search(query); + await rows(`search: ${query}`, expected); + } + await page.goto(baseUrl); + await Promise.all([ + page.waitForNavigation(), + page + .locator("#changelist-filter a") + .filter({ hasText: `PartnerProgram<${fixture.program}> - FinFor25-26` }) + .click(), + ]); + await rows("program", [fixture.ivan, fixture.anna]); + await search("Иванов"); + await rows("search + program", [fixture.ivan]); + check( + new URL(page.url()).searchParams.get(parameter), + String(fixture.program), + "search preserves filter", + ); + await page.screenshot({ + path: `${output}/search-and-program.png`, + fullPage: true, + }); + await Promise.all([ + page.waitForNavigation(), + page + .locator("#changelist-filter a") + .getByText("All", { exact: true }) + .click(), + ]); + await rows("All preserves search", [fixture.ivan, fixture.otherIvanov]); + await page.goto(`${baseUrl}?${parameter}=${fixture.otherProgram}`); + await rows("other program; dual registration appears once", [ + fixture.ivan, + fixture.otherIvanov, + ]); + await page.goto(`${baseUrl}?${parameter}=999999999`); + await rows("nonexistent program", []); + await page.goto(`${baseUrl}?${parameter}=not-a-number`); + check( + new URL(page.url()).searchParams.get("e"), + "1", + "invalid filter redirects to admin error marker", + ); + check(errors, [], "no runtime errors or HTTP 500"); + fs.writeFileSync( + `${output}/browser-results.json`, + JSON.stringify( + { + base: "11f1e66a85e266d57fc6b0f8c080ee8cd0683f19", + browser: await browser.version(), + assertions, + errors, + findings, + }, + null, + 2, + ) + "\n", + ); + console.log( + `PASS ${assertions} assertions, ${findings.length} result states, errors ${errors.length}`, + ); + await browser.close(); +})().catch((error) => { + console.error(error); + process.exit(1); +}); diff --git a/docs/expert-admin/browser-results.json b/docs/expert-admin/browser-results.json new file mode 100644 index 00000000..2b5d0a19 --- /dev/null +++ b/docs/expert-admin/browser-results.json @@ -0,0 +1,63 @@ +{ + "base": "11f1e66a85e266d57fc6b0f8c080ee8cd0683f19", + "browser": "154.0.8037.57", + "assertions": 17, + "errors": [], + "findings": [ + { + "scenario": "all", + "expertIds": [1, 2, 3, 4], + "url": "http://127.0.0.1:8340/admin/users/expert/" + }, + { + "scenario": "search: Анна", + "expertIds": [2], + "url": "http://127.0.0.1:8340/admin/users/expert/?q=%D0%90%D0%BD%D0%BD%D0%B0" + }, + { + "scenario": "search: Иванов", + "expertIds": [1, 3], + "url": "http://127.0.0.1:8340/admin/users/expert/?q=%D0%98%D0%B2%D0%B0%D0%BD%D0%BE%D0%B2" + }, + { + "scenario": "search: иван@example.com", + "expertIds": [1], + "url": "http://127.0.0.1:8340/admin/users/expert/?q=%D0%B8%D0%B2%D0%B0%D0%BD%40example.com" + }, + { + "scenario": "search: Иван Иванов", + "expertIds": [1, 3], + "url": "http://127.0.0.1:8340/admin/users/expert/?q=%D0%98%D0%B2%D0%B0%D0%BD+%D0%98%D0%B2%D0%B0%D0%BD%D0%BE%D0%B2" + }, + { + "scenario": "search: Несуществующий", + "expertIds": [], + "url": "http://127.0.0.1:8340/admin/users/expert/?q=%D0%9D%D0%B5%D1%81%D1%83%D1%89%D0%B5%D1%81%D1%82%D0%B2%D1%83%D1%8E%D1%89%D0%B8%D0%B9" + }, + { + "scenario": "program", + "expertIds": [1, 2], + "url": "http://127.0.0.1:8340/admin/users/expert/?user__partner_program_profiles__partner_program__id__exact=1" + }, + { + "scenario": "search + program", + "expertIds": [1], + "url": "http://127.0.0.1:8340/admin/users/expert/?q=%D0%98%D0%B2%D0%B0%D0%BD%D0%BE%D0%B2&user__partner_program_profiles__partner_program__id__exact=1" + }, + { + "scenario": "All preserves search", + "expertIds": [1, 3], + "url": "http://127.0.0.1:8340/admin/users/expert/?q=%D0%98%D0%B2%D0%B0%D0%BD%D0%BE%D0%B2" + }, + { + "scenario": "other program; dual registration appears once", + "expertIds": [1, 3], + "url": "http://127.0.0.1:8340/admin/users/expert/?user__partner_program_profiles__partner_program__id__exact=2" + }, + { + "scenario": "nonexistent program", + "expertIds": [], + "url": "http://127.0.0.1:8340/admin/users/expert/?user__partner_program_profiles__partner_program__id__exact=999999999" + } + ] +} diff --git a/docs/expert-admin/search-and-program.png b/docs/expert-admin/search-and-program.png new file mode 100644 index 00000000..fc7beb71 Binary files /dev/null and b/docs/expert-admin/search-and-program.png differ diff --git a/users/admin.py b/users/admin.py index ff993c4f..a7b3936a 100644 --- a/users/admin.py +++ b/users/admin.py @@ -193,7 +193,9 @@ class CustomUserAdmin(admin.ModelAdmin): def formfield_for_manytomany(self, db_field, request, **kwargs): if db_field.name == "user_permissions": - kwargs["queryset"] = Permission.objects.select_related("content_type").order_by( + kwargs["queryset"] = Permission.objects.select_related( + "content_type" + ).order_by( "content_type__app_label", "content_type__model", "codename", @@ -429,6 +431,9 @@ class UserLinkAdmin(admin.ModelAdmin): @admin.register(Expert) class ExpertAdmin(admin.ModelAdmin): list_display = ("id", "user") + search_fields = ("user__first_name", "user__last_name", "user__email") + list_filter = ("user__partner_program_profiles__partner_program",) + list_select_related = ("user",) @admin.register(UserEducation) diff --git a/users/tests/test_expert_admin.py b/users/tests/test_expert_admin.py new file mode 100644 index 00000000..4fc0656e --- /dev/null +++ b/users/tests/test_expert_admin.py @@ -0,0 +1,193 @@ +from django.contrib.admin.filters import RelatedFieldListFilter +from django.db import connection +from django.test import TestCase +from django.urls import reverse +from django.utils import translation + +from users.models import CustomUser + +from .helpers import ( + add_user_to_program, + build_partner_program, + build_superuser, + build_user, +) + + +class ExpertAdminTests(TestCase): + program_parameter = "user__partner_program_profiles__partner_program__id__exact" + + @classmethod + def setUpTestData(cls): + cls.admin_user = build_superuser() + cls.program = build_partner_program(name="FinFor25-26", tag="finfor") + cls.other_program = build_partner_program(name="Другая программа", tag="other") + cls.empty_program = build_partner_program(name="Без экспертов", tag="empty") + cls.ivan = build_user( + email="иван@example.com", + first_name="Иван", + last_name="Иванов", + user_type=CustomUser.EXPERT, + ) + cls.anna = build_user( + email="anna@example.com", + first_name="Анна", + last_name="Смирнова", + user_type=CustomUser.EXPERT, + ) + cls.other_ivanov = build_user( + email="petr@example.com", + first_name="Пётр", + last_name="Иванов", + user_type=CustomUser.EXPERT, + ) + cls.no_program = build_user( + email="no-program@example.com", + first_name="Ольга", + last_name="Петрова", + user_type=CustomUser.EXPERT, + ) + cls.member = build_user(email="member@example.com") + add_user_to_program(cls.ivan, cls.program) + add_user_to_program(cls.ivan, cls.other_program) + add_user_to_program(cls.anna, cls.program) + add_user_to_program(cls.other_ivanov, cls.other_program) + add_user_to_program(cls.member, cls.program) + + def setUp(self): + self.client.force_login(self.admin_user) + self.url = reverse("admin:users_expert_changelist") + + def changelist(self, params=None): + response = self.client.get(self.url, params or {}) + self.assertEqual(response.status_code, 200) + return response + + def assert_experts(self, response, users): + # Keep a list: set equality would hide duplicate rows from a reverse JOIN. + result = list(response.context["cl"].result_list) + self.assertCountEqual( + [expert.user_id for expert in result], [user.pk for user in users] + ) + self.assertEqual(response.context["cl"].result_count, len(users)) + + def test_changelist_has_standard_search_and_program_choices(self): + with translation.override("en"): + response = self.changelist() + self.assertContains(response, 'id="searchbar"') + self.assertContains(response, 'id="changelist-filter"') + self.assertContains(response, "By partner program") + changelist = response.context["cl"] + program_filter = changelist.filter_specs[0] + self.assertIsInstance(program_filter, RelatedFieldListFilter) + choices = list(program_filter.choices(changelist)) + self.assertEqual(choices[0]["display"], "All") + self.assertTrue(choices[0]["selected"]) + self.assertCountEqual( + [choice["display"] for choice in choices[1:]], + [str(self.program), str(self.other_program), str(self.empty_program)], + ) + + def test_search_first_name(self): + self.assert_experts(self.changelist({"q": "Анна"}), [self.anna]) + + def test_search_last_name(self): + self.assert_experts( + self.changelist({"q": "Иванов"}), [self.ivan, self.other_ivanov] + ) + + def test_search_email(self): + self.assert_experts(self.changelist({"q": "иван@example.com"}), [self.ivan]) + + def test_search_full_name_matches_terms_across_fields(self): + # Standard admin search ANDs words across all fields, not exact full names. + self.assert_experts( + self.changelist({"q": "Иван Иванов"}), [self.ivan, self.other_ivanov] + ) + self.assert_experts(self.changelist({"q": "Анна Смирнова"}), [self.anna]) + self.assert_experts(self.changelist({"q": "Анна Иванов"}), []) + + def test_search_ascii_email_is_case_insensitive(self): + self.assert_experts(self.changelist({"q": "ANNA@EXAMPLE.COM"}), [self.anna]) + + def test_search_cyrillic_is_case_insensitive_on_postgresql(self): + # SQLite LIKE не обеспечивает полноценную регистронезависимость Unicode/кириллицы. + # Поведение production PostgreSQL проверяется в Backend PostgreSQL CI. + if connection.vendor != "postgresql": + self.skipTest( + "Регистронезависимость кириллицы проверяется только на PostgreSQL" + ) + self.assert_experts(self.changelist({"q": "АННА СМИРНОВА"}), [self.anna]) + + def test_unknown_search_is_empty(self): + self.assert_experts(self.changelist({"q": "Несуществующий"}), []) + + def test_without_filter_all_experts_appear_once_including_without_program(self): + self.assert_experts( + self.changelist(), [self.ivan, self.anna, self.other_ivanov, self.no_program] + ) + + def test_program_filter_excludes_other_program_and_unregistered_experts(self): + self.assert_experts( + self.changelist({self.program_parameter: self.program.pk}), + [self.ivan, self.anna], + ) + + def test_expert_registered_in_two_programs_appears_once_in_each(self): + for program, expected in ( + (self.program, [self.ivan, self.anna]), + (self.other_program, [self.ivan, self.other_ivanov]), + ): + with self.subTest(program=program.pk): + self.assert_experts( + self.changelist({self.program_parameter: program.pk}), expected + ) + + def test_program_without_experts_returns_empty_results(self): + self.assert_experts( + self.changelist({self.program_parameter: self.empty_program.pk}), [] + ) + + def test_search_and_program_filter_are_combined(self): + self.assert_experts( + self.changelist({self.program_parameter: self.program.pk, "q": "Иванов"}), + [self.ivan], + ) + + def test_all_clears_only_program_filter_and_keeps_search(self): + response = self.changelist( + {self.program_parameter: self.program.pk, "q": "Иванов"} + ) + changelist = response.context["cl"] + all_choice = next(iter(changelist.filter_specs[0].choices(changelist))) + response = self.client.get(self.url + all_choice["query_string"]) + self.assertEqual(response.status_code, 200) + self.assert_experts(response, [self.ivan, self.other_ivanov]) + + def test_nonexistent_program_is_empty(self): + self.assert_experts(self.changelist({self.program_parameter: 999999999}), []) + + def test_invalid_program_uses_standard_admin_redirect_without_server_error(self): + response = self.client.get(self.url, {self.program_parameter: "not-a-number"}) + self.assertRedirects(response, self.url + "?e=1") + + def test_user_column_does_not_query_once_per_expert(self): + for params in ({}, {self.program_parameter: self.program.pk}): + with self.subTest(params=params): + changelist = self.changelist(params).context["cl"] + # Clone to discard the render cache; rows and their users need one query. + with self.assertNumQueries(1): + rows = list(changelist.result_list.all()) + labels = [str(expert.user) for expert in rows] + self.assertTrue(labels) + + def test_non_staff_cannot_access_changelist(self): + self.client.force_login(self.ivan) + response = self.client.get(self.url) + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("admin:login"), response.url) + + def test_staff_without_expert_permission_cannot_access_changelist(self): + staff = build_user(email="staff@example.com", is_staff=True) + self.client.force_login(staff) + self.assertEqual(self.client.get(self.url).status_code, 403)