Skip to content

Conversation

@zinigor
Copy link
Contributor

@zinigor zinigor commented Dec 19, 2018

In order to synchronize code between Jetpack and WordPress.com better one of the little pieces we needed was the is_module_active method present in the Site Abstraction Layer. This change adds it as well as enforcing its existence in all extending classes.

Changes proposed in this Pull Request:

  • Adds unit tests that will run on SAL classes.
  • Adds the is_module_active method for SAL Sites.

Testing instructions:

  • Functionally there's nothing to change, just run the tests.
  • This should fail on wpcom check after being created, this is expected, because the method needs to be added on the other side as well.

Proposed changelog entry for your changes:

  • Added base unit test suites for the SAL library.

This blocks #10945

@matticbot
Copy link
Contributor

D22570-code. (newly created revision)

@jetpackbot
Copy link
Collaborator

jetpackbot commented Dec 19, 2018

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 6fcd58d

lezama
lezama previously approved these changes Dec 20, 2018
Copy link
Contributor

@lezama lezama left a comment

Choose a reason for hiding this comment

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

Thank you @zinigor 🙇

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 20, 2018
}

function is_module_active( $module ) {
if ( is_user_member_of_blog() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the membership check here?

@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 20, 2018
}

function get_jetpack_modules() {
if ( is_user_member_of_blog() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we remove the check from here we will want to add it in json-endpoints/class.wpcom-json-api-get-site-endpoint.php

@zinigor
Copy link
Contributor Author

zinigor commented Dec 21, 2018

This is already deployed on the other side. Thanks, Miguel!

@zinigor zinigor added the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 21, 2018
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

looks good. Merging.

@jeherve jeherve merged commit a17c5da into master Dec 21, 2018
@jeherve jeherve deleted the add/sal-unit-tests branch December 21, 2018 14:29
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 21, 2018
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.

7 participants