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.

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

Added action_set_object_terms #1969

Conversation

brandon-m-skinner
Copy link
Contributor

@brandon-m-skinner brandon-m-skinner commented Nov 5, 2020

Description of the Change

Queues posts for re-indexing when their terms are changed.

Props @nickdaugherty
cc: @rinatkhaziev @pschoffer @netsuso @parkcityj

Alternate Designs

N/A

Benefits

Post data remains consistent with the database which keeps queries returning expected results.

Possible Drawbacks

The default behaviour is problematic when there are a lot of posts. We only do this behaviour for 10k or less posts and offload any indexing with counts above that to be processed asynchronously.

Verification Process

We noticed data inconsistencies when comparing posts from the DB against posts from ES when editing terms. After applying these changes, the index and the DB were consistent.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

Added queuing of affected posts on term change. Props @nickdaugherty

Queues posts for re-indexing when their terms are changed
@mckdemps
Copy link
Contributor

Using this approach https://www.elastic.co/guide/en/elasticsearch/guide/current/partial-updates.html we should update just the taxonomy field that changes with a bulk update.

@mckdemps
Copy link
Contributor

mckdemps commented Dec 14, 2021

@rebeccahum Curious for your thoughts on the approach suggested above as I know VIP has some potential customers with large term sets. cc @brandon-m-skinner @rinatkhaziev

@mckdemps mckdemps modified the milestones: 4.0.0 (beta 2), 4.0.0 Dec 14, 2021
Copy link
Contributor

@rebeccahum rebeccahum left a comment

Choose a reason for hiding this comment

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

Left some notes.

But overall, I think this is will prove to be useful, especially if we have clients with larger term sets — this way, they won't have to run a re-index to get the accurate terms assigned to posts.

* @since 3.5
*/
public function action_set_object_terms( $object_id, $terms, $tt_ids, $taxonomy, $append, $old_tt_ids ) {
$post_id = $object_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a check for $this->kill_sync():

Suggested change
$post_id = $object_id;
if ( $this->kill_sync() ) {
return;
}
$post_id = $object_id;

public function action_set_object_terms( $object_id, $terms, $tt_ids, $taxonomy, $append, $old_tt_ids ) {
$post_id = $object_id;

$indexable = Indexables::factory()->get( 'post' );
Copy link
Contributor

Choose a reason for hiding this comment

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

For code consistency, let's use $this->indexable_slug even though it will end up evaluating to post:

Suggested change
$indexable = Indexables::factory()->get( 'post' );
$indexable = Indexables::factory()->get( $this->indexable_slug );

// Bypass saving if doing autosave
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a filter for skipping a sync triggered by term changes

@mckdemps mckdemps modified the milestones: 4.0.0, 4.0.0 (beta 2) Jan 25, 2022
@felipeelia
Copy link
Member

This also closes #750

@felipeelia
Copy link
Member

Closing in favor of #2603

@felipeelia felipeelia closed this Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants