Add functionality to reset page order #129

ruscoe · 2023-01-29T21:31:30Z

Description of the Change

In the page admin section, the Simple Page Ordering plugin help section has been modified to include a "Reset page order" link. Clicking this link first displays a confirmation dialog then, if the user confirms, resets the "menu_order" field of all pages to 0, resetting any changes made with this plugin.

I think this is the most minimally invasive solution, adding little code to achieve the purpose. I chose to place the reset link in the help menu to minimize chances of accidental order resets. Plus, if someone needs to reset the order of their posts, they have probably made a mistake and will be looking for help.

Closes #24

How to test the Change

Install and enable the Simple Page Ordering plugin
Navigate to the page admin section /wp-admin/edit.php?post_type=page
Add some pages
Drag some pages up or down so the order changes
Refresh the page to ensure the order remains (AJAX calls have been successful)
At the top of the page, click "Help", then "Simple Page Ordering", then "Reset page order"
Click "OK"
If the page refreshes and the pages now appear in their original order, this change is functioning properly

Changelog Entry

Added - Feature to reset page order

Credits

Props @pattonwebz, @ruscoe, @Sidsector9, @dkotter

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests pass.

This reverts commit 65b2d68.

Sidsector9

Hi @ruscoe

Thank you so much for working on this and congratulations on your first PR at 10up!

I have reviewed your code and I have a concern.

Since the REST endpoint in open to public, anyone with access to it can reset the page ordering.

Watch this video below where I am able to reset the ordering through Postman:

spo-129.mp4

For this feature, I would suggest you to use wp_ajax_{$action} instead.

You can use the following:

wp_nonce_url() to generate a URL with nonce which you can verify in the PHP callback using wp_verify_nonce().

OR

If you would like to go ahead with the current implementation, then you can pass a nonce and verify it in the permissions_callback.

ruscoe · 2023-01-30T04:37:35Z

@Sidsector9 Very good point! Thank you for reviewing and catching that. I'll take your suggestion and submit an updated PR shortly.

ruscoe · 2023-01-30T06:15:24Z

@Sidsector9 Changes made! It's now using the admin AJAX functionality rather than a REST endpoint. Is there something better I could do to avoid the code duplication between ajax_simple_page_ordering and the new ajax_reset_simple_page_ordering or is that as efficient as I can get it?

Sidsector9 · 2023-01-30T14:42:10Z

@ruscoe Thanks for working on those changes, tested well for me 👍

No pressure at all, but would you be comfortable at writing end-to-end tests for the same?
We use Cypress, you can see the tests for existing features here - https://github.com/10up/simple-page-ordering/tree/develop/tests/cypress/integration

Else let us know if you'd like someone from our team to pick up writing the tests.

Answering your question:

Is there something better I could do to avoid the code duplication between ajax_simple_page_ordering and the new ajax_reset_simple_page_ordering or is that as efficient as I can get it?

Looking at the code, I think we can let it pass for now as the duplication is less. In the future when the number of AJAX handlers increase using the same set of checks, then we can look at refactoring it under a common handler.

ruscoe · 2023-01-30T16:28:12Z

@Sidsector9 I'd like to write the tests. It'll be a great opportunity for me to learn how they work. Will update soon!

ruscoe · 2023-01-30T21:18:29Z

@Sidsector9 Thank you for all your assistance with this! I've just committed my first attempt at a Cypress test. I took a lot of guidance from the other tests.

The tests pass for me and look correct when I run them through the command line and UI / browser aspect of Cypress. Let me know what you think!

Sidsector9 · 2023-01-31T08:18:41Z

tests/cypress/integration/reset-page-ordering.test.js

