Closed redrun45 closed 1 month ago
redrun45
commented
2 months ago Bad news: I don't have anything for simple search. Good news: we'd like to have this in Advanced Search, with the further exclusions I mentioned just under the "Instead" heading of the first post. If there are no objections by then, I'll update the PR and mark it ready, on either Wednesday or Thursday.
redrun45
commented
1 month ago Well... I'm learning this process, at any rate. My technique for splitting commits needs some work. 😓
At any rate, this PR should now:
Both of these changes have been described to admins, and all concerns addressed. We have consensus. 👍
notartom
commented
1 month ago Let's merge and deploy this, but keep the performance aspect in mind. We don't have any sort of monitoring thing, but if we notice slowdowns and I open mytop and see a bunch of these queries there, we'll know something is up :)
redrun45
commented
1 month ago Let's merge and deploy this, but keep the performance aspect in mind. We don't have any sort of monitoring thing, but if we notice slowdowns and I open
mytopand see a bunch of these queries there, we'll know something is up :)
Yep, this is where I feel very much that I don't yet know how much I don't know. 😅
While we're on the topic, the way we combine these queries is also not ideal. We take the result of one query (run by either _get_projects_by_section_language or _get_reader_ids), and then we use PHP to "copy/paste" that into the string for the next query, which seems like a relatively expensive way to do a sub-query!
Whether that makes the list of IDs "constant" from the DB's perspective as it runs that second query, I don't know. I think a definitely-sub-optimal use of 'IN' might be better than this overhead of passing results from the DB to PHP and back again... but for now, I'll be content if the search results are correct without a major (or hopefully even a noticeable) slowdown. So yes, please watch and see! 🎉
Status has changed, see the latest comment in chain. 😄
Status of this PR:
Continuing discussion from #12.
The earlier proposed change would indeed hide Sections from the search results, where they were part of a mono-lingual compilation Project. The Project itself would ideally be a single result, rather than a scattering of individual Sections.
This makes perfect sense, unless we're searching by a detail that is specific to each Section. For example, even if our search is an exact match for a Section's title, that Section will still be hidden. And since the Project title will not match that search, we'll get no results at all.
One approach: I know there is a special Project <-> Group relation, where searching for the title of a Project (e.g. "The Tower Treasure") will make the relevant Group ("The Hardy Boys") show up in the results. You can see the mechanism for this in
application/controllers/cron/Search_table_update.php. That would work, as far as I can see, but it comes with both technical and usability trade-offs - more entries in the search table, and less visibility into "what is this thing, and why is it in my results for this particular search?"Instead: Here, I've set up @rillig's change to apply only when we are not searching by title. We could extend this exception for searches by author and by reader, as are also Section-specific. That would come with some edge-case false positives... which we also have in master, so why not? I digress.
I'm now wondering if we wouldn't rather leave the Advanced Search as it is, and instead apply a change like this to the Browse by Language and/or Simple Search features. If other admins have the same thought, then I'll likely approach that with fresh eyes in a week or three. I'm tapped out on this for the moment. 😪
Happy to hear any opinions! I'll get back to them when ready.