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

10804 componentizing core search #10807

Merged
merged 218 commits into from
Aug 21, 2024
Merged

Conversation

whatisgalen
Copy link
Member

@whatisgalen whatisgalen commented Apr 19, 2024

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Fundamentally, here's what this PR tries to accomplish:

  • Greater Extensibility: for search customizations to be more useful to the community, they must be easy-ish to "plug in". Overriding the Search view in a project doesn't really meet that criteria. Creating a component that lives in your project makes a lot more sense. For Arches to be able to switch out its core search logic, however, that core search logic needs to be removed and moved into a component itself
  • Outsource technical debt from the SearchView onto search_components: By stripping down one of the core methods (search_results) to just: initialize a SearchEngine, create a response object, and call 3 standard methods from each ordered set of search filters, there's less of a reason to tinker with the search view and development can be channeled into the search_components.

Specifically, here's an overview of the changes:

  • a special component called a SearchLogicComponent governs both what search components are available to the frontend (and what the frontend logic is) as well as what search components are required on the backend and how to handle the search request
  • search.htm is just a placeholder for whatever template the search-logic component has and search.js doesn't do much beyond set up the necessities and send a request to get a SearchView. SearchLogic can be set as default when none is provided, or it can be provided as a named parameter in the url: search?...&search-logic=custom-search-logic
  • a migration inserts the arches default core logic called standard-search-logic along with changes to specific components and adding a config column to all.
  • search components extending BaseFilter class methods (append_dsl, etc.) now take the entire response_object and bundled kwargs as opposed to specifically a results object or include_provisional kwargs, respectively.

Issues Solved

#10804

Checklist

  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Ticket Background

Further comments

@whatisgalen whatisgalen marked this pull request as draft April 19, 2024 06:36
@whatisgalen
Copy link
Member Author

The latest commits address all requested changes except for finalizing specific values for search_component.type

@chiatt chiatt removed their assignment Aug 14, 2024
Copy link
Member

@apeters apeters left a comment

Choose a reason for hiding this comment

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

Almost there!

if (this.requiredFiltersLoaded() === false) {
this.requiredFiltersLoaded.subscribe(function () {
this.mapFilter = this.getFilterByType("map-filter");
this.mapFilter = this.getFilter("map-filter", false);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be this.getFilterByType('map-filter-type', false)

Copy link
Member Author

@whatisgalen whatisgalen Aug 20, 2024

Choose a reason for hiding this comment

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

if you check out line 61: if (tab === "map-filter") { the value for tab comes from that search-filter's component name, so even if another filter like galen-map-filter was the right type ("map-filter-type"), the tab wouldn't be called "map-filter". If that check weren't there, I would agree that it should just be getFilterByType.

In short: we need to ensure that the filter we're assigning to this.mapFilter is the same one we're calling via if (tab === "map-filter")

@@ -15,10 +15,9 @@ Arches 7.6.0 Release Notes

- Adds Github actions to build applications, run tests, and compare coverage between branches for projects.

- Plugins now support the configuration boolean `is_standalone`. Standalone plugins do not appear in the sidebar, and do not display the sidebar or application header.
Copy link
Member

Choose a reason for hiding this comment

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

should this have been removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

arches/app/search/components/standard_search_view.py Outdated Show resolved Hide resolved
arches/app/search/components/standard_search_view.py Outdated Show resolved Hide resolved
arches/app/search/components/standard_search_view.py Outdated Show resolved Hide resolved
Copy link
Member

@apeters apeters left a comment

Choose a reason for hiding this comment

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

phew! that took a while but I think it was worth it!

@apeters apeters merged commit be8d9f5 into dev/7.6.x Aug 21, 2024
7 checks passed
@whatisgalen whatisgalen deleted the 10804_componentizing_core_search branch August 22, 2024 17:29
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.

4 participants