fix order and update of user settings form

src/Controller/ProfileController.php

@@ -21,7 +21,6 @@ use App\Form\UserTeamsType;
 use App\Repository\TeamRepository;
 use App\Repository\TimesheetRepository;
 use App\Utils\LocaleSettings;
-use Doctrine\Common\Collections\ArrayCollection;
 use Sensio\Bundle\FrameworkExtraBundle\Configuration\Security;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\Form\FormInterface;
@@ -212,13 +211,6 @@ class ProfileController extends AbstractController
         $event = new PrepareUserEvent($profile);
         $this->dispatcher->dispatch($event);
 
-        /** @var \ArrayIterator $iterator */
-        $iterator = $profile->getPreferences()->getIterator();
-        $iterator->uasort(function (UserPreference $a, UserPreference $b) {
-            return ($a->getOrder() < $b->getOrder()) ? -1 : 1;
-        });
-        $profile->setPreferences(new ArrayCollection(iterator_to_array($iterator)));
-
         $original = [];
         foreach ($profile->getPreferences() as $preference) {
             $original[$preference->getName()] = $preference;
@@ -227,7 +219,8 @@ class ProfileController extends AbstractController
         $form = $this->createPreferencesForm($profile);
         $form->handleRequest($request);
 
-        if ($form->isSubmitted() && $form->isValid()) {
+        if ($form->isSubmitted()) {
+            if ($form->isValid()) {
                 $entityManager = $this->getDoctrine()->getManager();
                 $preferences = $profile->getPreferences();
 
@@ -261,14 +254,24 @@ class ProfileController extends AbstractController
                     '_locale' => $locale,
                     'username' => $profile->getUsername()
                 ]);
+            } else {
+                $this->flashError('action.update.error', ['%reason%' => 'Validation failed']);
+            }
         }
 
+        // prepare ordered preferences
         $sections = [];
 
+        /** @var \ArrayIterator $iterator */
+        $iterator = $profile->getPreferences()->getIterator();
+        $iterator->uasort(function (UserPreference $a, UserPreference $b) {
+            return ($a->getOrder() < $b->getOrder()) ? -1 : 1;
+        });
+
         /** @var UserPreference $pref */
