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: Fix JS error caused from polling sites #5273

Merged
merged 1 commit into from
May 9, 2016

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented May 6, 2016

Today, I noticed that a JS error was occurring for logged out invites.
screen shot 2016-05-06 at 4 54 02 pm

To reproduce:

  • Go to /people/new/$site where $site is a WP.com site
  • Send an invite to a non-user
  • Click link in your email
  • Check console

After digging in, it looks like the error was coming from this line:

https://github.com/Automattic/wp-calypso/blob/master/client/layout/index.jsx#L62

Specifically, trying to add a poller to this.props.sites even when the user is logged out. To fix this, I simply removed the accept-invite path from the sites group.

I tested accepting invites afterward, and it worked. But frankly, I'm not sure if this will cause issues with chunking or anything else. I did notice several other sections did not have groups set, so my guess is no. All the same, pining @mtias who has a better idea.

To test:

  • Checkout fix/invites-poller-js-error branch
  • Go to /people/new/$site where $site is a WP.com site
  • Send an invite to a non-user
  • Get link from email and replace wordpress.com with calypso.localhost:3000
  • Check console

Error on production:
screen shot 2016-05-06 at 5 21 22 pm

No error with this branch in development:
screen shot 2016-05-06 at 5 21 25 pm

cc @lezama for code review.

@ebinnion ebinnion self-assigned this May 6, 2016
@ebinnion ebinnion added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. People Management Framework labels May 6, 2016
@mtias
Copy link
Member

mtias commented May 9, 2016

How does it work if you are logged in? The sites group highlights the "My Sites" item in the masterbar if you are logged in. Not sure if that's a requirement for this section.

@ebinnion
Copy link
Contributor Author

ebinnion commented May 9, 2016

Here is what a logged in invite looks like:

screen shot 2016-05-09 at 1 29 12 pm

The "My Sites" menu item isn't highlighted as you predicted. Frankly, since we don't show the "My Sites" sidebar, I would suggest we not highlight the "My Sites" menu item either. I'll ping @rickybanister to get some input there.

@rickybanister
Copy link

I don't think that tab needs to be highlighted at the stage. You're sort of in limbo until you choose to join or decline.

@mtias
Copy link
Member

mtias commented May 9, 2016

Sounds good to me :)

@mtias mtias 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 May 9, 2016
@ebinnion ebinnion merged commit 46592d3 into master May 9, 2016
@ebinnion ebinnion deleted the fix/invites-poller-js-error branch May 9, 2016 19:33
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