diff --git a/src/Controller/Admin/AccommodationBooking/FilterController.php b/src/Controller/Admin/AccommodationBooking/FilterController.php index b3b71fc..81c7b2d 100644 --- a/src/Controller/Admin/AccommodationBooking/FilterController.php +++ b/src/Controller/Admin/AccommodationBooking/FilterController.php @@ -6,7 +6,6 @@ namespace App\Controller\Admin\AccommodationBooking; use App\Controller\Traits\ListFilterTrait; use App\Entity\User; -use App\Form\Admin\Filter\AccommodationBookingFilterOptionsProvider; use App\Form\Admin\Filter\AccommodationBookingFilterType; use App\Form\Model\Filter\AccommodationBookingFilterDto; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -26,25 +25,19 @@ class FilterController extends AbstractController { use ListFilterTrait; - public function __construct( - private readonly AccommodationBookingFilterOptionsProvider $filterOptions, - ) { - } - #[Route('/admin/accommodation-booking/filter', name: 'app_admin_accommodationbooking_filter', methods: ['GET'])] public function filter(Request $request): Response { // ROLE_GROUPS_ADMIN is granted explicitly since 6c1073e4 removed it from ROLE_ADMIN's // role_hierarchy — an administrator who needs this has to hold it in their own right. - $seesAllBookings = $this->isGranted('ROLE_GROUPS_ADMIN'); + $isGroupsAdmin = $this->isGranted('ROLE_GROUPS_ADMIN'); $user = $this->getUser(); $filterView = $this->createListFilterView( $request, AccommodationBookingFilterType::class, - AccommodationBookingFilterDto::defaults($seesAllBookings, $user instanceof User ? $user : null), + AccommodationBookingFilterDto::defaults($isGroupsAdmin, $user instanceof User ? $user : null), 'app_admin_accommodationbooking', - $this->filterOptions->formOptions($seesAllBookings), ); return $this->render('admin/accommodation_booking/modal_filter.html.twig', [ diff --git a/src/Controller/Admin/AccommodationBooking/IndexController.php b/src/Controller/Admin/AccommodationBooking/IndexController.php index b9ad84a..ec726a5 100644 --- a/src/Controller/Admin/AccommodationBooking/IndexController.php +++ b/src/Controller/Admin/AccommodationBooking/IndexController.php @@ -6,7 +6,6 @@ namespace App\Controller\Admin\AccommodationBooking; use App\Controller\Traits\ListFilterTrait; use App\Entity\User; -use App\Form\Admin\Filter\AccommodationBookingFilterOptionsProvider; use App\Form\Admin\Filter\AccommodationBookingFilterType; use App\Form\Model\Filter\AccommodationBookingFilterDto; use App\Repository\Groups\AccommodationBookingRepository; @@ -24,7 +23,6 @@ class IndexController extends AbstractController public function __construct( private readonly AccommodationBookingRepository $bookingRepository, - private readonly AccommodationBookingFilterOptionsProvider $filterOptions, private readonly PaginatorInterface $paginator, ) { } @@ -34,30 +32,21 @@ class IndexController extends AbstractController { // ROLE_GROUPS_ADMIN is granted explicitly since 6c1073e4 removed it from ROLE_ADMIN's // role_hierarchy — an administrator who needs this has to hold it in their own right. - $seesAllBookings = $this->isGranted('ROLE_GROUPS_ADMIN'); + $isGroupsAdmin = $this->isGranted('ROLE_GROUPS_ADMIN'); $user = $this->getUser(); $user = $user instanceof User ? $user : null; // The form writes into $filter, so after this call it holds either the defaults or // whatever the query string asked for. - $filter = AccommodationBookingFilterDto::defaults($seesAllBookings, $user); + $filter = AccommodationBookingFilterDto::defaults($isGroupsAdmin, $user); $filterView = $this->createListFilterView( $request, AccommodationBookingFilterType::class, $filter, 'app_admin_accommodationbooking', - $this->filterOptions->formOptions($seesAllBookings), ); - // The manager fields are not part of the form for this role, but the scope is pinned - // here as well so that a hand-written query parameter cannot widen it either. - if (!$seesAllBookings) { - $filter->managedBy = $user; - $filter->unassigned = false; - $filter->managedByLocked = true; - } - $pagination = $this->paginator->paginate( $this->bookingRepository->createFilteredQueryBuilder($filter), $request->query->getInt('page', 1), diff --git a/src/Form/Admin/Filter/AccommodationBookingFilterOptionsProvider.php b/src/Form/Admin/Filter/AccommodationBookingFilterOptionsProvider.php deleted file mode 100644 index 7953abc..0000000 --- a/src/Form/Admin/Filter/AccommodationBookingFilterOptionsProvider.php +++ /dev/null @@ -1,64 +0,0 @@ - - */ - public function formOptions(bool $seesAllBookings): array - { - return [ - 'can_filter_by_manager' => $seesAllBookings, - 'managers' => $seesAllBookings ? $this->managers() : [], - 'accommodations' => $this->accommodationRepository->findBy([], ['name' => 'ASC']), - ]; - } - - /** - * Everyone worth filtering by: the staff a booking can be assigned to, plus whoever is - * currently assigned to one. - * - * The second half matters because the two lists drift apart — somebody assigned last season - * may have lost the groups role since, and their bookings would otherwise be impossible to - * find. The edit form keeps the current value in its choices for the same reason. - * - * @return User[] - */ - private function managers(): array - { - $managers = []; - - foreach ([...$this->userRepository->findGroupsStaff(), ...$this->bookingRepository->findAssignedManagers()] as $manager) { - $managers[(int) $manager->getId()] = $manager; - } - - uasort($managers, static fn (User $a, User $b) => strcasecmp($a->getDisplayName(), $b->getDisplayName())); - - return array_values($managers); - } -} diff --git a/src/Form/Admin/Filter/AccommodationBookingFilterType.php b/src/Form/Admin/Filter/AccommodationBookingFilterType.php index a6b0fa9..f8f5882 100644 --- a/src/Form/Admin/Filter/AccommodationBookingFilterType.php +++ b/src/Form/Admin/Filter/AccommodationBookingFilterType.php @@ -9,6 +9,8 @@ use App\Entity\User; use App\Enum\Groups\AccommodationBookingOrigin; use App\Enum\Groups\AccommodationBookingStatus; use App\Form\Model\Filter\AccommodationBookingFilterDto; +use App\Repository\Groups\AccommodationRepository; +use App\Repository\UserRepository; use Symfony\Bridge\Doctrine\Form\Type\EntityType; use Symfony\Component\Form\Extension\Core\Type\CheckboxType; use Symfony\Component\Form\Extension\Core\Type\EnumType; @@ -21,7 +23,7 @@ class AccommodationBookingFilterType extends AbstractListFilterType { protected function filterFields(array $options): array { - $fields = [ + return [ 'status' => [EnumType::class, [ 'label' => 'Status', 'class' => AccommodationBookingStatus::class, @@ -41,36 +43,28 @@ class AccommodationBookingFilterType extends AbstractListFilterType 'accommodation' => [EntityType::class, [ 'label' => 'Gruppenhaus', 'class' => Accommodation::class, - 'choices' => $options['accommodations'], + 'query_builder' => static fn (AccommodationRepository $er) => $er + ->createQueryBuilder('a') + ->orderBy('a.name', 'ASC'), 'choice_label' => 'name', 'placeholder' => 'alle', 'required' => false, ]], - ]; - - // Mirrors the edit form: only group admins get to look at other people's bookings, so - // for everyone else the fields simply do not exist and the scope is pinned server-side. - if (true === $options['can_filter_by_manager']) { // Triage does not depend on there being anyone to choose from, so this stays even // when nobody carries a groups role yet. - $fields['unassigned'] = [CheckboxType::class, [ + 'unassigned' => [CheckboxType::class, [ 'label' => 'nur ohne Zuordnung', 'required' => false, - ]]; - - if ([] !== $options['managers']) { - $fields['managedBy'] = [EntityType::class, [ - 'label' => 'Betreuer:in', - 'class' => User::class, - 'choices' => $options['managers'], - 'choice_label' => 'displayName', - 'placeholder' => 'alle', - 'required' => false, - ]]; - } - } - - return $fields; + ]], + 'managedBy' => [EntityType::class, [ + 'label' => 'Betreuer:in', + 'class' => User::class, + 'query_builder' => static fn (UserRepository $er) => $er->createAccommodationBookingManagersQueryBuilder(), + 'choice_label' => 'displayName', + 'placeholder' => 'alle', + 'required' => false, + ]], + ]; } public function configureOptions(OptionsResolver $resolver): void @@ -79,13 +73,7 @@ class AccommodationBookingFilterType extends AbstractListFilterType $resolver->setDefaults([ 'data_class' => AccommodationBookingFilterDto::class, - 'can_filter_by_manager' => false, - 'managers' => [], - 'accommodations' => [], ]); - $resolver->setAllowedTypes('can_filter_by_manager', 'bool'); - $resolver->setAllowedTypes('managers', User::class.'[]'); - $resolver->setAllowedTypes('accommodations', Accommodation::class.'[]'); } protected function searchPlaceholder(): string diff --git a/src/Form/Model/Filter/AccommodationBookingFilterDto.php b/src/Form/Model/Filter/AccommodationBookingFilterDto.php index 89f2416..d89f610 100644 --- a/src/Form/Model/Filter/AccommodationBookingFilterDto.php +++ b/src/Form/Model/Filter/AccommodationBookingFilterDto.php @@ -28,22 +28,16 @@ class AccommodationBookingFilterDto extends AbstractListFilterDto public ?Accommodation $accommodation = null; - /** - * Whether the manager scope is imposed rather than chosen. A group manager without admin - * rights only ever sees their own bookings, so the chip has to be shown — otherwise the - * short list looks like a bug — but must not offer a way out that would not work. - */ - public bool $managedByLocked = false; - /** * The list as it presents itself to someone who has not filtered yet. * * The still-live statuses — drafts, inquiries awaiting an offer, offers out with the * customer and bookings awaiting validation, i.e. everything that may still need work — * nothing whose stay is already over, and, for staff who are not group admins, only the - * bookings they are responsible for. + * bookings they are responsible for by default (they remain free to widen or change this + * through the filter, same as a group admin). */ - public static function defaults(bool $seesAllBookings, ?User $currentUser): self + public static function defaults(bool $isGroupsAdmin, ?User $currentUser): self { $filter = new self(); $filter->status = [ @@ -54,9 +48,8 @@ class AccommodationBookingFilterDto extends AbstractListFilterDto ]; $filter->dateFrom = new \DateTimeImmutable('today'); - if (!$seesAllBookings) { + if (!$isGroupsAdmin) { $filter->managedBy = $currentUser; - $filter->managedByLocked = true; } return $filter; @@ -85,11 +78,7 @@ class AccommodationBookingFilterDto extends AbstractListFilterDto if ($this->unassigned) { $chips[] = new ListFilterChip('Betreuer:in', 'keine Zuordnung', ['unassigned']); } elseif (null !== $this->managedBy) { - $chips[] = new ListFilterChip( - 'Betreuer:in', - $this->managedBy->getDisplayName(), - $this->managedByLocked ? [] : ['managedBy'], - ); + $chips[] = new ListFilterChip('Betreuer:in', $this->managedBy->getDisplayName(), ['managedBy']); } if (null !== $this->accommodation) { diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 818f948..c2e9355 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -4,10 +4,12 @@ declare(strict_types=1); namespace App\Repository; +use App\Entity\Groups\AccommodationBooking; use App\Entity\User; use App\Form\Model\Filter\UserFilterDto; use App\Repository\Filter\AppliesListFiltersTrait; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; +use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; use Doctrine\Persistence\ManagerRegistry; @@ -67,4 +69,23 @@ class UserRepository extends ServiceEntityRepository ->getQuery() ->getResult(); } + + /** + * Everyone worth filtering an accommodation booking by: current groups staff, plus whoever + * a booking is still assigned to even after losing the role — otherwise a booking assigned + * last season would become unfindable the moment its manager is deroled. + */ + public function createAccommodationBookingManagersQueryBuilder(): QueryBuilder + { + return $this->createQueryBuilder('u') + ->leftJoin(AccommodationBooking::class, 'booking', Join::ON, 'booking.managedBy = u') + ->andWhere('u.roles LIKE :groupsAdmin OR u.roles LIKE :groupsManager OR booking.id IS NOT NULL') + ->setParameter('groupsAdmin', '%"ROLE_GROUPS_ADMIN"%') + ->setParameter('groupsManager', '%"ROLE_GROUPS_MANAGER"%') + ->distinct() + ->orderBy('u.firstName') + ->addOrderBy('u.lastName') + ->addOrderBy('u.email') + ; + } } diff --git a/tests/Form/Admin/Filter/AccommodationBookingFilterOptionsProviderTest.php b/tests/Form/Admin/Filter/AccommodationBookingFilterOptionsProviderTest.php deleted file mode 100644 index cdc2253..0000000 --- a/tests/Form/Admin/Filter/AccommodationBookingFilterOptionsProviderTest.php +++ /dev/null @@ -1,97 +0,0 @@ -user(1, 'staff@example.test'); - $formerStaff = $this->user(2, 'former@example.test'); - - $options = $this->provider([$staff], [$formerStaff])->formOptions(true); - - self::assertSame( - ['former@example.test', 'staff@example.test'], - array_map(static fn (User $u) => $u->getEmail(), $options['managers']), - 'a booking assigned to someone who lost the groups role has to remain findable', - ); - } - - public function testSomebodyBothAssignedAndOnStaffIsOfferedOnce(): void - { - $manager = $this->user(1, 'manager@example.test'); - - $options = $this->provider([$manager], [$manager])->formOptions(true); - - self::assertCount(1, $options['managers']); - } - - public function testManagersAreSortedByEmail(): void - { - $options = $this->provider( - [$this->user(1, 'zoe@example.test'), $this->user(2, 'anna@example.test')], - [$this->user(3, 'Mia@example.test')], - )->formOptions(true); - - self::assertSame( - ['anna@example.test', 'Mia@example.test', 'zoe@example.test'], - array_map(static fn (User $u) => $u->getEmail(), $options['managers']), - ); - } - - public function testStaffWhoCannotSeeOtherBookingsGetNoManagerFilterAtAll(): void - { - $options = $this->provider([$this->user(1, 'staff@example.test')], [])->formOptions(false); - - self::assertFalse($options['can_filter_by_manager']); - self::assertSame([], $options['managers']); - } - - public function testTheManagerFilterIsAPermissionNotAConsequenceOfEmptyData(): void - { - // Nobody holds a groups role and nothing is assigned yet — a group admin still gets to - // filter, which is what keeps the "no assignment" triage available on day one. - $options = $this->provider([], [])->formOptions(true); - - self::assertTrue($options['can_filter_by_manager']); - self::assertSame([], $options['managers']); - } - - /** - * @param User[] $groupsStaff - * @param User[] $assigned - */ - private function provider(array $groupsStaff, array $assigned): AccommodationBookingFilterOptionsProvider - { - $users = $this->createMock(UserRepository::class); - $users->method('findGroupsStaff')->willReturn($groupsStaff); - - $bookings = $this->createMock(AccommodationBookingRepository::class); - $bookings->method('findAssignedManagers')->willReturn($assigned); - - $accommodations = $this->createMock(AccommodationRepository::class); - $accommodations->method('findBy')->willReturn([]); - - return new AccommodationBookingFilterOptionsProvider($users, $bookings, $accommodations); - } - - private function user(int $id, string $email): User - { - $user = new User($email); - - $idProperty = new \ReflectionProperty(User::class, 'id'); - $idProperty->setValue($user, $id); - - return $user; - } -} diff --git a/tests/Form/Model/Filter/AccommodationBookingFilterDtoTest.php b/tests/Form/Model/Filter/AccommodationBookingFilterDtoTest.php index 7709ca1..dcf3257 100644 --- a/tests/Form/Model/Filter/AccommodationBookingFilterDtoTest.php +++ b/tests/Form/Model/Filter/AccommodationBookingFilterDtoTest.php @@ -28,26 +28,24 @@ class AccommodationBookingFilterDtoTest extends TestCase ); self::assertSame((new \DateTimeImmutable('today'))->format('Y-m-d'), $filter->dateFrom?->format('Y-m-d')); self::assertNull($filter->managedBy, 'a group admin sees everybody’s bookings'); - self::assertFalse($filter->managedByLocked); } - public function testEverybodyElseStartsPinnedToTheirOwnBookings(): void + public function testEverybodyElseStartsFilteredToTheirOwnBookings(): void { $user = new User('manager@example.test'); $filter = AccommodationBookingFilterDto::defaults(false, $user); self::assertSame($user, $filter->managedBy); - self::assertTrue($filter->managedByLocked); } - public function testThePinnedManagerChipCannotBeDismissed(): void + public function testTheDefaultManagerChipCanBeDismissed(): void { $filter = AccommodationBookingFilterDto::defaults(false, new User('manager@example.test')); $chip = $this->chipFor($filter, 'Betreuer:in'); self::assertNotNull($chip); - self::assertFalse($chip->isRemovable()); + self::assertTrue($chip->isRemovable()); } public function testAChosenManagerChipCanBeDismissed(): void