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: Do not show sign up for match email error to known user #2793

Merged
merged 1 commit into from
Jan 29, 2016

Conversation

roccotripaldi
Copy link
Member

If an invite was sent to a known user, and the site requires that all
invites be accepted with a matching email, there's no need to show a signup form.

Closes #2767

Prior to this PR we'd present this for invites to known users:
screen shot 2016-01-26 at 12 32 31 pm

But it doesn't make sense to ask a known user to sign up. So this PR will instead present this:
screen shot 2016-01-26 at 1 36 09 pm

To test:

  • Invite a known user to a blog that requires matching email addresses
  • As a logged out user, visit the accept invite page in calypso.localhost:3000
  • Ensure that the flow makes sense, and there are no errors
  • Repeat for an invite to an unknown user

@roccotripaldi
Copy link
Member Author

cc: @lezama and @ebinnion

@roccotripaldi roccotripaldi added People Management [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 26, 2016
@roccotripaldi roccotripaldi added this to the People Management: m6 milestone Jan 26, 2016
@roccotripaldi roccotripaldi self-assigned this Jan 26, 2016
@roccotripaldi
Copy link
Member Author

@rickybanister mentions that an error notice is not needed in this case

@roccotripaldi roccotripaldi 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 Jan 26, 2016
@roccotripaldi roccotripaldi force-pushed the update/logged-out-invite-known-user branch from 8a2f77c to 2eaaf0a Compare January 26, 2016 19:24
@roccotripaldi roccotripaldi 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 Jan 26, 2016
@ebinnion
Copy link
Contributor

Have you already addressed not showing the error notice for logged out invites to know users? I'm still seeing the notice, and can verify that the invite is to a known user since the button says, "Sign in as..."

logged-out-known

@ebinnion ebinnion added [Status] Needs Author Reply [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Author Reply labels Jan 26, 2016
@ebinnion
Copy link
Contributor

Disregard, I was actually logged in 😞 Code wise, this looks good to me. Let's just get a design review from @rickybanister. Here are updated screenshots:

  • Logged out match email error to a known user:

logged-out-known-error

  • Logged out match email error to an unknown user (note that the email input is disabled):

logged-out-unknown-error

  • Logged out no error:

logged-out-a-ok

@rickybanister
Copy link

The disabled email field still needs an italic note beneath it explaining why it's disabled/cannot be edited. Something like

The invitation can only be accepted by %email-address%

@roccotripaldi roccotripaldi force-pushed the update/logged-out-invite-known-user branch from 2eaaf0a to 6c4ca2c Compare January 27, 2016 04:15
@ebinnion
Copy link
Contributor

Screenshot for the italic note that @rickybanister suggested.

logged-out-force-input

@@ -6,6 +6,10 @@
max-height: 0;
overflow: hidden;
transition: all .3s ease-in-out;
&.no-validate {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we need a blank line above this.

@ebinnion
Copy link
Contributor

I left a couple of comments about the no validate style. Besides that, this LGTM.

@rickybanister
Copy link

Perfect @roccotripaldi ! Now we're talking.

@roccotripaldi roccotripaldi force-pushed the update/logged-out-invite-known-user branch from 6c4ca2c to c1b177c Compare January 28, 2016 19:02
@roccotripaldi
Copy link
Member Author

Addressed your feedback @ebinnion - thanks!

@roccotripaldi roccotripaldi added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Jan 28, 2016
@ebinnion
Copy link
Contributor

I noticed this error when going to a logged out invite to a known user. I'm looking into it now.

screen shot

@roccotripaldi
Copy link
Member Author

whoa, good catch

@ebinnion
Copy link
Contributor

@roccotripaldi – I pushed up a commit that I believe fixes that error. In my testing, the issue seemed to be that we update state after getting an invite and then right after that we update state to set matchEmailError. Because of this slight time difference, there was a period where matchEmailError was false and the SignupForm would be rendered in invite-accept-logged-out.

My commit refactors the refreshEmailMatchError() to return a boolean which we then use in invite-accept.

I tested both logged in and logged out flows for forcing email, and I didn't notice any regressions. If you're fine with my changes, then :shipit:

@roccotripaldi roccotripaldi force-pushed the update/logged-out-invite-known-user branch from dc5a820 to 968b2fb Compare January 28, 2016 22:16
@roccotripaldi
Copy link
Member Author

I was getting Uncaught TypeError: Cannot read property 'forceMatchingEmail' of undefined on you last commit. I amended it. should be good now.

@roccotripaldi roccotripaldi force-pushed the update/logged-out-invite-known-user branch from 968b2fb to d0d5c44 Compare January 28, 2016 22:59
};
console.log( props );
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this.

@ebinnion ebinnion force-pushed the update/logged-out-invite-known-user branch from d0d5c44 to cb438a2 Compare January 28, 2016 23:14
@ebinnion ebinnion added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 28, 2016
@ebinnion
Copy link
Contributor

@roccotripaldi - Thanks for fixing the error. I reviewed again and this LGTM.

@roccotripaldi roccotripaldi force-pushed the update/logged-out-invite-known-user branch from cb438a2 to c6d0781 Compare January 29, 2016 01:17
If an invite was sent to a known user, and the site requires that all
invites be accepted with a matching email, there's no need to show a signup form.

Invites: Fixes issue with stale matchEmailError in state
@roccotripaldi roccotripaldi force-pushed the update/logged-out-invite-known-user branch from c6d0781 to cdba7e9 Compare January 29, 2016 01:23
roccotripaldi added a commit that referenced this pull request Jan 29, 2016
…wn-user

Invites: Do not show sign up for match email error to known user
@roccotripaldi roccotripaldi merged commit 8076fd4 into master Jan 29, 2016
@ebinnion ebinnion deleted the update/logged-out-invite-known-user branch January 29, 2016 15:35
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.

4 participants