Fix remove from index on update #3937
Conversation
This change fixes updates that trigger multiple filters for the same post. Currently, when any of the filters adds the post to the update queue the post is indexed, even when other filters determine that the post should not be indexed. This change makes the last filter have the final word about this.
|
Hi @dtakken, thanks for sending the PR! We'll have it reviewed and get back to you with our findings/approval. It can take a while though, so I'll ask you some patience :) |
There was a problem hiding this comment.
Hey @dtakken, thanks for putting this together! We'll need a couple of changes before merging this one.
Also, regarding Cypress tests, do you have any specific questions we can help you with? Were you able to set it up locally?
| if ( ! isset( $this->sync_queue[ $object_id ] ) ) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Since EP 5.0.0 the sync_queue is indexed by the blog ID, so this if can go away (we check the same thing a bit further down.)
There was a problem hiding this comment.
This test should either go to the protected-content.cy.js file or (preferably) to indexables/post.cy.js, but we'll need to make sure the test actually fails if we remove those additional remove_from_queue calls added to SyncManager.php.
| @@ -202,6 +202,26 @@ Cypress.Commands.add('publishPost', (postData, viewPost) => { | |||
| cy.wait(2000); | |||
| }); | |||
|
|
|||
| Cypress.Commands.add('postSetPassword', (id, password) => { | |||
There was a problem hiding this comment.
Can we rename this to setPostPassword instead, please?
This change fixed an issue that I came across: Setting a password on a post and updating it from the admin page did not remove it from the index, unless I hit the update button a second time.
Details about the bug can be found here: #3936
This PR makes the last running filter have the final say about adding the post to the index or not.
I found this issue on WordPress 6.4.3 using ElasticPress 4.7.2. As far as I can see the relevant code has not changed since. I did not test this change on the latest ElasticPress version yet.
I tried to whip up a Cypress test to demonstrate the issue but it does not seem to work correctly. I am not familiar with Cypress, some help is appreciated.
Closes #3936
How to test the Change
Changelog Entry
Credits
Props @dtakken
Checklist: