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.

Sign up for GitHub

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

Jump to bottom

Admin Page: Get the connection URL from initial state instead of fetching it #8014

Merged
merged 5 commits into from
Oct 25, 2017
Merged

Admin Page: Get the connection URL from initial state instead of fetching it #8014

merged 5 commits into from
Oct 25, 2017

Conversation

oskosk
Copy link
Contributor

@oskosk oskosk commented Oct 18, 2017 •
edited
Loading

Makes the components that use the connection URL, get the data from the initial state instead of attempting to fetch.

Problem here is that any given output or error that any other plugin putos on the php output will make the user not be able to connect. This is a general problem, but it's critical for us to at least allow the user to connect.

Fixes #7979

Changes proposed in this Pull Request:

  • Makes the connection reducer react to JETPACK_SET_INITIAL_STATE by loading the state.jetpack.connectUrl with the data coming from the server via wp_localize_script.
  • Removes every usage of <QueryJetpackConnect /> because there's no more need for it.
  • Makes the promise chain in the disconnectSite() action also call fetchConnectUrl() after disconnection is successful.

Testing instructions:

  • Start with a disconnected site and user.
  • Visit the dashboard and confirm that the Connect button has an appropriate URL and that you can connect
  • After connecting, login in with a non-admin user and confirm that the banner button that reads "Connect to WordPress.com" works and has a good href.
  • Confirm that you can cycle the connection. Connect Jetpack, get back to the admin page. Disconnect, and try to connet again without refreshing the page.

@oskosk oskosk added [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Oct 18, 2017
@oskosk oskosk requested a review from a team as a code owner October 18, 2017 14:46
@oskosk oskosk force-pushed the update/connect-url-in-connect-button branch 2 times, most recently from 35df958 to 623b5cb Compare October 18, 2017 14:55
@oskosk oskosk force-pushed the update/connect-url-in-connect-button branch from 623b5cb to 11a7848 Compare October 18, 2017 15:11
@oskosk oskosk changed the title Admin Page: Get the connection URL from initial state instead of fetching in the JetpackConnect component Admin Page: Get the connection URL from initial state instead of fetching it Oct 18, 2017
@oskosk oskosk added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Oct 18, 2017
@oskosk oskosk added [Status] In Progress and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 18, 2017
@oskosk oskosk added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Oct 18, 2017
dereksmart
dereksmart approved these changes Oct 23, 2017
View reviewed changes
Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

The idea makes sense, and I couldn't seem to break anything when testing.

cc @ebinnion because I know you used to have some concerns/theories about the connect URLs were causing issues in some places.

@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 23, 2017
@ebinnion
Copy link
Contributor

Off of the top of my head, I couldn't recall specific issues.

After thinking about it for a bit, I did think of one specific flow where we could get in a bind:

  • Site is connected, so the build_connect_url() is the one that points to jetpack.wordpress.com
  • Site disconnects
  • User clicks connect link which sends them to jetpack.wordpress.com with old connect credentials

But, then I tested and that flow worked. 🤔

So, I read the PR again, and it looks like @oskosk handled that flow ( 🎉 ) with the following:

Makes the promise chain in the disconnectSite() action also call fetchConnectUrl() after disconnection is successful.

With that being said, this seems reasonable to me. 👍

@oskosk
Copy link
Contributor Author

oskosk commented Oct 25, 2017

Thanks for the review @ebinnion . Merging now

@oskosk oskosk merged commit 1db9c61 into master Oct 25, 2017
@oskosk oskosk deleted the update/connect-url-in-connect-button branch October 25, 2017 15:14
@oskosk oskosk removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 25, 2017
@jeherve jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Oct 26, 2017
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Oct 27, 2017
jeherve added a commit that referenced this pull request Oct 27, 2017
@jeherve
Changelog: add #8014
ad77454
anomiex added a commit that referenced this pull request May 10, 2022
@anomiex
jetpack: Remove unused JS client components
e58a88a 
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle
@anomiex anomiex mentioned this pull request May 10, 2022
Merged
anomiex added a commit that referenced this pull request May 10, 2022
@anomiex @kraftbj
jetpack: Remove unused JS client components (#24319)
d878bee 
Did a pass over _inc/client/components looking for things that weren't
used anywhere, and found a bunch:

* components/data/query-connect-url - Last use removed in #8014
* components/data/query-connection-status - Last use removed in 62e9ab0
* components/data/query-modules - Last use removed in bfc40ad
* components/data/query-plugin-updates - Last use removed in #17003
* components/data/query-site-products - Last use removed in #21594
* components/form/* - Didn't check for last use, too many bits. But it
  looks like the `formsy-react` package many of these depended on wasn't
  even installed since #8208.
* components/inline-expand - Last use removed in #6550
* components/jetpack-dialogue - Last use removed in #16518
* components/jetpack-logo - Last use removed in #20148
* components/jetpack-termination-dialog - Last use removed in #21048
* components/module-settings/index.jsx - Last use removed in #10644
* components/module-settings/inline-module-toggle.jsx - Last use removed
  in #12118
* components/screen-reader-text - Last use removed in #18843
* components/settings - Last use removed in 26315e1, I think
* components/tags-input - Last use removed in #11772

Then there were a few more that were only used from some of the above:

* components/data/query-connected-plugins
* components/module-settings/form-components.jsx
* components/multiple-choice-question
* components/setting-toggle

Co-authored-by: Brandon Kraft <public@brandonkraft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dereksmart dereksmart dereksmart approved these changes

@zinigor zinigor Awaiting requested review from zinigor

Assignees
No one assigned
Labels
[Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oskosk @ebinnion @dereksmart @jeherve