Conversation
…/feat/x_model_search
bferguso
left a comment
There was a problem hiding this comment.
I like the way you've added the grouping - I think that's really cool.
I see that you can drag & drop the nodegroups in the resource models list- not sure if that does something or not. It'd be nice if the models were sorted alphabetically, unless I'm missing something here (very possible).
I think you may need the {"layoutType": "tabbed"} config value, in the migration used to add the filter, and also needs to be added to the standard-search-view filters list for it to be visible.
Not sure if you're done the logic, or if I'm misinterpreting the results, but if I'm getting some questionable results. If I search the Site Visit for Cynthia, it returns 10 site visits. The when I select the "Intersect to Archaeological Site" I end up w/ 11430 sites which seems suspect. Maybe you can run me through the feature next week.
I'll push this branch into DLVR but I may not push to TEST until we sort out some of the above details.
bferguso
left a comment
There was a problem hiding this comment.
This is looking really good. The grouping UI functionality adds a ton of power to advanced search, and it looks to solve a really complex business requirement.
Some notes before releasing into the wild (in no particular order):
- We should confirm it is honouring the Arches permissions model (ie resources a user doesn't have access to are still being filtered out)
- We should do some load testing before we go to PROD. We'll need to make sure that we're not going to run into performance issues if we have several users using this functionality at the same time. If this becomes a concern we may need to limit it to a user group, or potentially ensure that the filters are restricted enough to to avoid performance issues in the system and on the server
- We should do some additional results verification considering the complexity of this (which is absolutely required to satisfy the requirements). I know @braydencstratusadv has done some initial testing, but we'll need to do some in-depth functional testing, as decisions will be made using this.
- We should provide some documentation about the overall strategy and component details/functionality.
- I'd like to make sure we some of the files overridden in this PR are necessary (eg - paging-filter.js, search-export.js)
- Confirm we're overriding some of the search filters minimally (they're full copies now, and maybe the changes belong in arches core, or the the overridden functionality is more specifically targeted) to minimize core upgrade efforts
- Should we replace the OOB advanced search tab with this one? Are there any downsides to it since this seems to be Advanced Search ++.
I think we can merge this in after ensuring only the minimal set of arches core files have been overridden, and some basic docs have been added. Any other issues we can happen as part of the functional testing cycle.
bferguso
left a comment
There was a problem hiding this comment.
This is looking great! A couple of questions about the short-circuit logic noted below. Also, when searching for "Has no value -> Unreviewed ADIF Record", I'm not getting any results, but when I do the same in the regular advanced search I get the list expected. Great work!
bferguso
left a comment
There was a problem hiding this comment.
Thanks for all the hard work on this. Looks great!
No description provided.