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

Jetpack Sites: Use active modules list added to /me/sites endpoint #1700

Merged
merged 3 commits into from
Dec 17, 2015
Merged

Jetpack Sites: Use active modules list added to /me/sites endpoint #1700

merged 3 commits into from
Dec 17, 2015

Conversation

lezama
Copy link
Contributor

@lezama lezama commented Dec 16, 2015

Use jetpack_modules list of active modules recently added to /me/sites endpoint, instead of fetching the list of modules on individual requests.

How to test

  • Check this branch
  • Test all the things!!!

cc @mtias @jkudish @aduth @enejb

@lezama lezama added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Jetpack Jetpack Plugins labels Dec 16, 2015
@lezama lezama self-assigned this Dec 16, 2015
@jkudish
Copy link
Contributor

jkudish commented Dec 16, 2015

Sweet! Confirming this fixes #1652! Though now I am getting #1653 (separate but related issue) on the sharing and editor screens with an author.

Played around with other parts of Calypso and this PR seems to be working well.

johnHackworth
johnHackworth reviewed Dec 16, 2015
View reviewed changes
client/lib/site/jetpack.js
JetpackSite.prototype.toggleModule = function( moduleId, callback ) {
const isActive = this.isModuleActive( moduleId ),
method = isActive ? 'jetpackModulesDeactivate' : 'jetpackModulesActivate',
prevActiveModules = Object.assign( {}, this.modules );
Copy link
Contributor

Choose a reason for hiding this comment

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

could we name this prevStateModules or something like that? if I'm not understanding it wrong, it saves the previous state of the modules to be able to restore them if there's an error, but the name suggest me a subset of the modules that includes only the active ones

Copy link
Contributor

Choose a reason for hiding this comment

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

(maybe I'm not getting it 100%)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.modules only contains active modules, maybe that's the one we should change

Copy link
Contributor

Choose a reason for hiding this comment

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

no, ok, I wasn't understand that... sounds good to me then 👍

@jkudish jkudish mentioned this pull request Dec 16, 2015
Merged
johnHackworth
johnHackworth reviewed Dec 16, 2015
View reviewed changes
client/lib/site/jetpack.js
if ( typeof module === 'string' ) {
module = this.getModule( module );
if ( isActive ) {
this.modules = this.modules.fiter( module => module === moduleId );
Copy link
Contributor

Choose a reason for hiding this comment

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

fiter... filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@lezama lezama force-pushed the update/active-modules branch from ad7d82f to c9f0b8f Compare December 16, 2015 22:08
@enejb
Copy link
Member

enejb commented Dec 16, 2015

I just a bit more testing and I wasn't able to activate and deactivate modules in sites security setting any more. The JS error that I get is this.modules.includes

to test go to http://calypso.localhost:3000/settings/security/example.com

@enejb enejb added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 16, 2015
@lezama
Copy link
Contributor Author

lezama commented Dec 16, 2015

Thanks for the exhaustive testing @enejb! I was able to get the error once, but now I can't reproduce it anymore ¯_(ツ)_/¯

@lezama lezama force-pushed the update/active-modules branch from c9f0b8f to 68f9027 Compare December 17, 2015 12:48
@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] Needs Author Reply labels Dec 17, 2015
@lezama
Copy link
Contributor Author

lezama commented Dec 17, 2015

@enejb I was able to reproduce the error, I was filtering the active modules list the wrong way. It's fixed now.

@johnHackworth
Copy link
Contributor

Works great!! And the feeling of having that data right away, without having to fetch it and have content screen and then a manage disabled error (for example) is awesome! \o/

🚢

@johnHackworth johnHackworth 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 Dec 17, 2015
lezama added a commit that referenced this pull request Dec 17, 2015
@lezama
Merge pull request #1700 from Automattic/update/active-modules
ad5a392 
Jetpack Sites: Use active modules list added to /me/sites endpoint
@lezama lezama merged commit ad5a392 into master Dec 17, 2015
@lezama lezama deleted the update/active-modules branch December 17, 2015 14:10
aduth
aduth reviewed Dec 17, 2015
View reviewed changes
client/post-editor/media-modal/index.jsx
@@ -63,11 +63,6 @@ module.exports = React.createClass( {
MediaActions.setLibrarySelectedItems( nextProps.site.ID, [] );
}

if ( nextProps.site && nextProps.site.jetpack && ! nextProps.site.modulesFetched &&
( ! this.props.site || this.props.site.ID !== nextProps.site.ID ) ) {
nextProps.site.fetchModules();
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me happy to see removed 😃

@aduth
Copy link
Contributor

aduth commented Dec 17, 2015

Does this fix #1425 ?

@mtias
Copy link
Member

mtias commented Dec 17, 2015

It does — single render sidebar finally :)

@mtias mtias mentioned this pull request Dec 17, 2015
Closed
@johnHackworth
Copy link
Contributor

it also fixes a lot of ugly content flashes in jetpack sites with manage turned off ... this is truly a great update! \o/

drewblaisdell
drewblaisdell reviewed Dec 17, 2015
View reviewed changes
client/my-sites/plugins/main.jsx
if ( selectedSite &&
selectedSite.modulesFetched &&
! selectedSite.canManage() ) {
if ( selectedSite && ! selectedSite.canManage() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to note somewhere: canManage only exists on Jetpack sites, so we need to either check selectedSite.jetpack before calling it, or add the same method to a .com site in a way that makes sense. I'm not sure what the best solution is, but we pushed an update to check if the site is a Jetpack site before calling this to fix it in the meantime.

@jkudish jkudish mentioned this pull request Dec 17, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lezama lezama

Labels
Jetpack
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lezama @jkudish @enejb @johnHackworth @aduth @mtias @drewblaisdell @scruffian