+		const newSecondText = cy.get(`${second} .row-title`);
+
+		cy.get(`${first} .row-title`).then($el => {
+			newFirstText.should('have.text', $el.text());


I think this will always be true, as newFirstText and cy.get('${first} .row-title') refer to the same element.

We need to save the original post names so that we can verify their positions after swapping and reverting.
This is something I was trying today, I used cy.as() to store the text values and refer them later on for verification:

describe( 'Test Reset Page Order Change', () => { it( 'Can reset pages order', () => { cy.login(); cy.visit('/wp-admin/edit.php?post_type=page'); const firstRow = '.wp-list-table tbody tr:nth-child(1)'; const secondRow = '.wp-list-table tbody tr:nth-child(2)'; // Alias titles as `firstRowText` and `secondRowText` for convenience. cy.get( firstRow ).find( '.row-title' ).invoke( 'text' ).as( 'firstRowText' ); cy.get( secondRow ).find( '.row-title' ).invoke( 'text' ).as( 'secondRowText' ); // Swap position of `Page 1` with `Page 2`. cy.get( firstRow ).drag( secondRow ); // Verifies if 1st row has title `Page 2`. cy.get( firstRow ).find( '.row-title' ).invoke( 'text' ).then( function( text ) { expect( text ).to.eq( this.secondRowText ); } ); // Verifies if 2nd row has title `Page 1`. cy.get( secondRow ).find( '.row-title' ).invoke( 'text' ).then( function( text ) { expect( text ).to.eq( this.firstRowText ); } ); // Now reset the page order and verify original values are back. cy.get( '#contextual-help-link' ).click(); cy.get( '#tab-link-simple_page_ordering_help_tab' ).click(); cy.get( '#simple-page-ordering-reset' ).click(); cy.on( 'window:confirm', () => true ); // Perform a reload as Cypress won't after window:confirm. cy.reload(); // Verifies if 1st row has title `Page 1`. cy.get( firstRow ).find( '.row-title' ).invoke( 'text' ).then( function( text ) { expect( text ).to.eq( this.firstRowText ); } ); // Verifies if 2nd row has title `Page 2`. cy.get( secondRow ).find( '.row-title' ).invoke( 'text' ).then( function( text ) { expect( text ).to.eq( this.secondRowText ); } ); } ); } );

Sidsector9 · 2023-01-31T08:41:05Z

tests/cypress/integration/reset-page-ordering.test.js

+		cy.on(`window:confirm`, () => true);
+
+		// wait for the page to reload before checking for reset post order.
+		cy.wait(1000);


Cypress considers using cy.wait() as anti-pattern. We can replace this with cy.reload() instead because window.location.reload() will not actually reload the page in cypress.

Sidsector9 · 2023-01-31T08:52:42Z

@ruscoe Thanks so much for the tests! I have reviewed and requested some minor changes.
You can use npm run cypress:open for visual reference.

… original post titles after order reset.

ruscoe · 2023-01-31T17:09:49Z

@Sidsector9 Thank you! I replaced my test code with your modified code and it works perfectly. Do you mind if I just commit your modified test in place of mine?

Sidsector9 · 2023-02-13T08:52:15Z

simple-page-ordering.php

+			}
+
+			// reset the order of all posts of given post type
+			$wpdb->update( 'wp_posts', array( 'menu_order' => 0 ), array( 'post_type' => $post_type ) );


@dkotter This PR works well. The only area that I am concerned about is the performance impact due to this line.
Would this be an issue for an extremely large database? If so, should we look into performing the updates in batches?

Looking at the code wpdb::update runs, seems like the final query will be something like:

UPDATE 'wp_posts' SET 'menu_order' = 0 WHERE 'post_type' = 'page'

I think that should scale fairly well and we won't need to worry about doing any sort of batching here. That said, @ruscoe if you'd be able to do some quick testing on various sizes to see the performance impact, that would help us be more confident about merging this in. Something like profiling this code when run on a site that has 10 pages, 100 pages and 1000 pages, that would be helpful.

In addition, I think best practice here is to utilize the format properties of the update method. Something like

Suggested change

$wpdb->update( 'wp_posts', array( 'menu_order' => 0 ), array( 'post_type' => $post_type ) );

$wpdb->update( 'wp_posts', array( 'menu_order' => 0 ), array( 'post_type' => $post_type ), array( '%d' ), array( '%s' ) );

@dkotter No problem at all! I profiled by measuring execution time.

Just to show my process, first I used a quick shell script to create pages:

#!/bin/bash for i in {1..10} do wp post create --post_type=page --post_title="Page $i" done

Then used the following to test:

$start_time = microtime( true ); $wpdb->update( 'wp_posts', array( 'menu_order' => 0 ), array( 'post_type' => 'page' ), array( '%d' ), array( '%s' ) ); $end_time = microtime( true ); $total_time = ( $end_time - $start_time ); echo $total_time;

Taking the average of five test runs each:

10 posts: 0.0041 microseconds

100 posts: 0.0042 microseconds

1000 posts: 0.0116 microseconds

5000 posts: 0.0299 microseconds

How does that look?

Thanks @ruscoe! Definitely see an increase in time as the number of posts increase but the overall amount of time is so low that this doesn't seem like it will cause any problems. This looks good to go on my end

Co-authored-by: Darin Kotter <[email protected]>

Add functionality to reset page order via REST.

ad88c90

ruscoe requested a review from a team as a code owner January 29, 2023 21:31

ruscoe requested review from Sidsector9 and removed request for a team January 29, 2023 21:31

ruscoe added 3 commits January 29, 2023 20:17

Formatting fix.

65b2d68

Revert "Formatting fix."

0ed6d42

This reverts commit 65b2d68.

Formatting fix.

6c03c86

Sidsector9 requested changes Jan 30, 2023

View reviewed changes

Replace REST endpoint with admin AJAX solution.

3094a0b

ruscoe requested a review from Sidsector9 January 30, 2023 06:15

Add test for resetting page order.

f84e170

Sidsector9 assigned ruscoe Jan 31, 2023

Sidsector9 requested changes Jan 31, 2023

View reviewed changes

Update reset page order test to replace cy.wait and to properly check…

b3cf1c7

… original post titles after order reset.

Sidsector9 added 2 commits February 1, 2023 11:23

reset state after tests complete

e33dec6

fix coding standards and replace ajax referer with verify nonce

bd8a6fc

Sidsector9 force-pushed the 24-reset-ordering branch from 481a3fb to bd8a6fc Compare February 4, 2023 06:56

ruscoe requested a review from Sidsector9 February 8, 2023 05:37

Sidsector9 reviewed Feb 13, 2023

View reviewed changes

Add wpdb::update() format properties.

c2dc23d

Co-authored-by: Darin Kotter <[email protected]>

dkotter approved these changes Feb 21, 2023

View reviewed changes

Sidsector9 approved these changes Feb 28, 2023

View reviewed changes

Sidsector9 merged commit 025072b into 10up:develop Feb 28, 2023

jeffpaul added this to the 2.5.0 milestone Feb 28, 2023

dkotter mentioned this pull request Jun 21, 2023

Fix a few minor code issues #149

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to reset page order #129

Add functionality to reset page order #129

ruscoe commented Jan 29, 2023 •

edited

Loading

Sidsector9 left a comment •

edited

Loading

ruscoe commented Jan 30, 2023 •

edited

Loading

ruscoe commented Jan 30, 2023

Sidsector9 commented Jan 30, 2023

ruscoe commented Jan 30, 2023

ruscoe commented Jan 30, 2023

Sidsector9 Jan 31, 2023

Sidsector9 Jan 31, 2023

Sidsector9 Jan 31, 2023

Sidsector9 commented Jan 31, 2023

ruscoe commented Jan 31, 2023

Sidsector9 Feb 13, 2023

dkotter Feb 16, 2023

ruscoe Feb 16, 2023 •

edited

Loading

dkotter Feb 21, 2023

	$wpdb->update( 'wp_posts', array( 'menu_order' => 0 ), array( 'post_type' => $post_type ) );
	$wpdb->update( 'wp_posts', array( 'menu_order' => 0 ), array( 'post_type' => $post_type ), array( '%d' ), array( '%s' ) );

Add functionality to reset page order #129

Add functionality to reset page order #129

Conversation

ruscoe commented Jan 29, 2023 • edited Loading

Description of the Change

How to test the Change

Changelog Entry

Credits

Checklist:

Sidsector9 left a comment • edited Loading

Choose a reason for hiding this comment

OR

ruscoe commented Jan 30, 2023 • edited Loading

ruscoe commented Jan 30, 2023

Sidsector9 commented Jan 30, 2023

ruscoe commented Jan 30, 2023

ruscoe commented Jan 30, 2023

Sidsector9 Jan 31, 2023

Choose a reason for hiding this comment

Sidsector9 Jan 31, 2023

Choose a reason for hiding this comment

Sidsector9 Jan 31, 2023

Choose a reason for hiding this comment

Sidsector9 commented Jan 31, 2023

ruscoe commented Jan 31, 2023

Sidsector9 Feb 13, 2023

Choose a reason for hiding this comment

dkotter Feb 16, 2023

Choose a reason for hiding this comment

ruscoe Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

dkotter Feb 21, 2023

Choose a reason for hiding this comment

ruscoe commented Jan 29, 2023 •

edited

Loading

Sidsector9 left a comment •

edited

Loading

ruscoe commented Jan 30, 2023 •

edited

Loading

ruscoe Feb 16, 2023 •

edited

Loading