-        foreach ($profile->getPreferences() as $pref) {
+        foreach ($iterator as $pref) {
             if ($pref->isEnabled()) {
-                $sections[$pref->getSection()] = $pref->getSection();
+                $sections[$pref->getSection()][] = $pref->getName();
             }
         }
 

src/Entity/User.php

@@ -336,7 +336,7 @@ class User extends BaseUser implements UserInterface
         }
 
         foreach ($this->preferences as $preference) {
-            if ($preference->getName() == $name) {
+            if ($preference->getName() === $name) {
                 return $preference;
             }
         }

src/Event/UserPreferenceEvent.php

@@ -14,7 +14,7 @@ use App\Entity\UserPreference;
 use Symfony\Contracts\EventDispatcher\Event;
 
 /**
- * This event should be used, if further user preferences should added dynamically
+ * This event should be used, if further user preferences should be added dynamically.
  */
 final class UserPreferenceEvent extends Event
 {
@@ -26,11 +26,11 @@ final class UserPreferenceEvent extends Event
     /**
      * @var User
      */
-    protected $user;
+    private $user;
     /**
      * @var UserPreference[]
      */
-    protected $preferences;
+    private $preferences = [];
 
     /**
      * @param User $user
@@ -44,9 +44,10 @@ final class UserPreferenceEvent extends Event
 
     /**
      * Do not set the preferences directly to the user object, but ONLY via addPreference()
+     *
      * @return User
      */
-    public function getUser()
+    public function getUser(): User
     {
         return $this->user;
     }
@@ -54,7 +55,7 @@ final class UserPreferenceEvent extends Event
     /**
      * @return UserPreference[]
      */
-    public function getPreferences()
+    public function getPreferences(): array
     {
         return $this->preferences;
     }

src/EventSubscriber/UserProfileSubscriber.php

@@ -17,31 +17,23 @@ use Symfony\Component\HttpKernel\Event\KernelEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 
-class UserProfileSubscriber implements EventSubscriberInterface
+final class UserProfileSubscriber implements EventSubscriberInterface
 {
     /**
      * @var EventDispatcherInterface
      */
-    protected $eventDispatcher;
+    private $eventDispatcher;
-
     /**
      * @var TokenStorageInterface
      */
-    protected $storage;
+    private $storage;
 
-    /**
-     * @param EventDispatcherInterface $dispatcher
-     * @param TokenStorageInterface $storage
-     */
     public function __construct(EventDispatcherInterface $dispatcher, TokenStorageInterface $storage)
     {
         $this->eventDispatcher = $dispatcher;
         $this->storage = $storage;
     }
 
-    /**
-     * @return array
-     */
     public static function getSubscribedEvents(): array
     {
         return [
@@ -49,10 +41,7 @@ class UserProfileSubscriber implements EventSubscriberInterface
         ];
     }
 
-    /**
+    public function prepareUserProfile(KernelEvent $event): void
-     * @param KernelEvent $event
-     */
-    public function prepareUserProfile(KernelEvent $event)
     {
         if (!$this->canHandleEvent($event)) {
             return;
@@ -65,11 +54,7 @@ class UserProfileSubscriber implements EventSubscriberInterface
         $this->eventDispatcher->dispatch($event);
     }
 
-    /**
+    private function canHandleEvent(KernelEvent $event): bool
-     * @param KernelEvent $event
-     * @return bool
-     */
-    protected function canHandleEvent(KernelEvent $event): bool
     {
         // Ignore sub-requests
         if (!$event->isMasterRequest()) {

src/Form/Type/UserPreferenceType.php

@@ -34,10 +34,6 @@ class UserPreferenceType extends AbstractType
         $this->translate = $translator;
     }
 
-    /**
-     * @param FormBuilderInterface $builder
-     * @param array $options
-     */
     public function buildForm(FormBuilderInterface $builder, array $options)
     {
         $builder->addEventListener(
@@ -56,7 +52,7 @@ class UserPreferenceType extends AbstractType
                 }
 
                 $required = true;
-                if (CheckboxType::class == $preference->getType()) {
+                if (CheckboxType::class === $preference->getType()) {
                     $required = false;
                 }
 

templates/user/form.html.twig

@@ -2,11 +2,11 @@
 
 {% block main %}
     {{ form_start(form) }}
-    {% for section, counter in sections %}
+    {% for section, entries in sections %}
         {% embed '@AdminLTE/Widgets/box-widget.html.twig' %}
             {% block box_body %}
                 {% for pref in form.children.preferences %}
-                    {% if pref.vars.data.section == section %}
+                    {% if pref.vars.data.name in entries %}
                         {{ form_row(pref) }}
                     {% endif %}
                 {% endfor %}

tests/Controller/ProfileControllerTest.php

@@ -410,10 +410,14 @@ class ProfileControllerTest extends ControllerBaseTest
         return [
             // assert that the user doesn't have the "hourly-rate_own_profile" permission
             [User::ROLE_USER, UserFixtures::USERNAME_USER, 82, 82, 'ar', null],
-            // admins are allowed to update their own hourly rate
+            // teamleads are allowed to update their own hourly rate, but not other peoples hourly rate
+            [User::ROLE_TEAMLEAD, UserFixtures::USERNAME_TEAMLEAD, 35, 37.5, 'ar', 19.54],
+            // admins are allowed to update their own hourly rate, but not other peoples hourly rate
             [User::ROLE_ADMIN, UserFixtures::USERNAME_ADMIN, 81, 37.5, 'ar', 19.54],
-            // admins are allowed to update other peoples hourly rate
+            // super-admins are allowed to update other peoples hourly rate
-            [User::ROLE_SUPER_ADMIN, UserFixtures::USERNAME_USER, 82, 37.5, 'en', 19.54],
+            [User::ROLE_SUPER_ADMIN, UserFixtures::USERNAME_ADMIN, 81, 37.5, 'en', 19.54],
+            // super-admins are allowed to update their own hourly rate
+            [User::ROLE_SUPER_ADMIN, UserFixtures::USERNAME_SUPER_ADMIN, 46, 37.5, 'ar', 19.54],
         ];
     }
 
@@ -431,21 +435,16 @@ class ProfileControllerTest extends ControllerBaseTest
         $this->assertEquals($hourlyRateOriginal, $user->getPreferenceValue(UserPreference::HOURLY_RATE));
         $this->assertNull($user->getPreferenceValue(UserPreference::INTERNAL_RATE));
         $this->assertNull($user->getPreferenceValue(UserPreference::SKIN));
-        $this->assertEquals(false, $user->getPreferenceValue('theme.collapsed_sidebar'));
-        $this->assertEquals('month', $user->getPreferenceValue('calendar.initial_view'));
 
         $form = $client->getCrawler()->filter('form[name=user_preferences_form]')->form();
         $client->submit($form, [
             'user_preferences_form' => [
                 'preferences' => [
-                    ['name' => UserPreference::HOURLY_RATE, 'value' => 37.5],
+                    0 => ['name' => UserPreference::HOURLY_RATE, 'value' => 37.5],
-                    ['name' => UserPreference::INTERNAL_RATE, 'value' => 19.54],
+                    1 => ['name' => UserPreference::INTERNAL_RATE, 'value' => 19.54],
-                    ['name' => 'timezone', 'value' => 'America/Creston'],
+                    2 => ['name' => UserPreference::TIMEZONE, 'value' => 'America/Creston'],
-                    ['name' => 'language', 'value' => 'ar'],
+                    3 => ['name' => UserPreference::LOCALE, 'value' => 'ar'],
-                    ['name' => UserPreference::SKIN, 'value' => 'blue'],
+                    4 => ['name' => UserPreference::SKIN, 'value' => 'blue'],
-                    ['name' => 'theme.layout', 'value' => 'fixed'],
-                    ['name' => 'theme.collapsed_sidebar', 'value' => true],
-                    ['name' => 'calendar.initial_view', 'value' => 'agendaDay'],
                 ]
             ]
         ]);
@@ -462,10 +461,11 @@ class ProfileControllerTest extends ControllerBaseTest
 
         $this->assertEquals($hourlyRate, $user->getPreferenceValue(UserPreference::HOURLY_RATE));
         $this->assertEquals($expectedInternalRate, $user->getPreferenceValue(UserPreference::INTERNAL_RATE));
-        $this->assertEquals('', $user->getPreferenceValue('America/Creston'));
+        $this->assertEquals('America/Creston', $user->getPreferenceValue(UserPreference::TIMEZONE));
-        $this->assertEquals('ar', $user->getPreferenceValue('language'));
+        $this->assertEquals('America/Creston', $user->getTimezone());
+        $this->assertEquals('ar', $user->getPreferenceValue(UserPreference::LOCALE));
+        $this->assertEquals('ar', $user->getLanguage());
+        $this->assertEquals('ar', $user->getLocale());
         $this->assertEquals('blue', $user->getPreferenceValue(UserPreference::SKIN));
-        $this->assertEquals(true, $user->getPreferenceValue('theme.collapsed_sidebar'));
-        $this->assertEquals('agendaDay', $user->getPreferenceValue('calendar.initial_view'));
     }
 }