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

Create new form state for Enroll Account #55

Merged
merged 3 commits into from
Jan 7, 2017

Conversation

todda00
Copy link
Contributor

@todda00 todda00 commented Jun 7, 2016

Enroll account now has its own form state to allow more control over form labels and buttons, prompting user to set a password instead of changing a password.

Note: Ignore the reset and enroll hooks commit and revert, this change is in a separate PR.

@timbrandin
Copy link
Member

timbrandin commented Jun 9, 2016

This looks great, I will need to test this out so it works as expected, could you confirm these are the steps one need to take to validate this works? Taken from email flows in the guide:

Dependencies: accounts-password

  1. Create a user without a password using Accounts.createUser
  2. Trigger Accounts.sendEnrollmentEmail from the server for the user.
  3. User clicks the link in their email
  4. The LoginForm with formState: ENROLL_ACCOUNT shows up with the new label: setPassword which can is translated using softwarerero:accounts-t9n.

Also without accounts-password one should expect the form not to render a reset password or change password button. But that is another issue, but we could check that while testing this too.

@todda00
Copy link
Contributor Author

todda00 commented Jun 9, 2016

That is my exact workflow I used to produce / test this.

Probably would be good to test for accounts-password, but would be a very rare situation since Accounts.sendEnrollmentEmail is part of the accounts-password package, so a user would only get in that situation if an app had it installed, sent out links, then removed the package before the user clicked the link.

@shoetten
Copy link
Contributor

Hey @timbrandin,
i just saw you merged #58. Would you consider this PR for 1.2.10 as well?
This would finally allow me to get rid of my custom fork.

@todda00
Copy link
Contributor Author

todda00 commented Dec 28, 2016

I resolved the conflicts, should be ready to merge.

@timbrandin
Copy link
Member

I'm confused there's no changes in the files here, have I merged this already?

@shoetten
Copy link
Contributor

shoetten commented Jan 6, 2017

Well, this is strange indeed. But the functionality is definitely not in the code base, yet.
For example helpers.js should include the ENROLL_ACCOUNT: Symbol('ENROLL_ACCOUNT') state and LoginForm.jsx should say

this.setState({
  formState: STATES.ENROLL_ACCOUNT
});
Session.set(KEY_PREFIX + 'state', null);

after line 59.

Maybe something got mangled, when resolving the conflicts @todda00?

@todda00
Copy link
Contributor Author

todda00 commented Jan 6, 2017

Well, I probably messed it up somehow. I went ahead and manually re-applied the changes to the code in my branch, @timbrandin should be good to merge now.

@timbrandin timbrandin self-requested a review January 7, 2017 11:06
@timbrandin timbrandin merged commit a61a789 into studiointeract:master Jan 7, 2017
Copy link
Member

@timbrandin timbrandin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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.

3 participants