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

Single sync option if index is deleted #3011

Merged
merged 22 commits into from
Nov 9, 2022
Merged

Single sync option if index is deleted #3011

merged 22 commits into from
Nov 9, 2022

Conversation

MARQAS
Copy link
Contributor

@MARQAS MARQAS commented Sep 20, 2022

Description of the Change

This PR will address the issue where the Sync screen was showing 2 sync options along with the Last sync info. This will only show the Sync option which is visible during the initial syn.

Closes #2962

How to test the Change

  • In WP-CLI run wp elasticpress delete-index --index-name=$index_name --yes
  • Visit the Sync page
  • Observe that it shows single sync option without last sync info

Changelog Entry

Fixed - Sync page shows a successful previous sync even if the index has been deleted

Credits

Props @MARQAS

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.

@MARQAS MARQAS self-assigned this Sep 20, 2022
@MARQAS MARQAS marked this pull request as ready for review September 20, 2022 13:30
Copy link
Member

@felipeelia felipeelia left a comment

Choose a reason for hiding this comment

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

The approach you have here almost works @MARQAS but if you delete only one index, you get a false positive.
Steps to reproduce:

  1. Enable "Post Search" and "Users" features
  2. Run a sync and see 2 indices in the Health screen
  3. Delete only one index (wp elasticpress delete-index --index-name=elasticpresstest-post-1 --yes)
  4. See 2 boxes in the Sync page (it should have only the Full Sync one)

Comment on lines 176 to 177
// Get the stats of the current index
$index_stats = Stats::factory()->get_totals();
Copy link
Member

Choose a reason for hiding this comment

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

get_totals() are not the best way to check what we need @MARQAS. What we will need to do is to compare the results of Command::get_cluster_indexes() and Command::get_index_names(). As we won't be calling methods from the Command class outside the WP_CLI context, we can create similar methods in the Elasticsearch class and start calling those from the Command class and here, in the Sync class.

@felipeelia felipeelia added this to the 4.4.0 milestone Oct 11, 2022
Copy link
Member

@felipeelia felipeelia left a comment

Choose a reason for hiding this comment

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

@MARQAS, In addition to my comments. as you've moved the get_index_names and get_cluster_indices methods to the Elasticsearch class, shouldn't that generate a change in Command.php? Thanks!

includes/classes/Elasticsearch.php Outdated Show resolved Hide resolved
includes/classes/Screen/Sync.php Outdated Show resolved Hide resolved
tests/cypress/integration/dashboard-sync.cy.js Outdated Show resolved Hide resolved
assets/js/sync/components/sync-page.js Outdated Show resolved Hide resolved
@felipeelia felipeelia merged commit ef729d7 into develop Nov 9, 2022
@felipeelia felipeelia deleted the fix/2962 branch November 9, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Sync page shows a successful previous sync even if the index has been deleted
3 participants