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

Issue/1091 Fix Simplify give_is_admin_page conditional #2639

Merged
merged 19 commits into from
Jan 16, 2018
Merged

Issue/1091 Fix Simplify give_is_admin_page conditional #2639

merged 19 commits into from
Jan 16, 2018

Conversation

emgk
Copy link
Contributor

@emgk emgk commented Jan 9, 2018 •
edited
Loading

Description

This PR fixes #1091

How Has This Been Tested?

  • By calling function with params inside admin_init hook and compare the result with old function's code.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@ravinderk ravinderk self-requested a review January 9, 2018 15:53
@ravinderk ravinderk changed the base branch from release/2.0 to release/2.1 January 9, 2018 15:53
DevinWalker
DevinWalker reviewed Jan 10, 2018
View reviewed changes
includes/admin/admin-pages.php Outdated
if (
( 'give_forms' === $typenow || 'give_forms' === $query_args['post_type'] )
|| in_array( $passed_page, $expected_pages, true )
) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than nesting everything in this can you do the opposite condition and pass $found = false?
I would prefer less nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will be great, I am working on this. @DevinWalker

Copy link
Member

Choose a reason for hiding this comment

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

What's the status on this @emgk ?

DevinWalker
DevinWalker requested changes Jan 10, 2018
View reviewed changes
Copy link
Member

@DevinWalker DevinWalker left a comment

Choose a reason for hiding this comment

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

How many conditions and lines was this previous as compared to how it is now?

@emgk emgk changed the title Issue/1091 Issue/1091 Fix Simplify give_is_admin_page conditional Jan 10, 2018
@emgk
Copy link
Contributor Author

emgk commented Jan 11, 2018

@DevinWalker Previously the number of lines was 238 after optimizing the code the now number of lines is 95, lots of nesting conditional has been reduced, also used new function Give_Admin_Settings::is_setting_page fn to check setting/reports/addons pages.

@DevinWalker
Copy link
Member

Thanks @emgk it's looking much better. Can you now finish it up with a solid unit test written for give_is_admin_page() that passes both Give and non-Give pages and checks for proper true/false returns?

@emgk
Copy link
Contributor Author

emgk commented Jan 12, 2018

@DevinWalker I am working on PHPUnit testing.

@emgk
Copy link
Contributor Author

emgk commented Jan 16, 2018 •
edited
Loading

@DevinWalker @ravinderk I have added unit test cases for non-give and give pages.

@DevinWalker
Copy link
Member

Good job on this one @emgk 👍

@DevinWalker DevinWalker merged commit ad78acf into impress-org:release/2.1 Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DevinWalker DevinWalker DevinWalker approved these changes

@ravinderk ravinderk Awaiting requested review from ravinderk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emgk @DevinWalker @ravinderk