Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user profile and change password form tags #6400

Merged
merged 26 commits into from
Dec 13, 2022

Conversation

jacksleight
Copy link
Contributor

@jacksleight jacksleight commented Jul 29, 2022

Continuing with what @edalzell started in #6023, merging in what I've implemented in my addon.

This PR adds the following:

  • Adds a {{ user:profile_form }} tag that allows users to edit their information
    • Supports uploading assets in the profile form
  • Adds a {{ user:password_form }} tag that allows users to change their password
  • Adds support for uploading assets in the register form
  • Tests

Couple of notes:

  1. Profile Form Defaults
    I'm sure the way I've passed the default values from the user data to the profile form (including the change to getRenderableField) probably isn't the best way to do that, but I wasn't really sure how else to do it.
  2. Assets
    At the moment it's not possible for a user to remove assets from their accounts through the profile form, only replace them. I'm not sure if that should be supported and if so how best to do it?
  3. Protected Fields
    In my addon I implemented the profile form with a list of fields that were allowed to be submitted, defined via a config option. I've not implemented that here as the registration form doesn't have that. But perhaps it would be a good idea, to stop people submitting values they shouldn't have the ability to change? Maybe a block list instead of an allow list?

Also the tests have to check for either validation.current_password or The password is incorrect. because there's no validation.current_password string when running prefer-lowest.

Closes statamic/ideas#676.

@jacksleight jacksleight marked this pull request as ready for review August 10, 2022 17:04
@edalzell
Copy link
Contributor

Great work on this Jack

@j3ll3yfi5h
Copy link
Contributor

Hi @jacksleight! Thanks for your work! Any ideas, what's missing or why it is still not merged?

@jacksleight
Copy link
Contributor Author

jacksleight commented Oct 15, 2022

Think they just haven’t got to it yet, sure it'll be reviewed in due course. It's a fairly big PR so might take some time.

@jasonvarga
Copy link
Member

This is really great, thank you.

But the asset fields are complicated. As it stands, if you have an avatar, and you submit the form, the avatar field gets wiped.

So while you said:

At the moment it's not possible for a user to remove assets from their accounts through the profile form, only replace them

It is possible... It just does it when you probably don't want it to.
There's actually no way to maintain the value.

For now, so this can get merged and used, I'm going to filter out asset fields.

We can figure out asset field handling in a separate PR.

@jacksleight
Copy link
Contributor Author

I could be wrong (can't test it right now) but I think that's accounted for by these two bits:

request()->hasFile($field->handle())
->only(array_keys($values))

These should prevent blank files overwriting the existing values by filtering out the item from the collection if nothing has been uploaded.

Will do some testing to check though, I'm fairly sure that works OK in the addon (where most of this comes from).

@jasonvarga
Copy link
Member

It wiped it when trying here. That hasFile only happens in the uploadAssetFiles, but not normalizeAssetValues, so the null value still gets submitted.

There's also the issue of how to display the file when one already exists, how to let a user remove it, and what to do when you have other assets fields that may have max_files != 1.

@what-the-diff
Copy link

what-the-diff bot commented Dec 1, 2022

  • Added profile and password routes
  • Added user:profile_form, user:password_form tags
  • Updated UserController to handle the new forms (and added a few helper methods)
  • Removed asset fields from registration form values so they don't get validated as required when not present in request data
  • Added a new test file
  • Created tests for the user:password_form tag
  • Added a new test file
  • Created the class ProfileFormTest and added use statements for NormalizesHtml, PreventSavingStacheItemsToDisk, TestCase traits
  • Used trait to prevent saving stache items to disk in tests/PreventSavingStacheItemsToDisk.php
  • Wrote it_renders_form() method that asserts if form is rendered with correct attributes like action url etc.. using assertStringContainsString(), assertStringEndsWith() methods of PHPUnit\Framework\AssertionFailedError class which extends from Exception Class (PHPUnit)
  • Wrote it_renders_form_with params() method that asserts if form is rendered with redirect attribute value as /submitted and error-redirect attribute value as /errors using assertEquals($expectedRedirectUrl, $actualRedirectUrl),assertEquals($expectedErrorRedirectUrl ,$actualErrorRedirctURL) methods of PHPUnit\Framework\AssertionFailedError class which extends from Exception Class (PHPUnit). Also used Parse::template(string $content = '', array $data = []) static function provided by Statamic Facade to parse template tags within content string passed into this function

@jacksleight
Copy link
Contributor Author

Ah OK, must not have spotted that before, no worries. 👍 And yeah there are other issues to consider as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back {{ member:profile_form }} from v2
4 participants