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

feat(search): ES-466 Facets with & symbol works now #1654

Merged
merged 1 commit into from Apr 6, 2020

Conversation

bc-krishsenthilraj
Copy link
Contributor

@bc-krishsenthilraj bc-krishsenthilraj commented Apr 1, 2020

What?

Faceted search filters that include ampersand (&) breaks 'Show More' links.

With this fix, now we can have Facets with & symbol and Show More link works fine.

Tickets / Documentation

Testing Steps:

  1. Login to CP
  2. Navigate to Products
  3. Edit few Products and add a Custom Field (with same name like _ Custom & Color _) and with different values for each product and save the Products.
  4. Navigate to Products->Product Filtering
  5. Enable the newly created facet.
  6. Click the settings of this facet filter and change the value for show to 5 items and save
  7. Go to SF and click on the Show More link for this new filter and the popup window shows all the filter values.

Screenshots (if appropriate)

  1. List of all Facets:
    image

  2. Facet with & in the name & value
    image

  3. Show More link displays the popup with filterable values
    image

  4. Other Facet's Show More link works fine too..
    image

@bigcommerce/artemis-dt @lord2800 @kwang30

@bigbot
Copy link

bigbot commented Apr 1, 2020

Autotagging @bigcommerce/storefront-team @davidchin

Copy link
Contributor

@lord2800 lord2800 left a comment

Choose a reason for hiding this comment

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

cc @bigcommerce/storefront-team (code in general) and @bigcommerce/security (to verify that there's no injectable parts for product option and custom field values)

We did some testing to ensure it doesn't allow injection, but we didn't go in depth. I'm pretty sure it works, I'd just like independent confirmation.

@@ -3,7 +3,7 @@
<div
class="accordion-navigation toggleLink {{#unless ../start_collapsed }} is-open {{/unless}}"
role="button"
data-collapsible="#facetedSearch-content--{{hyphenate facet }}">
data-collapsible="#facetedSearch-content--{{#replace '&' (hyphenate facet) }}{{/replace}}">
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like the idea of doing special characters replaces at the handlebars level. Today we are doing & tomorrow it is going to be some other special characters.

We should either strip it or escape it at the backend level. Right now

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't escape & to be placed in a fragment. There are no other special characters like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are just doing a replace for & in the string for the fragment can't we do that on the backend itself ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have two variables we can key this on, unfortunately.

Copy link

@bc-benjaminpogue bc-benjaminpogue left a comment

Choose a reason for hiding this comment

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

Completed testing and this LGTM! 👍

@bc-aodvak
Copy link

Tested on staging by uploading and applying theme from the ticket

Screen Shot 2020-04-06 at 12 54 05 PM

Looks good 💚

@bc-williamkwon bc-williamkwon merged commit 3163444 into bigcommerce:master Apr 6, 2020
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.

None yet

7 participants