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: Use new search endpoint in FE and add search filter operation selection #1623

Merged

Conversation

kristenarmes
Copy link
Contributor

@kristenarmes kristenarmes commented Dec 16, 2021

Summary of Changes

This change has three major sections that are all included together for compatibility:

  1. Work done by @allisonsuarez to have the frontend service point to the new search endpoint.
  2. Search input filters now accept comma separated values rather than only a single value.
  3. When a comma to separate multiple filter terms is typed in any input filter box, a AND/OR toggle will appear, allowing the user to specify whether they are filtering by all of the entered filter terms in the same results, or just looking for at least one of the terms in the results.

Some design decisions to note:

  1. There is a new optional frontend config in the BaseFilterCategory called allowableOperation. If it is not set, the default behavior will allow both AND and OR operations for filtering. If it is set to FilterOperationType.OR, only the OR filter operation will be enabled for that category, which can be used when each search result would only ever include one of the filter terms in the category - e.g. a given table would only have one source database. If it is set to FilterOperationType.AND, only the AND filter operation will be enabled for that category.
  2. A special type called FilterOptions was used to keep track of which checkbox filters were checked or unchecked where a true value meant it should be checked and a false value unchecked. Since now the input filters accept multiple values, this type was removed and the checkboxes are handled the same way as the input filters, where the name of the checkbox being included in the filter means it should be checked and any not included are unchecked.
  3. Since there is now only one search API call per search rather than one per resource, when a search action for a specific resource is taken, only that resource has its state updated from the response to avoid clearing out the previous search for the other resources.
  4. Previously, clearing filters would trigger an empty search in order to clear out the results. With the new API, this was causing an error because it did not expect to get empty searches, so instead of doing an unnecessary API call when clearing filters it will just clear the results state instead.

Tests

Various changes were made to JS tests to account for changes to the way the frontend data is handled with filtering, such as checkboxes using a list of values rather than a special type to make it more consistent with the input filters. New tests were added for the AND/OR toggle behavior.

Documentation

frontend/docs/application_config.md was updated to account for the renamed filter category types, along with fixing outdated links.

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does

allisonsuarez and others added 7 commits December 16, 2021 12:37
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
… make the ts changes needed to show changes in FE

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
…arch api

Signed-off-by: Kristen Armes <karmes@lyft.com>
@boring-cyborg boring-cyborg bot added area:frontend From the Frontend folder category:api labels Dec 16, 2021
Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Kristen Armes <karmes@lyft.com>
Copy link
Member

@Golodhros Golodhros left a comment

Choose a reason for hiding this comment

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

Looking great!

Only some minor comments

…nit tests for v1

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
…narmes/amundsen into search-filter-operation-selection
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
…ests

Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Kristen Armes <karmes@lyft.com>
…p check

Signed-off-by: Kristen Armes <karmes@lyft.com>
…ations to be more clear on what it does

Signed-off-by: Kristen Armes <karmes@lyft.com>
Copy link
Member

@Golodhros Golodhros left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for going the extra mile!

…e instead of keeping it stored as an undefined value

Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Kristen Armes <karmes@lyft.com>
@kristenarmes
Copy link
Contributor Author

kristenarmes commented Dec 22, 2021

Another way of doing this on an immutable way would be:

      // Remove the categoryId from the filters if the new value is empty
      const {[categoryId]: remove, ...updatedResourceFilters} = filterState[resourceType];

      newFilters = {
         ...filterState,
         [resourceType]: updatedResourceFilters,
      };

This would avoid the 'delete', that is a mutating operation (although made on a copy of the object)

@Golodhros I did it this way at first actually, but found I was getting a linting error because the remove is an unused variable. I didn't know if it was preferable to do it like this but then disable the no-unused-vars check for this spot? I figured the delete on the copy of the object was a middle ground to avoid linting issues but not directly mutating the original, but let me know if it's better the other way.

@Golodhros
Copy link
Member

Another way of doing this on an immutable way would be:

      // Remove the categoryId from the filters if the new value is empty
      const {[categoryId]: remove, ...updatedResourceFilters} = filterState[resourceType];

      newFilters = {
         ...filterState,
         [resourceType]: updatedResourceFilters,
      };

This would avoid the 'delete', that is a mutating operation (although made on a copy of the object)

@Golodhros I did it this way at first actually, but found I was getting a linting error because the remove is an unused variable. I didn't know if it was preferable to do it like this but then disable the no-unused-vars check for this spot? I figured the delete on the copy of the object was a middle ground to avoid linting issues but not directly mutating the original, but let me know if it's better the other way.

I see, try using '_' instead of rest, does that remove the warning?
If not, I would go ahead and disable the rule on this line!

