Moving accounts package into /imports, adding email update to profile #2745
Conversation
/imports| sendUpdatedVerificationEmail(user._id); | ||
|
|
||
| // Sync users and accounts collections | ||
| syncUsersAndAccounts(); |
There was a problem hiding this comment.
I'm not sure this is the best way to sync users and accounts... any input would be appreciated
| * @param {String} email - user email | ||
| * @returns {Boolean} - returns boolean | ||
| */ | ||
| export function updateEmailAddress(email) { |
There was a problem hiding this comment.
After a bit of research, this seems to be the way people update email addresses on Meteor, first by adding a new one, then removing the old one. There is no callback on meteor's Accounts.addEmail, so I think you kind of have to send it back tot he client and do it this way.
|
bitHound - Dependencies — 1 failing dependency. |
|
|
||
| this.state = { | ||
| email: props.email, | ||
| newEmail: false, |
There was a problem hiding this comment.
this.state.newEmail isn't used anywhere.
| }; | ||
|
|
||
| this.handleFieldChange = this.handleFieldChange.bind(this); | ||
| this.handleSubmit = this.handleSubmit.bind(this); |
There was a problem hiding this comment.
These bindings aren't needed because you defined those as arrow functions (accomplishes the same thing).
|
|
||
| componentWillReceiveProps() { | ||
| this.setState({ showSpinner: false }); | ||
| } |
There was a problem hiding this comment.
Is this needed? I don't see how it does anything.
| const { email } = this.state; | ||
|
|
||
| Meteor.call("accounts/validation/email", email, false, (result, error) => { | ||
| if (error.error) { |
There was a problem hiding this comment.
All server communication or data fetching should always be in a container. This should just be passed in as an event handler prop. The UI component shouldn't need to know/care about how something gets done. It just handles the state of the UI.
|
|
||
| Meteor.call("accounts/updateEmailAddress", email, (err) => { | ||
| if (err) { | ||
| Alerts.toast(i18next.t("accountsUI.error.emailAlreadyExists", { err: error.message }), "error"); |
There was a problem hiding this comment.
Wrong error object here in the Alert. Should be err because error is from the upper scope.
| import { Reaction } from "/client/api"; | ||
| import { i18next } from "/client/api"; | ||
| import * as Collections from "/lib/collections"; | ||
| import UpdateEmail from "/imports/plugins/core/accounts/client/components/email/updateEmail"; |
There was a problem hiding this comment.
This should be a registered component so it can be overridden.
| */ | ||
| Template.accountProfile.helpers({ | ||
|
|
||
| UpdateEmail() { |
There was a problem hiding this comment.
In both of these, the data fetching logic should be in a container, not the Blaze template.
| import classnames from "classnames/dedupe"; | ||
| import { registerComponent } from "@reactioncommerce/reaction-components"; | ||
|
|
||
| class ReactionAvatar extends Component { |
There was a problem hiding this comment.
Any component that only has a render function should be a simple stateless function. None of the React lifecycle methods or state handling are being used, so a Component class adds unnecessary overhead.
This also depends on an email being passed in, so it should have a container wrapping it (currently being done by a Blaze template)
server/api/core/accounts/password.js
Outdated
| } | ||
|
|
||
| // make sure we have a valid address | ||
| if (!address || !_.includes(_.map(user.emails || [], "address"), address)) { |
There was a problem hiding this comment.
_.includes should be Array.includes (I know you likely copied this from the other email verification function that I wrote. Just trying to be do it right moving forward. :))
| * @param {String} email - user email | ||
| * @returns {Boolean} - returns boolean | ||
| */ | ||
| export function updateEmailAddress(email) { |
…e/reaction into ek-accountsPatch
|
@aaronjudd The |
…e/reaction into ek-accountsPatch
|
Would love to get this merged quickly please so we're able to pull the changes into a client project. |
Resolves issue #2740
Resolves issue #2493
Resolves issue #2778
Move some account related components from Move accounts package from
/client/modules/accountsinto/imports/plugins/core/accounts. Follow up task in #2746 .Add email update fields to profile page
Pull up
ReactionAvatarcomponent from client project into the coreFollow-up tasks:
Email copy for new email: #2778