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

Feature activation: Alert when a new reindex needs to happen #1936

Closed
wants to merge 3 commits into from

Conversation

felipeelia
Copy link
Member

@felipeelia felipeelia commented Oct 26, 2020

Description of the Change

Currently, users don't have much info that a new reindex will start when a certain feature is activated. This PR adds a notice based on the Feature requires_install_reindex attribute.

Alternate Designs

Perhaps we could place the message somewhere else.

Benefits

Users won't start a reindex without notice.

Possible Drawbacks

n/a

Verification Process

Created the code and checked in the Features Screen.

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

Closes #1932

Changelog Entry

Adds a new notice to features that need a resync after activation.


if ( ! $this->is_active() && $this->requires_install_reindex && $can_save ) {
$requirements_status->message = (array) $requirements_status->message;
$requirements_status->message[] = __( 'A new reindex will start when you activate this feature.', 'elasticpress' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, should we put this message at the top?

image

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good point @Rahmon. I think all messages have the same importance, so IDK if we want to come up with a prioritization thing but we can bring that up during our internal sync.
Also, we need to change that __() call to use esc_html__()

@Rahmon Rahmon assigned felipeelia and unassigned Rahmon Nov 15, 2021
@felipeelia felipeelia assigned JakePT and unassigned felipeelia Nov 16, 2021
@felipeelia
Copy link
Member Author

@JakePT I'm assigning this one to you as it can be used to some extent to fix the problem outlined in #1932. Also, this only can be worked when the new sync page is done, as activating a new feature that requires a reindex will now point the user to the new sync page. cc @Rahmon

@felipeelia
Copy link
Member Author

Closing this in favor of #2491

@felipeelia felipeelia closed this Dec 6, 2021
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.

notify of required resync while editing feature settings
4 participants