Skip to content

Conversation

eliorivero
Copy link
Contributor

@eliorivero eliorivero commented Feb 21, 2017

Fixes #6464
Fixes #6466 props @oskosk

Changes proposed in this Pull Request:

  • remove usage of window.Initial_State.
  • if non admin users unlink their accounts, the view is refreshed.
  • if site is not connected and user can't do it, show notice prompting user to ask an admin to connect and don't show link button.

captura de pantalla 2017-02-23 a las 18 49 07

Testing instructions:

  • Disconnect site and log with non admin: it should display a notice prompting to ask admin to connect the site.
  • Disconnect site, the view should be refreshed

@eliorivero eliorivero added Admin Page React-powered dashboard under the Jetpack menu [Status] In Progress [Type] Bug When a feature is broken and / or not performing as intended labels Feb 21, 2017
@eliorivero eliorivero added this to the Settings UI milestone Feb 21, 2017
@eliorivero eliorivero self-assigned this Feb 21, 2017
@eliorivero eliorivero added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Feb 21, 2017
@dereksmart
Copy link
Contributor

dereksmart commented Feb 21, 2017

Previously, the unlinked admin could view stats

This is very much intentional, and I would be hesitant (opposed) to require a connection to view the site stats within the wp-admin. This is the same for the rest of the content that we currently show (and showed before) in the prior UI and also in the wp-admin main dashboard widget.

Non-admins we can be more strict with, but still should make sure we're respecting the stats module options, since admins can choose which roles can see stats.

@eliorivero eliorivero force-pushed the fix/unlinked-admin-can-view-stats branch 2 times, most recently from 23ffb60 to 42f0852 Compare February 21, 2017 22:28
@eliorivero eliorivero added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Feb 21, 2017
@eliorivero eliorivero force-pushed the fix/unlinked-admin-can-view-stats branch 3 times, most recently from cbfd98b to 480c273 Compare February 22, 2017 00:05
@eliorivero eliorivero added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Feb 22, 2017
@eliorivero eliorivero force-pushed the fix/unlinked-admin-can-view-stats branch 2 times, most recently from 923dc06 to b9e2564 Compare February 22, 2017 00:28
@eliorivero
Copy link
Contributor Author

What Derek mentioned has been restored now.

@oskosk
Copy link
Contributor

oskosk commented Feb 22, 2017

@eliorivero @dereksmart I think the cause of your headache yesterday is the following

  1. With a non-admin linked user, after disconnecting, the <NonAdminView /> component does not update to reflect the unlinked state. The thing is that

<NonAdminView /> is getting isLinked={ true } always among its props passed by its parent (<Main /> )

screen shot 2017-02-22 at 9 02 27 am

Even when its parent <Main /> is getting a isLinked={ false } from redux state after unlinking the user.

screen shot 2017-02-22 at 9 02 35 am

The problem here is that we used the lifecycle method shouldComponentUpdate() in <Main />.
And this method only returns true on some particular conditions. Changing the link state for a user is not among those conditions.

screen shot 2017-02-22 at 9 02 44 am

What this causes is that even when <Main /> knows by its props that the user is now not linked, it doesn't re-render (because shouldComponentUpdate() returns false ) and thus, its children don't get the new props because they're not re-rendered.

Something like this could work:

image

I've just added a commit, including this condition to the shouldComponentUpdate() method and it looks like it's working as expected.

@eliorivero
Copy link
Contributor Author

eliorivero commented Feb 22, 2017

Gracias @oskosk for checking and fixing this! 👍 and the detailed explanation.

@dereksmart
Copy link
Contributor

When I enabled stats to be visible by Editors, I expected to be able to see them, but I'm only seeing a connect page

screen shot 2017-02-22 at 10 18 45 am

screen shot 2017-02-22 at 10 19 53 am

@dereksmart
Copy link
Contributor

dereksmart commented Feb 22, 2017

If we're showing a dedicated connect/link page for non-admins, shouldn't we be removing the features from the promotional stuff that includes things they can't activate/deactivate, or may not have access to? Like Photon, Security, Protect, Monitor, Manage, and maybe stats if they're not allowed to see them?

I think that would just leave "Free Support" 😂

@dereksmart dereksmart added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 23, 2017
@eliorivero eliorivero force-pushed the fix/unlinked-admin-can-view-stats branch from 69497cd to d3660bc Compare February 23, 2017 21:50
eliorivero and others added 2 commits February 23, 2017 18:54
…ce prompting user to ask an admin to connect. Don't show link button.
… changed.

The Main component implements a shouldComponentUpdate() lifecycle method
to check whether it should re-render under certain conditions. We were
not including the user linked state among these conditions.
@eliorivero eliorivero force-pushed the fix/unlinked-admin-can-view-stats branch from d3660bc to 18ed27c Compare February 23, 2017 21:54
@eliorivero eliorivero changed the title Settings UI: unlinked admin can view stats Settings UI - Non admin: refresh view after unlink. Don't show features if site disconnected Feb 23, 2017
@eliorivero
Copy link
Contributor Author

Updated this PR to do only 2 things: don't show features when site is disconnected (and display a warning prompting user to ask admin to connected) and refresh the view after non admin unlink.

@dereksmart
Copy link
Contributor

Tested, and works as expected, considering the latest changes:

don't show features when site is disconnected (and display a warning prompting user to ask admin to connected) and refresh the view after non admin unlink.

Code looks good. 🐑

@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 24, 2017
@eliorivero eliorivero merged commit b23c3a1 into feature/settings-overhaul Feb 24, 2017
@eliorivero eliorivero deleted the fix/unlinked-admin-can-view-stats branch February 24, 2017 15:50
@eliorivero eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants