Add ability to edit user's informations and password #834

ker0x · 2018-07-17T23:30:09Z

As requested in #820

OlivierToussaint · 2018-07-18T07:10:00Z

src/Controller/UserController.php

+            $em->persist($user);
+            $em->flush();
+
+            return $this->redirectToRoute('security_logout');


I should remplace redirect to $this->redirectToRoute('user_index') and I add a flashbag message

I find it safer to logout the user after he has changed his password, but I can be wrong. I would like to get feedback from the community about that.

voronkovich · 2018-07-18T17:23:43Z

tests/Controller/UserControllerTest.php

+
+    public function testChangePassword()
+    {
+        $newUserPassword = $this->generateRandomString(10);


I think it's better to use a simple constant here. The test should be simple as possible.

$newUserPassword = 'new-password';

voronkovich · 2018-07-18T17:24:47Z

src/Controller/UserController.php

+    /**
+     * @Route("/", methods={"GET", "POST"}, name="user_index")
+     *
+     * @param \Symfony\Component\HttpFoundation\Request $request


This docblock comment is not needed.

voronkovich · 2018-07-18T17:25:03Z

src/Controller/UserController.php

+     *
+     * @param \Symfony\Component\HttpFoundation\Request $request
+     *
+     * @return \Symfony\Component\HttpFoundation\Response


This docblock comment is not needed too.

voronkovich · 2018-07-18T18:11:54Z

translations/messages.en.xlf

+            <trans-unit id="action.change_password">
+                <source>action.change_password</source>
+                <target>Change password</target>
+            </trans-unit>


The action.update_user_info is missing.

Actually, it was the translation key in the template file that was wrong.

yceruto · 2018-07-18T18:56:10Z

src/Form/Type/ChangePasswordType.php

+    public function configureOptions(OptionsResolver $resolver): void
+    {
+        $resolver->setDefaults([
+            'data_class' => null,


I think this config isn't necessary. If you aren't going to bind any data to this form, it's already null by default (source).

yceruto · 2018-07-18T19:01:37Z

src/Form/Type/ChangePasswordType.php

+            ->add('currentPassword', PasswordType::class, [
+                'constraints' => [
+                    new NotBlank(),
+                    new UserPassword(),


I think NotBlank constraint isn't necessary since a "Not blank" value is already verified in UserPassword constraint.

yceruto · 2018-07-18T19:30:40Z

src/Form/Type/ChangePasswordType.php

+                    new NotBlank(),
+                    new Length([
+                        'min' => 5,
+                    ]),


I also suggest adding a callback() constraint and check by BasePasswordEncoder::isPasswordTooLong($password) otherwise we could get an uncatched error BadCredentialsException in controller during password encoding.

That's not something that would happen in a real-life scenario unless really trying to make it fail. No sensible user would try setting a 4096+ chars long password. So I would not showcase it here or instead arbitrary set a reasonable max value in Length constraint above.

(relates to #834 (comment))

It's 72 chars for BCryptPasswordEncoder which is the current configured encoder :P but you're right, setting it (BCryptPasswordEncoder::MAX_PASSWORD_LENGTH) in max Length constraint is simple.

yceruto · 2018-07-18T19:34:41Z

src/Controller/UserController.php

+     * @Route("/password", methods={"GET", "POST"}, name="user_password")
+     *
+     * @param \Symfony\Component\HttpFoundation\Request                             $request
+     * @param \Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface $encoder


All these PHPDoc can be removed too.

ogizanagi · 2018-07-18T20:08:41Z

src/Controller/UserController.php

+            $user->setPassword($encoder->encodePassword($user, $form->get('newPassword')->getData()));
+
+            $em = $this->getDoctrine()->getManager();
+            $em->persist($user);


persist is unnecessary here as the entity is not a new one and already managed by doctrine.

ogizanagi · 2018-07-18T20:15:54Z

src/Form/Type/ChangePasswordType.php

+                    new NotBlank(),
+                    new Length([
+                        'min' => 5,
+                    ]),


That's not something that would happen in a real-life scenario unless really trying to make it fail. No sensible user would try setting a 4096+ chars long password. So I would not showcase it here or instead arbitrary set a reasonable max value in Length constraint above.

(relates to #834 (comment))

ogizanagi · 2018-07-18T20:20:14Z

src/Form/UserType.php

+
+        $builder
+            ->add('username', TextType::class, [
+                'attr' => ['readonly' => true],


setting the readonly attribute will not enforce this field cannot be submit and username not changed.
You must use disabled instead.

If I use disabled, I got an error Argument 1 passed to App\Entity\User::setUsername() must be of the type string, null given.

So, what should I do ?

leave readonly

add the nullable type to setUsername() in App\Entity\User

allow user to change his username

remove the username in UserType

@ker0x, He means a form option, not an HTML attribute:

$builder ->add('username', TextType::class, 'label' => 'label.username', 'disabled' => true, ])

Ho right, i'm so stupid! Thanks 👍

ogizanagi · 2018-07-19T06:12:11Z

src/Controller/UserController.php

+    /**
+     * @Route("/password", methods={"GET", "POST"}, name="user_password")
+     */
+    public function password(Request $request, UserPasswordEncoderInterface $encoder): Response


changePassword ? (same for the template name & route name)

I changed it for /profile/change-password

ogizanagi · 2018-07-19T06:13:23Z

src/Controller/UserController.php

+    /**
+     * @Route("/", methods={"GET", "POST"}, name="user_index")
+     */
+    public function index(Request $request): Response


profile / editProfile ? (same for the template name & route name)

yceruto · 2018-07-19T11:41:56Z

src/Form/UserType.php

+        $builder
+            ->add('username', TextType::class, [
+                'label' => 'label.username',
+                'disabled' => true,


If the intention is not to modify the username, how about removing this field and print {{ user.username }} directly in the template?

As this is a demo application, maybe we can keep the disabled option to show people how to disable an input in a form ?

yceruto · 2018-07-19T11:42:32Z

src/Controller/UserController.php

+            $user->setPassword($encoder->encodePassword($user, $form->get('newPassword')->getData()));
+
+            $em = $this->getDoctrine()->getManager();
+            $em->flush();


$this->getDoctrine()->getManager()->flush()?

Yep, already change 👍

yceruto · 2018-07-19T11:43:02Z

src/Controller/UserController.php

+/**
+ * Controller used to manage current user.
+ *
+ * @Route("/user")


/user-profile, /profile/user, /user-account, /account/user... ?

I changed it for /profile/edit

yceruto · 2018-07-19T13:17:30Z

templates/user/index.html.twig

+        {{ form_widget(form) }}
+
+        <button type="submit" class="{{ button_css|default("btn btn-primary") }}">
+            <i class="fa fa-save" aria-hidden="true"></i> {{ button_label|default('action.save'|trans) }}


same here {{ 'action.save'|trans }}

yceruto · 2018-07-19T13:17:33Z

templates/user/index.html.twig

+    {{ form_start(form, {attr: attr|default({})}) }}
+        {{ form_widget(form) }}
+
+        <button type="submit" class="{{ button_css|default("btn btn-primary") }}">


why not btn btn-primary directly? same for password.html.twig template.

yceruto · 2018-07-19T13:17:36Z

templates/user/index.html.twig

+{% block main %}
+    <h1>{{ 'title.edit_user'|trans }}</h1>
+
+    {{ form_start(form, {attr: attr|default({})}) }}


{attr: attr|default({})} can be removed.

…unnecessary form options

maxhelias

It seems good to me.
Nevertheless, I did not see if there was a check that the new password was different from the old one

bobdenotter · 2018-09-23T16:23:56Z

Is there a reason that it's App\Form\Type\ChangePasswordType and App\Form\UserType? Id think they should both be in App\Form\.

sebastianblum · 2018-09-26T19:32:10Z

@bobdenotter I think I can answer your question.
Root Types should be in App\Form
Child Types should be in App\Form\Type
so in my opinion this behavior is the current symfony best practise

javiereguiluz · 2018-10-04T13:15:46Z

@ker0x you did it really great. This is a ver well done feature. Thanks for working on this and thanks for your patience during the review. And thanks to all reviewers too!

OlivierToussaint approved these changes Jul 18, 2018

View reviewed changes

voronkovich suggested changes Jul 18, 2018

View reviewed changes

yceruto reviewed Jul 18, 2018

View reviewed changes

ogizanagi reviewed Jul 18, 2018

View reviewed changes

ker0x force-pushed the master branch 3 times, most recently from 1237860 to 19b0bb5 Compare July 19, 2018 00:25

Add ability to edit user's informations and password

9d1833d

ker0x force-pushed the master branch from 19b0bb5 to 9d1833d Compare July 19, 2018 00:28

ogizanagi reviewed Jul 19, 2018

View reviewed changes

yceruto reviewed Jul 19, 2018

View reviewed changes

Change methods, routes and templates name for UserController, remove …

d5b6f9b

…unnecessary form options

voronkovich approved these changes Jul 21, 2018

View reviewed changes

maxhelias approved these changes Jul 26, 2018

View reviewed changes

javiereguiluz added Feature FINISHED labels Oct 4, 2018

javiereguiluz closed this in 611fc54 Oct 4, 2018

javiereguiluz mentioned this pull request Oct 4, 2018

feature request #820

Closed

ker0x mentioned this pull request Feb 23, 2020

Change directory for ChangePasswordType #1083

Closed

Add ability to edit user's informations and password #834

Add ability to edit user's informations and password #834

Conversation

ker0x commented Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

ogizanagi Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ker0x Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxhelias left a comment

Choose a reason for hiding this comment

bobdenotter commented Sep 23, 2018

sebastianblum commented Sep 26, 2018

javiereguiluz commented Oct 4, 2018

ker0x commented Jul 17, 2018 •

edited

Loading

yceruto Jul 18, 2018 •

edited

Loading

ogizanagi Jul 18, 2018 •

edited

Loading

ogizanagi Jul 18, 2018 •

edited

Loading

ker0x Jul 19, 2018 •

edited

Loading

yceruto Jul 19, 2018 •

edited

Loading