Signed-off-by: Kristen Armes <karmes@lyft.com>
@allisonsuarez allisonsuarez merged commit e55476f into amundsen-io:main Dec 22, 2021
@allisonsuarez allisonsuarez deleted the search-filter-operation-selection branch December 22, 2021 19:26
amommendes pushed a commit to amommendes/amundsen that referenced this pull request Jan 21, 2022
…election (amundsen-io#1623)

* almost there

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* almost done

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* FE search API correctly requests search service and responds, need to make the ts changes needed to show changes in FE

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* correct mappings for resource results

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* SEARCH IS FUNCTIONAL WITH NEW API

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* cleanup

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Majority of the work done for filter operation selection using new search api

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing resource specific search with filters and clear filters behavior

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing js tests and betterer issues

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing python test and lint/betterer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Betterer improvements/test fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* removed v0 because it is not needed or accesible anymore, and added unit tests for v1

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* a bit of cleanup from utils, not done yet

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint fixes

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* test _transform_filters

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Adding description and timestamp to dashboard mapping and adding js tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Verified that badge is primitive string type during search, cleaned up check

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the name of the config that optionally restricts filter operations to be more clear on what it does

Signed-off-by: Kristen Armes <karmes@lyft.com>

* If a filter term is deleted, remove the category from the filter state instead of keeping it stored as an undefined value

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Bumping major FE version

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Using destructuring instead of delete for removing empty filter

Signed-off-by: Kristen Armes <karmes@lyft.com>

Co-authored-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Amom Mendes <amommendes@hotmail.com>
ozandogrultan pushed a commit to deliveryhero/amundsen that referenced this pull request Apr 28, 2022
…election (amundsen-io#1623)

* almost there

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* almost done

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* FE search API correctly requests search service and responds, need to make the ts changes needed to show changes in FE

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* correct mappings for resource results

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* SEARCH IS FUNCTIONAL WITH NEW API

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* cleanup

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Majority of the work done for filter operation selection using new search api

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing resource specific search with filters and clear filters behavior

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing js tests and betterer issues

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing python test and lint/betterer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Betterer improvements/test fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* removed v0 because it is not needed or accesible anymore, and added unit tests for v1

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* a bit of cleanup from utils, not done yet

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint fixes

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* test _transform_filters

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Adding description and timestamp to dashboard mapping and adding js tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Verified that badge is primitive string type during search, cleaned up check

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the name of the config that optionally restricts filter operations to be more clear on what it does

Signed-off-by: Kristen Armes <karmes@lyft.com>

* If a filter term is deleted, remove the category from the filter state instead of keeping it stored as an undefined value

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Bumping major FE version

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Using destructuring instead of delete for removing empty filter

Signed-off-by: Kristen Armes <karmes@lyft.com>

Co-authored-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>
zacr pushed a commit to SaltIO/amundsen that referenced this pull request May 13, 2022
…election (amundsen-io#1623)

* almost there

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* almost done

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* FE search API correctly requests search service and responds, need to make the ts changes needed to show changes in FE

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* correct mappings for resource results

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* SEARCH IS FUNCTIONAL WITH NEW API

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* cleanup

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Majority of the work done for filter operation selection using new search api

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing resource specific search with filters and clear filters behavior

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing js tests and betterer issues

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing python test and lint/betterer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Betterer improvements/test fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* removed v0 because it is not needed or accesible anymore, and added unit tests for v1

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* a bit of cleanup from utils, not done yet

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint fixes

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* test _transform_filters

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Adding description and timestamp to dashboard mapping and adding js tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Verified that badge is primitive string type during search, cleaned up check

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the name of the config that optionally restricts filter operations to be more clear on what it does

Signed-off-by: Kristen Armes <karmes@lyft.com>

* If a filter term is deleted, remove the category from the filter state instead of keeping it stored as an undefined value

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Bumping major FE version

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Using destructuring instead of delete for removing empty filter

Signed-off-by: Kristen Armes <karmes@lyft.com>

Co-authored-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
hansadriaans pushed a commit to DataChefHQ/amundsen that referenced this pull request Jun 30, 2022
…election (amundsen-io#1623)

* almost there

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* almost done

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* FE search API correctly requests search service and responds, need to make the ts changes needed to show changes in FE

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* correct mappings for resource results

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* SEARCH IS FUNCTIONAL WITH NEW API

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* cleanup

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Majority of the work done for filter operation selection using new search api

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing resource specific search with filters and clear filters behavior

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing js tests and betterer issues

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing python test and lint/betterer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Betterer improvements/test fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* removed v0 because it is not needed or accesible anymore, and added unit tests for v1

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* a bit of cleanup from utils, not done yet

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint fixes

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* test _transform_filters

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* Adding description and timestamp to dashboard mapping and adding js tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Verified that badge is primitive string type during search, cleaned up check

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the name of the config that optionally restricts filter operations to be more clear on what it does

Signed-off-by: Kristen Armes <karmes@lyft.com>

* If a filter term is deleted, remove the category from the filter state instead of keeping it stored as an undefined value

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Bumping major FE version

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Using destructuring instead of delete for removing empty filter

Signed-off-by: Kristen Armes <karmes@lyft.com>

Co-authored-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:frontend From the Frontend folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants