Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions includes/validation/class-amp-validation-error-taxonomy.php
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,9 @@ public static function register() {
'manage_terms' => AMP_Validation_Manager::VALIDATE_CAPABILITY, // Needed to give access to the term list table.
'delete_terms' => AMP_Validation_Manager::VALIDATE_CAPABILITY, // Needed so the checkbox (cb) table column will work.
'assign_terms' => 'do_not_allow', // Block assign_terms since associating terms with posts is done programmatically.
'edit_terms' => 'do_not_allow', // Terms are created (and updated) programmatically.
// Terms are created (and updated) programmatically, but we still need to assign capabilities while generating edit link for term.
// @see <https://github.com/ampproject/amp-wp/issues/7604#issuecomment-1704244763>.
'edit_terms' => AMP_Validation_Manager::VALIDATE_CAPABILITY,
Copy link
Member

Choose a reason for hiding this comment

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

Won't this then allow users to access the edit term page when they shouldn't be able to access it?

Copy link
Member

Choose a reason for hiding this comment

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

At the very least we'll also need to unset($actions['edit']) in:

public static function filter_tag_row_actions( $actions, WP_Term $tag ) {
global $pagenow;
$term_id = $tag->term_id;
$term = get_term( $term_id ); // We don't want filter=display given by $tag.
/*
* Hide deletion link since a validation error should only be removed once
* it no longer has an occurrence on the site. When a validated URL is re-checked
* and it no longer has this validation error, then the count will be decremented.
* When a validation error term no longer has a count, then it is hidden from the
* list table. A cron job could periodically delete terms that have no counts.
*/
unset( $actions['delete'] );
if ( 'post.php' === $pagenow ) {
$actions['details'] = sprintf(
'<button type="button" aria-label="%s" class="single-url-detail-toggle button-link">%s</button>',
esc_attr__( 'Toggle error details', 'amp' ),
esc_html__( 'Details', 'amp' )
);
$actions['copy'] = sprintf(
'<button type="button" class="single-url-detail-copy button-link" data-error-json="%s">%s</button>',
esc_attr( self::get_error_details_json( $term ) ),
esc_html__( 'Copy to clipboard', 'amp' )
);
} elseif ( 'edit-tags.php' === $pagenow ) {
$actions['details'] = sprintf(
'<a href="%s">%s</a>',
admin_url(
add_query_arg(
[
self::TAXONOMY_SLUG => $term->name,
'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG,
],
'edit.php'
)
),
esc_html__( 'Details', 'amp' )
);
if ( 0 === $term->count ) {
$actions['delete'] = sprintf(
'<a href="%s">%s</a>',
wp_nonce_url(
add_query_arg( array_merge( [ 'action' => 'delete' ], compact( 'term_id' ) ) ),
'delete'
),
esc_html__( 'Delete', 'amp' )
);
}
}
$actions = wp_array_slice_assoc( $actions, [ 'details', 'delete', 'copy' ] );
self::$current_validation_error_row_index++;
return $actions;
}

Then there's the question of the REST API. I suppose they can't access it because it's not public and show_in_rest is not true.

On the other hand, is this actually a bug that should rather be fixed in core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Won't this then allow users to access the edit term page when they shouldn't be able to access it?

We are not providing an edit link so it will be hard to generate the link with the correct tag_ID.

At the very least we'll also need to unset($actions['edit']) in:

I think it will do it.

$actions = wp_array_slice_assoc( $actions, [ 'details', 'delete', 'copy' ] );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the other hand, is this actually a bug that should rather be fixed in core?

Yes, and I have filed a ticket for that - https://core.trac.wordpress.org/ticket/59336

],
]
);
Expand Down Expand Up @@ -2923,10 +2925,19 @@ public static function handle_validation_error_update( $redirect_to, $action, $t
}
}

if ( $updated_count ) {
delete_transient( AMP_Validated_URL_Post_Type::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
}

if ( false !== $has_pre_term_description_filter ) {
add_filter( 'pre_term_description', 'wp_filter_kses', $has_pre_term_description_filter );
}

// Bail if redirect_is passed as null.
Copy link
Member

Choose a reason for hiding this comment

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

redirect_is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be $redirect_to. Will update it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 6d8a722

if ( null === $redirect_to ) {
return $redirect_to;
}

$term_ids_count = count( $term_ids );
if ( 'edit.php' === $pagenow && 1 === $updated_count ) {
// Redirect to error index screen if deleting an validation error with no associated validated URLs.
Expand All @@ -2947,10 +2958,6 @@ public static function handle_validation_error_update( $redirect_to, $action, $t
);
}

if ( $updated_count ) {
delete_transient( AMP_Validated_URL_Post_Type::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
}

return $redirect_to;
}

Expand Down
8 changes: 4 additions & 4 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
bootstrap="tests/php/bootstrap.php"
backupGlobals="false"
colors="true"
convertDeprecationsToExceptions="false"
convertErrorsToExceptions="false"
convertNoticesToExceptions="false"
convertWarningsToExceptions="false"
convertDeprecationsToExceptions="true"
convertErrorsToExceptions="true"
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
defaultTestSuite="default"
>
<php>
Expand Down
2 changes: 1 addition & 1 deletion src/Admin/OnboardingWizardSubmenu.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static function get_registration_action() {
*/
public function register() {
add_submenu_page(
'',
'options.php',
Copy link
Member

Choose a reason for hiding this comment

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

Linking your pervious #7614 (comment) here.

This was made an empty string in e30168c.

You say:

But adding options.php provides the same behavior without breaking anything. So updating it to options.php.

Can you clarify further what this is doing? Is it that there is no menu item for options.php so it won't add anything to the menu?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. It won't be accessible from a submenu item but rather only accessible via the URL which is options.php?page=amp-onboarding-wizard.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or if we want to hook it to amp-options page and keep it hidden in the submenu items then we need to only register it when it's called via URL - e0ce678

__( 'AMP Onboarding Wizard', 'amp' ),
__( 'AMP Onboarding Wizard', 'amp' ),
'manage_options',
Expand Down
2 changes: 1 addition & 1 deletion tests/php/src/Admin/OnboardingWizardSubmenuTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,6 @@ public function test_register() {

$this->instance->register();

$this->assertEquals( end( $submenu[''] )[2], 'amp-onboarding-wizard' );
$this->assertEquals( end( $submenu['options.php'] )[2], 'amp-onboarding-wizard' );
}
}
7 changes: 7 additions & 0 deletions tests/php/src/Admin/OptionsMenuTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,13 @@ public function test_render_screen_for_admin_user() {
)
);

// Set current screen to be the options menu.
set_current_screen( $this->instance->screen_handle() );

// Set title to be used in the screen.
global $title;
$title = 'Test Title';
Copy link
Member

Choose a reason for hiding this comment

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

I see this is coming from 1e8de9a. It was needed due to a PHP warning during tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, while rendering the screen it requires the title, but the title is not being set anywhere.


ob_start();
$this->instance->render_screen();
$this->assertStringContainsString( '<div class="wrap">', ob_get_clean() );
Expand Down
26 changes: 21 additions & 5 deletions tests/php/validation/test-class-amp-validation-error-taxonomy.php
Copy link
Member

Choose a reason for hiding this comment

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

Is this commit needed to work around the issue in tests which you're fixing in core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which commit? I am unable to see any diff with this comment.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant 2c43d0d

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's different from that one. handle_single_url_page_bulk_and_inline_actions() uses get_edit_post_link() which requires edit_post capability in general. In test cases we were not setting up the user hence it was resulting into deprecation warnings.

Copy link
Member

Choose a reason for hiding this comment

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

What was the deprecation warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those were the same as passing null to add_query_arg()

Deprecated: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated in /tmp/wordpress/src/wp-includes/functions.php on line 1157

Deprecated: str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in /tmp/wordpress/src/wp-includes/functions.php on line 1164

Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use AmpProject\AmpWP\Services;
use AmpProject\AmpWP\Tests\Helpers\HandleValidation;
use AmpProject\AmpWP\Tests\TestCase;
use AmpProject\AmpWP\Tests\Helpers\MockAdminUser;

/**
* Tests for AMP_Validation_Error_Taxonomy class.
Expand All @@ -20,6 +21,7 @@
class Test_AMP_Validation_Error_Taxonomy extends TestCase {

use HandleValidation;
use MockAdminUser;

/**
* The tested class.
Expand Down Expand Up @@ -1429,27 +1431,41 @@ static function ( $redirect_url ) use ( &$redirected_url ) {
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $incorrect_post_type );
$this->assertEquals( get_term( $error_term->term_id )->term_group, $initial_accepted_status );

// Setup admin user which have edit_posts capability.
$this->mock_admin_user();

// Build the expected URL for the redirect.
$admin_post_url = admin_url( 'post.php' );
$redirect_query_args = [
'post' => $correct_post_type,
'action' => 'edit',
'amp_actioned' => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPT_ACTION,
'amp_actioned_count' => 1,
];

/*
* Although the post type is correct, this should not update the post accepted status to be 'accepted'.
* There should be a warning because wp_safe_redirect() should be called at the end of the tested method.
*/
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $correct_post_type );

$this->assertSame( '?action=edit&amp_actioned=amp_validation_error_accept&amp_actioned_count=1', $redirected_url );
$this->assertSame( add_query_arg( $redirect_query_args, $admin_post_url ), $redirected_url );
$this->assertEquals( get_term( $error_term->term_id )->term_group, AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS );

// When the action is to 'reject' the error, this should not update the status of the error to 'rejected'.
$_REQUEST['action'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION;
$_REQUEST['action'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION;
$redirect_query_args['amp_actioned'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION;
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $correct_post_type );

$this->assertSame( '?action=edit&amp_actioned=amp_validation_error_reject&amp_actioned_count=1', $redirected_url );
$this->assertSame( add_query_arg( $redirect_query_args, $admin_post_url ), $redirected_url );
$this->assertEquals( get_term( $error_term->term_id )->term_group, AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS );

// When the action is to 'delete' the error, this should delete the error.
$_REQUEST['action'] = 'delete';
$_REQUEST['action'] = 'delete';
$redirect_query_args['amp_actioned'] = 'delete';
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $correct_post_type );

$this->assertSame( '?action=edit&amp_actioned=delete&amp_actioned_count=1', $redirected_url );
$this->assertSame( add_query_arg( $redirect_query_args, $admin_post_url ), $redirected_url );
$this->assertEquals( null, get_term( $error_term->term_id ) );
}

Expand Down