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 : Update static function in get_modules() with use :: operator. #8178

Merged
merged 2 commits into from
Jan 30, 2018
Merged

Admin Page : Update static function in get_modules() with use :: operator. #8178

merged 2 commits into from
Jan 30, 2018

Conversation

Umangvaghela
Copy link
Contributor

Tested :

Fix minor changes in class-jetpack-admin.php get_modules().

@Umangvaghela Umangvaghela requested a review from a team as a code owner November 15, 2017 12:05
@Umangvaghela Umangvaghela changed the title fix minor changes fix call static function in get_modules() Nov 15, 2017
@jeherve jeherve added Admin Page React-powered dashboard under the Jetpack menu [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Janitorial labels Nov 15, 2017
@Umangvaghela Umangvaghela changed the title fix call static function in get_modules() Admin Page : Update static function in get_modules() with use :: operator. Nov 17, 2017
@Umangvaghela
Copy link
Contributor Author

@jeherve

Hello sir

Can you review this PR when you get a leisure time?

Thanks

@Umangvaghela
Copy link
Contributor Author

Umangvaghela commented Nov 24, 2017 •
edited
Loading

@oskosk

This changes is same as MY PR gallery: use scope operator in static function

Thanks

@oskosk
Copy link
Contributor

oskosk commented Nov 24, 2017

@Umangvaghela, please, we need to have "Testing Instructions" in PRs. We need to check that everything still works after the changes in every PR.

Writing Testing Instructions allow us to follow a series of steps to check that no regression is introduced... Sometimes small changes look great but they end up messing with other pieces.

Writing Testing instructions is easy. For example, if you update a file related to stats, just add something like this to your PR description:

#### Testing instructions

* Get to the Jetpack Dashboard
* Confirm that you get no errors in `debug.log` and that the stats chart shows fine

Also, you mentioned #8153 in your last comment. That's probably a typo.

I'm labeling this PR as "Needs Author Reply" and blocking it requesting that you add testing instructions before it gets reviewed. I'm really thankful for your contributions to this codebase but we've already mentioned this a few times.

oskosk
oskosk previously requested changes Nov 24, 2017
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Add Testing Instructions to the Description

@oskosk oskosk added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 24, 2017
@Umangvaghela
Copy link
Contributor Author

@oskosk

Thank you, sir, for your response.
I tested all functionality which is related to this function.

Testing instructions

Before testing:

  • Click Jetpack Dashboard and setting page and also create custom post type: portfolio and taxonomy.
  • Create an error.log file and no error found and all are working fine.

After Testing:

  • Go to the Jetpack Dashboard and setting page and also check the all custom post type and sharing functionality are working fine.
  • Get the same results to change the code.
  • Create an error.log file and no error found and all are working fine.

Existing code provides the same result as my changeable code. But I use the scop operator to call a static function.

Thank you, Sir, To conscious me. while I Add PR that time I definitely add testing instruction.

@oskosk oskosk removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 25, 2017
@oskosk oskosk dismissed their stale review November 25, 2017 14:58

Author added Testing instructions. Thank you!

@oskosk oskosk added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 25, 2017
@Automattic Automattic deleted a comment from Umangvaghela Dec 5, 2017
@Automattic Automattic deleted a comment from Umangvaghela Dec 11, 2017
zinigor
zinigor requested changes Jan 19, 2018
View reviewed changes
Copy link
Member

@zinigor zinigor 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, this makes sense, but I left one comment.

class.jetpack-admin.php Outdated
@@ -60,12 +60,13 @@ static function sort_requires_connection_last( $module1, $module2 ) {
// presentation like description, name, configuration url, etc.
function get_modules() {
include_once( JETPACK__PLUGIN_DIR . 'modules/module-info.php' );
$available_modules = $this->jetpack->get_available_modules();
$active_modules = $this->jetpack->get_active_modules();
$get_object_module = $this->jetpack;
Copy link
Member

Choose a reason for hiding this comment

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

Let's just call the method statically using the class name like it's done everywhere else: Jetpack::get_available_modules(). Thank you!

@zinigor zinigor added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 19, 2018
@Umangvaghela
Update class.jetpack-admin.php
1a74a4b 
@zinigor
I add all changes which is you suggest me. Please let me know if I miss some thing. Sir new changes  not tested so please test it when you get a chance.

Thanks
@Umangvaghela
Copy link
Contributor Author

@zinigor

I have added all changes which is you suggested me. Please let me know if i miss something.

Thanks

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 25, 2018
@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 25, 2018
@zinigor zinigor added this to the 5.8 milestone Jan 25, 2018
@dereksmart dereksmart merged commit f7fb20f into Automattic:master Jan 30, 2018
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zinigor zinigor zinigor approved these changes

@oskosk oskosk oskosk left review comments

Assignees
No one assigned
Labels
Admin Page React-powered dashboard under the Jetpack menu [Type] Janitorial
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.
6 participants
@Umangvaghela @oskosk @zinigor @jeherve @dereksmart @matticbot