Changed listing tables search logic to operate on catalog metadata #619
Conversation
|
Note to myself: |
|
I've done some functional testing. I just noticed that on filtering (via search term), the "Show more" button makes the whole page to be refreshed (with filtering and other params in GET) instead of adding the next items at the end of the list as it does when no term search is used. Could be nice to keep the same behavior when no search term is used. Is this possible? EDIT: @3c615a3 + @149c439 resolve this particular issue |
|
I've been testing your PR from a functional point of view and works excellent, but I am a bit worried about the performance of this search approach for large amounts of items. A search against 44289 ARs took ~30 seconds: but... I like more your approach than the previous one with Maybe we could add a |
|
Good idea and I really like it. I'm just not sure if these in-between searches are working as well for ZCText Indexes. An other approach would be maybe to reduce the final filter set even further or just yielding the filtered results until the pagesize is reached? |
|
I'd say that yielding the filtered results until the pagesize is reached is not worth. If you see the results I obtained above, the first search only found 11 matches from 44k records, while the pagesize is 50 by default. So, with the yeilding approach for this search, you'll not find any difference. Same happens if you are searching for a search tearm that only matches with records that were created long time ago (e.g. if you want to search for records from 2y ago). I still think that |
|
Good catch. Indeed it would be only an improvement if not all catalog results need to be processed. I'll do some further research on this to look for some further approaches. If I don't find one, I would suggest to do the improvement in another PR to get this functionality already in as it is. |
| toggle_cols = self.get_toggle_cols() | ||
| for col in self.columns.keys(): | ||
| self.columns[col]['toggle'] = col in toggle_cols | ||
| return self.pagesize |
There was a problem hiding this comment.
...or return utils.to_int(pagesize, self.pagesize)
| if self.get_rows_only(): | ||
| return self.contents_table(table_only=form_id) | ||
| else: | ||
| return self.template(self.context) |
| try: | ||
| return int(limit) | ||
| except (ValueError, TypeError): | ||
| return 0 |
| return False | ||
| catalog = self.get_catalog() | ||
| sort_index = catalog.Indexes.get(sort_on) | ||
| if not hasattr(sort_index, 'documentToKeyMap'): |
There was a problem hiding this comment.
pheew!, didn't know that an index without this documentToKeyMap attr was not suitable for sorting, good catch!
I'd add something like logger.warning("Index {} cannot be used for sorting".format(sort_on))
| """ | ||
| if isinstance(thing, DateTime.DateTime): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
return isinstance(thing, DateTime.DateTime) would be more concise
Description of the issue/feature this PR addresses
Linked issue: #608
Current behavior before PR
Listing table search build a big Advanced Query. It did not find any contents for most of the searches.
Desired behavior after PR is merged
The listing table generates now a searchable text out of all brain metadata and filters the brains where the searchterm is found in the catalog metadata.
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.