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

New weighting screen with meta support #3068

Merged
merged 78 commits into from
Oct 25, 2023

Conversation

JakePT
Copy link
Contributor

@JakePT JakePT commented Oct 19, 2022

Description of the Change

This PR implements a new @wordpress/element-based weighting dashboard with support for making meta fields searchable.

elasticpress test_wp-admin_admin php_page=elasticpress-weighting (1)

Originally based on #2588 but with significant rewrites that were required to work with real data and adapt to changes decided on in subsequent UX discussions.

This PR changes the way meta is indexed by ElasticPress. Currently, by default, ElasticPress only indexes:

  • Public meta (keys not prefixed with a _)
  • Keys added using the ep_prepare_meta_allowed_protected_keys filter.

With this change applied:

  1. ElasticPress will only index keys added using ep_prepare_meta_allowed_protected_keys. This includes keys added by the WooCommerce feature, to ensure that the integration works properly. No other keys will be indexed by deafult, public or private.
  2. These keys will be listed on the Weighting screen automatically, in the Metadata section, and the user can choose to make them searchable, and weigh them.
  3. Fields and groups added using the ep_weighting_fields_for_post_type filter, as per this blog post, are supported by the new screen. The new default Metadata section uses the key ep_metadata to avoid conflicts with custom groups, such as the one added in that post's example.
  4. The user can manually add additional keys to the index from the weighting screen using the Add field form in the Metadata section of each post type.
  5. Behind the scenes ElasticPress knows to index manually-added keys by finding them in the weighting settings, so a key can be set to be indexed but not searchable by adding the key from the weighting dashboard without checking Searchable.
  6. The previous behaviour can be restored using the ep_meta_mode filter. When 'auto' is returned by the filter ElasticPress will again index all public meta keys, but users will not be able to manually add keys using the weighting dashboard. When ElasticPress is network activated, this will continue to be the default behaviour.

This has the potential to be a breaking change, which is why it was moved to 5.0. Issues could arise if a site is using queries that are integrated with ElasticPress that assume all public meta keys will be indexed. To resolve this issue the user can add the required keys using the new functionality of the weighting screen, or they could restore the previous behaviour using the ep_meta_mode filter.

Closes #1690

How to test the Change

WIP

Changelog Entry

Added - New weighting dashboard with support for making meta fields searchable.

Credits

Props @JakePT, @mehidi258

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.

@JakePT
Copy link
Contributor Author

JakePT commented Oct 20, 2022

This is now in a state where the front end is fully functional (although a confirmation notice on save wouldn't go amiss), so I think it's time to hand it over for the back-end portion to be implemented. There are a number of things that need to be done on the back end.

The main thing is to respect the new ep_meta_mode option. When set to auto (the default) this is the default behaviour of ElasticPress prior to this change. It should continue to index public meta keys and any keys required by any active Features. However when this is set to manual only fields required by features or manually added to the weighting configuration should be indexed. In the UI the list of fields required by features is determined by the ep_prepare_meta_allowed_protected_keys filter, which is how the WooCommerce feature indexes its metadata, but there may be a better way to handle this.

At the current time any fields added to the weighting configuration will remain even if manual management of metadata is switched back off. This has the advantage of keeping any weighting changes the user made even if they temporarily disable manual metadata management, but could cause issues if weighting is applied for fields that are no longer indexed, or if unexpected fields are searchable. The two possible solutions for this are:

  1. If ep_meta_mod is auto when applying weighting, any fields like meta.${key}.value from the weighting configuration should be ignored. The complication is that the SKU and Variation SKU fields are metadata.
  2. When saving the weighting configuration, if ep_meta_mod is auto, sanitise the saved configuration to remove weighting for any meta fields. Again, the SKU and Variation SKU fields will need special handling.

In either case, handling the SKU and Variation SKU fields (and potentially fields added by users or their theme/plugins) could be achieved by cross referencing the weighting configuration with get_weightable_fields_for_post_type() and checking which group the field belongs to.

So I believe the remaining work required, most of it back end, is:

  1. Only index selected fields and fields required by features when manual metadata management is enabled.
  2. Properly handle saved weighting configuration for meta fields that are no longer indexed or should no longer be searchable once manual management is disabled.
  3. Add some sanitisation/validation to the weighting configuration.
  4. Add a save confirmation message and error handling to the weighting screen (Front-end).

Additionally it may be worth adding information to the response when saving the weighting configuration to indicate whether a reindex is required so that the front end can offer to sync.

@JakePT
Copy link
Contributor Author

JakePT commented Oct 21, 2022

@felipeelia Following our discussion today I have now:

  1. Given the Metadata field group a unique ID to avoid conflicts with https://www.elasticpress.io/blog/2020/06/custom-fields-and-weighted-search-with-elasticpress-part-2/. If a user has used code based on the example at that link to add fields, those fields will be displayed in a separate group to the new Metadata group. This means they will appear regardless of whether manual metadata management has been enabled. Such fields would also continue to be indexed regardless of the manual metadata setting because they will have a weighting configuration.
  2. Sanitised the weighting configuration on save so that if manual metadata management is disabled any weighting configuration for fields in the metadata group is removed. This ensures that the weighting configuration is valid for weighting regardless of whether metadata is being managed manually.

With those changes in place, all that is required on the back-end now is changes to indexing so that:

  • If ep_meta_mode ($weighting->get_ep_meta_mode()) is manual then the only meta keys that are indexed are:
    1. Fields that are required by features (currently determined by ep_prepare_meta_allowed_protected_keys).
    2. Fields with a weighting configuration ($weighting->get_weighting_configuration() are indexed.
  • ep_meta_mode ($weighting->get_ep_meta_mode()) is auto (the default), then the current behaviour is followed and the indexed meta keys are:
    1. Fields that are required by features (currently determined by ep_prepare_meta_allowed_protected_keys).
    2. Public meta keys (i.e. not prefixed with _).

With the sanitising in place no changes to weighting should be required.

@felipeelia
Copy link
Member

@JakePT I've pushed some commits addressing the backend part that was needed. I've also made some changes that I'd like you to review:

  • Brought back the old methods and marked them as deprecated (who knows who depends on those out there...)
  • Make sure we always use 'auto' in multisite, as we don't have the weighting dashboard on those
  • Do not enqueue weighting files on install (e2e tests were failing due to a React error here -- the element was not found on the page)
  • Pull the meta fields from the database instead of Elasticsearch

Things that need to be done:

  1. Fix weighting.cy.js e2e tests
  2. Write new tests for this new functionality (I wrote some unit tests, there is room for more. We will also want e2e tests)
  3. Think about caching those meta fields coming from the database (I can bring that to our sync tomorrow)

Are numbers 1 and 2 things you think you could help with?

Thanks!

@JakePT JakePT changed the title WIP: New weighting dashboard with meta support WIP: New weighting screen with meta support Sep 15, 2023
@JakePT JakePT marked this pull request as ready for review October 19, 2023 16:03
@JakePT JakePT changed the title WIP: New weighting screen with meta support New weighting screen with meta support Oct 19, 2023
@felipeelia felipeelia merged commit 8f19428 into 5.0.0 Oct 25, 2023
9 of 13 checks passed
@felipeelia felipeelia deleted the feature/mapping-weighting-dashboard-refactor branch October 25, 2023 23: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.

Weighting Engine: Post meta support
3 participants