[PASSWORD]: reset and forgot WIP #1231

snewcomer · 2017-04-05T03:21:31Z

References

joshsmith

Per some discussion outside this PR, we're going to limit the scope to unauthenticated users who forgot their password. This means there won't be anything in the user's settings, and everything can work without the authenticated route mixin.

joshsmith · 2017-04-06T01:00:06Z

app/routes/password/reset.js

+        method: 'POST',
+        data: {
+          password,
+          passwordConfirmation


Do we need to send the confirmation, as well, or can we just use that on the client-side? I don't believe we have a concept on the server of a confirmation.

@joshsmith so on the backend we do have a check validate_confirmation check

https://github.com/code-corps/code-corps-api/blob/develop/web/models/user.ex#L101

Oh, I missed that this was added. My mistake. Is it added to the user model in Ember here, too?

joshsmith · 2017-04-06T01:02:20Z

app/templates/components/password/forgot-password.hbs

+<div class="container">
+  <form>
+
+    {{input name="email" autocapitalize="off" type="text" placeholder='Your email' value=email key-down="keyDown"}}


Above this we can have a p that says:

Enter your email and we'll send you a link to reset your password

If the user's authenticated, this could even be auto-populated with the user's email!

Don't think keyDown is needed here.

joshsmith · 2017-04-06T01:03:15Z

app/templates/components/password/forgot-password.hbs

+    {{input name="email" autocapitalize="off" type="text" placeholder='Your email' value=email key-down="keyDown"}}
+
+    <div class="input-group">
+      <input class="button default {{if hasError "has-error"}}" name="forgot-password" type="submit" value='Forgot Password' {{action forgotPassword email}} />


Would this or the text input have the error?

I think so. If the ajax request hit a 500 or something...

I'm asking which one would have it. I think the button having it is a little weird, but the text having it makes more sense.

joshsmith · 2017-04-06T01:03:31Z

app/templates/components/password/reset-password.hbs

+  <form>
+    {{input autofocus="true" class="has-progress" name="password" type="password" placeholder='Enter new password' autocomplete="off" value=password}}
+
+    {{input class="has-progress" name="password" type="password" placeholder='Confirm password' autocomplete="off" value=passwordConfirmation}}


I wouldn't do placeholder on either of these.

This should probably have a label of "Confirm password".

I'm guessing this applies to forgot pwd as well?

Yeah, I don't think passwords should typically have a placeholder since it's a little confusing UX.

joshsmith · 2017-04-06T01:03:55Z

app/templates/components/password/reset-password.hbs

@@ -0,0 +1,15 @@
+<div class="container">
+  <form>
+    {{input autofocus="true" class="has-progress" name="password" type="password" placeholder='Enter new password' autocomplete="off" value=password}}


This should probably have a label of "Password".

joshsmith · 2017-04-06T01:04:20Z

app/templates/components/password/reset-password.hbs

+    {{input class="has-progress" name="password" type="password" placeholder='Confirm password' autocomplete="off" value=passwordConfirmation}}
+
+    <div class="input-group">
+      <input class="button default {{if hasError "has-error"}}" name="reset-password" type="submit" value='Reset Password' {{action resetPassword password passwordConfirmation}} />


Would change value to "Change password".

joshsmith · 2017-04-06T01:05:15Z

app/templates/password/reset.hbs

@@ -0,0 +1,3 @@
+{{password/reset-password 
+  resetPassword=(route-action "resetPassword")


I think route-action's are less testable in component tests, though you can correct me if I'm wrong on this. Also fairly certain that you don't need it here if the action is on the route, and similarly wonder if it shouldn't be in a controller.

So I believe route-action is for discoverability. If I change to action, it errors. I can create a controller for this action if you would like.

This is at a route level, so no need to generate a controller just for the sake of not using route-action, I think. The component is just as easily testable this way as it would be with a controller, but the boilerplate is reduced.

Also, thank you for that input, @snewcomer. I was in a mental place where I wasn't sure how route actions are useful exactly, since, in my experience, using them in nested components makes everything harder to test.

This example is where using a route action is useful for reducing boilerplate with no (to me) apparent negatives.

snewcomer · 2017-04-07T04:49:53Z

@joshsmith I'm guessing a Forgot Password link below the sign-in button? Text underline, smaller font, etc?

joshsmith · 2017-04-07T06:02:40Z

@snewcomer I think actually if we can do it just below the password field, that might be ideal. I'd like to have a "Don't have an account? Sign up." below the button on Sign In, and a "Have an account? Log in." below the button on Sign Up.

snewcomer · 2017-04-07T15:39:19Z

app/components/password/reset-password.js

+  error: null,
+
+  /**
+   * @property resetPasswordTask


@begedin Could you review this file? Was wondering what you thought of it as it follows a bit diff pattern than I have seen so far. Also, how should I handle errors here? This displays the error as you mentioned in the slack a while back. But have no way of getting rid of it.

Overall, it looks nice and clean. There isn't anything that seriously clashes with our usual approach.

snewcomer · 2017-04-07T15:39:41Z

tests/integration/components/password/reset-password-test.js

@@ -1,3 +1,5 @@
+import Ember from 'ember';
+const { RSVP } = Ember;
 import { moduleForComponent, test } from 'ember-qunit';
 import hbs from 'htmlbars-inline-precompile';



Here is the associated component test.

snewcomer

@joshsmith forgot password for your review. Once this is reviewed and g2g, will cleanup reset password w/ page objects and such and checkoff items in #1125

snewcomer · 2017-04-09T19:34:01Z

app/templates/components/login-form.hbs

@@ -9,6 +9,9 @@
  <div class="input-group">
    {{input name="password" id="password" placeholder="Password" type="password" value=password}}
  </div>
+  {{#link-to 'password.forgot' classNames="t-forgot-password"}}


@joshsmith this forgot-password link could use some styling when you get a chance.

snewcomer · 2017-04-09T19:34:30Z

app/templates/components/password/forgot-password.hbs

@@ -1,13 +1,13 @@
 <div class="container">
  <form>

-    <p>Enter your email and we'll send you a link to reset your password</p>
+    <p data-test-id="forgot-password-header">Enter your email and we'll send you a link to reset your password</p>


data test ids if you want to use them to target for tests. Thoughts?

We've been discussing the usage of https://github.com/simplabs/ember-test-selectors in the past, but I'm nut sure where we're with it now. The addon makes it so these test-only attributes are not present in the final build.

I think, let's not introduce it now, but what you could do is create an issue where we can discuss it and, if we opt into it, create a bunch of other issues. It's a tasks that would be very suitable for newbie volunteer.

begedin · 2017-04-10T07:36:06Z

app/components/password/reset-password.js

+   */
+  resetPasswordTask: task(function* (password, passwordConfirmation) {
+    try {
+      yield get(this, 'resetPassword')(password, passwordConfirmation);


Will get(this, 'resetPassword') actually return the function here? Isn't it defined under this.actions.resetPassword?

Also, this is where I would clear the error. Simply do a set(this, 'error', null) before the yield.

Yeah to it is passed in as closure action, so just need to get it off the props and call that function. This way also allows me to patch that action in the integration test.

snewcomer · 2017-04-10T18:04:52Z

app/components/password/reset-password.js

+   */
+  resetPasswordTask: task(function* (password, passwordConfirmation) {
+    try {
+      yield get(this, 'resetPassword')(password, passwordConfirmation);


Yeah to it is passed in as closure action, so just need to get it off the props and call that function. This way also allows me to patch that action in the integration test.

snewcomer · 2017-04-10T18:05:08Z

app/routes/password/reset.js

+          password,
+          passwordConfirmation
+        }
+      });


Need to handle success/error cases still.

begedin

Would love your comment on the merging of page objects. Otherwise looks great. Nicely written and well-tested.

begedin · 2017-04-12T08:27:40Z

tests/pages/password.js

+  form,
+  resetPasswordForm,
+  errorFormatter
+});


I believe a page object should match with a route 1 to 1. Otherwise, we risk confusing people. Is there any particular reason why you chose to merge the two password pages?

Split up, we would not have visitReset and visitForgot. The two pages would simply have a visit, a form and an errorFormatter.

Yeah I see your point. Well the components are based on the components and the password page b/c of the parent password route. Also because I have an acceptance/password-test.

@begedin ^^

snewcomer · 2017-04-13T21:33:54Z

@joshsmith all checks have passed. Probably a good idea to style some pieces here. I can tackle, but didn't know if you wanted to do that.

begedin · 2017-04-14T09:07:51Z

mirage/config.js

-    let passwordConfirmation = params.split('&')[1].split('=')[1];
+    let [first, second] = params.split('&');
+    let password = first.split('=')[1];
+    let passwordConfirmation = second.split('=')[1];


There should be a this._getAttrsForFormRequest() which returns a POJO containing the form data, doing this job for you. It's undocumented, but it is supported by mirage.

However, you are using an arrow function, so you do not have access to this.

I believe something like

this.post('/password/reset', function() { let { password, passwordConfirmation } = this._getAttrsForFormRequest(); // continue with the rest of your code here ));

ought to do the trick.

Awesome trick!

snewcomer · 2017-04-14T22:02:03Z

@joshsmith fyi this is failing only on phantom. That is why I put in PR #1253

snewcomer · 2017-05-08T15:10:10Z

app/routes/password/reset.js

+
+  ajax: service(),
+  session: service(),
+


One thing I need here is a model hook that extracts the token from params and used to send up with the POST request.

To get the same functionality you proposed by changing the route in API, you could do something like:

queryParams: { token: { refreshModel: true } }

If you add that code into the route, the params hash in the model hook will contain a token key.

The standard way to specify these query parameters which are not part of the dynamic route URL would be to specify them in the controller:

export default Controller.extend({ queryParams: ['token'] });

but the way I described above allows us to skip unnecessarily defining a controller file.

snewcomer · 2017-05-12T17:58:38Z

@joshsmith @begedin So I tried this exact same code out on a simple app I just spun up and it works. Just need the email created part.

snewcomer

Awesome! No real comments. I def forgot about the reset password url reflecting the token_id.

snewcomer · 2017-05-30T21:59:35Z

app/routes/password/reset.js

@@ -24,10 +24,10 @@ export default Route.extend({
        data: {
          token: get(this, 'controller.token'),
          password,
-          passwordConfirmation
+          "password-confirmation": passwordConfirmation


Curious about this change. Is this a stylistic thing? Just asking for future ref.

It's because most of the keys in JSON API are dasherized by default. There's some lack of consistency in some places at present that are not standard CRUD (like here), but would like to bring at least that consistency to bear here, even if the resource itself isn't a JSON API resource.

snewcomer · 2017-05-30T22:02:07Z

tests/acceptance/password-test.js


  andThen(() => {
-    assert.equal(currentURL(), '/password/reset');
+    assert.equal(currentURL(), '/password/reset?token=abc123');


ah yep, I forgot about those changes.

snewcomer · 2017-05-30T22:02:28Z

tests/acceptance/password-test.js

@@ -53,7 +59,9 @@ test('visiting /password/forgot', function(assert) {
    assert.equal(currentURL(), '/password/forgot');
  });

-  passwordPage.forgotPasswordForm.sendForgotPasswordSuccessfully('admin@gmail.com');
+  andThen(() => {


surprised this has to be wrapped in an andThen.

It's because the Mirage response is a promise and we don't know when it will return.

Outdated

joshsmith

Looks great. Merging as soon as build passes. Thanks for all the hard work on this @snewcomer! 👍

snewcomer added the feature label Apr 5, 2017

snewcomer self-assigned this Apr 5, 2017

snewcomer requested a review from joshsmith April 5, 2017 03:21

joshsmith reviewed Apr 6, 2017

View reviewed changes

joshsmith added the needs submitter response label Apr 6, 2017

snewcomer commented Apr 7, 2017

View reviewed changes

snewcomer commented Apr 9, 2017

View reviewed changes

begedin reviewed Apr 10, 2017

View reviewed changes

snewcomer mentioned this pull request Apr 10, 2017

Add a /reset-password route and component #1125

Closed

7 tasks

snewcomer commented Apr 10, 2017

View reviewed changes

begedin force-pushed the password-work branch from 6f1c21f to 870f13f Compare April 11, 2017 07:46

begedin requested changes Apr 12, 2017

View reviewed changes

begedin previously requested changes Apr 14, 2017

View reviewed changes

snewcomer commented May 8, 2017

View reviewed changes

snewcomer force-pushed the password-work branch from 4188036 to d1128e8 Compare May 11, 2017 17:18

snewcomer force-pushed the password-work branch 3 times, most recently from d6b23d0 to c335d4d Compare May 18, 2017 15:55

joshsmith force-pushed the password-work branch 2 times, most recently from fa5d7a6 to 21f22fc Compare May 30, 2017 21:52

snewcomer commented May 30, 2017

View reviewed changes

joshsmith force-pushed the password-work branch from 21f22fc to a2b549e Compare May 30, 2017 23:08

Add reset and forgot password functionality

a9c7c7c

joshsmith force-pushed the password-work branch from a2b549e to a9c7c7c Compare May 30, 2017 23:39

joshsmith approved these changes May 30, 2017

View reviewed changes

Add --silent to tests for CI

c8c2f77

joshsmith merged commit 0436aec into develop May 31, 2017

joshsmith deleted the password-work branch May 31, 2017 00:23

		@@ -0,0 +1,3 @@
		{{password/reset-password
		resetPassword=(route-action "resetPassword")

[PASSWORD]: reset and forgot WIP #1231

[PASSWORD]: reset and forgot WIP #1231

Conversation

snewcomer commented Apr 5, 2017

References

joshsmith left a 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

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

snewcomer commented Apr 7, 2017

joshsmith commented Apr 7, 2017

snewcomer Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

begedin Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer left a 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

begedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented May 12, 2017

snewcomer left a 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

joshsmith left a comment

Choose a reason for hiding this comment

snewcomer Apr 7, 2017 •

edited

Loading

begedin Apr 10, 2017 •

edited

Loading

snewcomer Apr 12, 2017 •

edited

Loading