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

Invites: Add invites create validation methods #3037

Merged
merged 11 commits into from
Feb 10, 2016

Conversation

lezama
Copy link
Contributor

@lezama lezama commented Feb 3, 2016

Closes #2844

  • Adds actions and stores for invites creation validation.
  • Hooks validation to "Usernames or Emails" tokens.

image

HOW TO TEST

return state.setIn( [ 'errors', action.siteId ], action.error );
}
return state;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this matches with the API. The validation endpoint doesn't return errors for usernames or emails that don't validate. The endpoint returns an object that looks a bit like:

{
  errors: [ WP_Error objects ],
  success: [ user1, useremail@test.com ]
}

@lezama
Copy link
Contributor Author

lezama commented Feb 3, 2016

😄 this is still in progress, I need to iterate on invites-create-validation.js that I copied from invites-accept-validation.js

@ebinnion
Copy link
Contributor

ebinnion commented Feb 3, 2016

Just giving early feedback 😜

@lezama lezama force-pushed the add/invites-create-validation-action-store branch from 6a8d1bd to da865c4 Compare February 4, 2016 15:58
@ebinnion
Copy link
Contributor

ebinnion commented Feb 4, 2016

#3058 is merged, which might be of interest if we want to go ahead and tie that UI into the validation that you're working on here.

@lezama lezama force-pushed the add/invites-create-validation-action-store branch 2 times, most recently from e6cbe34 to 552a050 Compare February 8, 2016 23:42
type: constants.action.RECEIVE_CREATE_INVITE_VALIDATION_SUCCESS,
siteId: siteId,
data: {
errors: [ 'asdfasdf' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

While this test works, I don't think it matches what we'll want from the API. On an error, we'll probably receive an array of objects, where each object contains the invitee as well as the error message. That currently looks like:

{
    "errors": {
        "test@gmail.com": {
            "errors": {
                "form-error-username-or-email": [
                    "User already has a role on your site."
                ]
            },
            "error_data": []
        }
    },
    "success": []
}

We could simplify errors a bit to be something like this perhaps:

errors: [
  {
    invitee: "test@gmail.com",
    error: "form-error-username-or-email",
    error_message: "User already has a role on your site."
  }
]

@ebinnion
Copy link
Contributor

ebinnion commented Feb 9, 2016

I left a couple of comments. I had issues with getting an error to show properly. When I attempted to invite someone that was already a user on my site, I got a green "successful" token instead of a red one.

Besides that, this feels pretty close.

@lezama
Copy link
Contributor Author

lezama commented Feb 9, 2016

When I attempted to invite someone that was already a user on my site, I got a green "successful" token instead of a red one

that's probably an API error 😞

@lezama lezama added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 9, 2016
@ebinnion ebinnion added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 9, 2016
@ebinnion ebinnion force-pushed the add/invites-create-validation-action-store branch from af646ef to df920c7 Compare February 9, 2016 23:06
@ebinnion
Copy link
Contributor

ebinnion commented Feb 9, 2016

I rebased the PR to fix a merge conflict and have updated the PR to fix an issue where validation was not happening because the site ID wasn't being sent to the API.

Of note, I also removed the is-success status when an invite is validated successfully. After a chat with @rickybanister it looks like we're going to just show the $gray-dark color for successful invites and $gray while validating. That is currently being worked on in #3195.

Note: Since I rebased against master, the new styles for isBorderless are now being shown. This also means that the error state is not currently visible. But, you can verify that the is-error class is being applied to the token.

@lezama – I think this is ready for another review. If you're fine with my changes, let's 🚢.

@ebinnion ebinnion added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 9, 2016
@ebinnion ebinnion force-pushed the add/invites-create-validation-action-store branch from 3b9857b to bb77af5 Compare February 9, 2016 23:29
lezama added a commit that referenced this pull request Feb 10, 2016
…n-action-store

Invites: Add invites create validation methods
@lezama lezama merged commit 5951c6f into master Feb 10, 2016
@lezama lezama deleted the add/invites-create-validation-action-store branch February 10, 2016 12:12
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants