diff --git a/plugins/knowledgebase/api/routes.py b/plugins/knowledgebase/api/routes.py index bae722b..70df875 100644 --- a/plugins/knowledgebase/api/routes.py +++ b/plugins/knowledgebase/api/routes.py @@ -43,6 +43,39 @@ def _visible_articles(): KnowledgeBase.appid.notin_(retired))) +def _search_clause(search): + """Articles containing EVERY word of the search, each word in the title, the + keywords, or the topic's name, in any order. + + A single `ilike('%CMM Community%')` needs the whole phrase contiguous in ONE + field. An article tagged 'community' under the CMM topic has 'CMM' only in + its topic's name and 'Community' only in its keywords, so it matched nothing + and the search came back empty. Splitting the query and requiring each word + somewhere is what people mean when they type two words. + + Kept in step with `_word_match` in shopdb/core/api/search.py, which does the + same job for global search. Two copies because the core helper is internal + and not on the plugin contract surface (ADR-001). + + Active applications only for the topic. A retired application is not a topic + anyone should be offered: matching its name surfaced its articles and printed + the retired app as their subject, which reads as though it were still in + service. + """ + words = [w for w in search.split() if w] or [''] + return db.and_(*[ + db.or_( + KnowledgeBase.shortdescription.ilike(f'%{w}%'), + KnowledgeBase.keywords.ilike(f'%{w}%'), + KnowledgeBase.appid.in_( + db.session.query(Application.appid).filter( + Application.appname.ilike(f'%{w}%'), + Application.isactive.is_(True))), + ) + for w in words + ]) + + @knowledgebase_bp.route('', methods=['GET']) @jwt_required(optional=True) def list_articles(): @@ -51,26 +84,11 @@ def list_articles(): query = _visible_articles() - # Search: title, keywords, and the topic (its Application's name). The topic - # is matched via an appid subquery instead of a join so it does not collide - # with the sort=='topic' join below; articles with no app just miss that - # clause and still match on title/keywords. + # The topic is matched via an appid subquery instead of a join so it does not + # collide with the sort=='topic' join below; articles with no app just miss + # that clause and still match on title/keywords. if search := request.args.get('search'): - like = f'%{search}%' - # Active applications only. A retired application is not a topic anyone - # should be offered: matching its name surfaced its articles and printed - # the retired app as their subject, which reads as though it were still - # in service. - topic_appids = db.session.query(Application.appid).filter( - Application.appname.ilike(like), - Application.isactive.is_(True)) - query = query.filter( - db.or_( - KnowledgeBase.shortdescription.ilike(like), - KnowledgeBase.keywords.ilike(like), - KnowledgeBase.appid.in_(topic_appids) - ) - ) + query = query.filter(_search_clause(search)) # Filter by topic/application if appid := request.args.get('appid'): diff --git a/shopdb/core/api/search.py b/shopdb/core/api/search.py index 02390b9..85c3753 100644 --- a/shopdb/core/api/search.py +++ b/shopdb/core/api/search.py @@ -31,7 +31,7 @@ search_bp = Blueprint('search', __name__) # cannot reorder. Relevance is applied afterwards in Python, over a stable set. -def _word_match(query, *columns): +def _word_match(query, *columns, extra=None): """SQL clause matching rows that contain EVERY word of the query, each word in any of the columns, in any order. @@ -39,14 +39,25 @@ def _word_match(query, *columns): multi-word search returned nothing. This splits the query into words and ANDs them (OR across the columns per word): 'CSF Roles' matches a row with 'CSF' and 'Roles' anywhere in the searched fields. + + `extra` is an optional callable taking one word and returning a further + clause to OR in for that word. Use it where a word may be satisfied by a + RELATED row rather than a column of this table - a knowledge base article + matching on its topic's name, say. Splitting per word matters there: without + it a word can only ever be matched by the columns, so a query naming the + topic and a keyword ('CMM Community') matches neither side alone. """ words = [w for w in query.split() if w] if not words: words = [''] - return db.and_(*[ - db.or_(*[c.ilike(f'%{w}%') for c in columns]) - for w in words - ]) + + def per_word(word): + clauses = [c.ilike(f'%{word}%') for c in columns] + if extra is not None: + clauses.append(extra(word)) + return db.or_(*clauses) + + return db.and_(*[per_word(w) for w in words]) def _require_enabled(name): @@ -253,6 +264,10 @@ def _search_knowledgebase(query, search_term): subject, which reads as though it were still in service. A null topic still matches. Not every article is about an application. + + The topic's NAME is searched too, per word. An article tagged 'community' + under the CMM topic has neither word in both of its own columns, so a search + for 'CMM Community' found it in neither the plugin's listing nor here. """ results = [] try: @@ -260,12 +275,24 @@ def _search_knowledgebase(query, search_term): from plugins.knowledgebase.models import KnowledgeBase retired = db.session.query(Application.appid).filter( Application.isactive.is_(False)) + + def topic_named(word): + """Articles whose topic is an ACTIVE application named like word. + + Active only, for the same reason the retired filter above exists: a + decommissioned application is not a topic anyone should be offered. + """ + return KnowledgeBase.appid.in_( + db.session.query(Application.appid).filter( + Application.appname.ilike(f'%{word}%'), + Application.isactive.is_(True))) + kb_articles = KnowledgeBase.query.filter( KnowledgeBase.isactive == True, db.or_(KnowledgeBase.appid.is_(None), KnowledgeBase.appid.notin_(retired)), _word_match(query, KnowledgeBase.shortdescription, - KnowledgeBase.keywords) + KnowledgeBase.keywords, extra=topic_named) ).order_by(KnowledgeBase.clicks.desc(), KnowledgeBase.linkid).limit(20).all() diff --git a/tests/test_plugins/test_knowledgebase_search_words.py b/tests/test_plugins/test_knowledgebase_search_words.py new file mode 100644 index 0000000..e26e15f --- /dev/null +++ b/tests/test_plugins/test_knowledgebase_search_words.py @@ -0,0 +1,128 @@ +"""Searching a knowledge base article by words that live in different fields. + +An article's searchable text is spread across three places: its title, its +keywords, and the name of its topic. People do not know or care which word came +from which, so they type the two words that identify the article and expect it +back - 'CMM Community' for an article tagged `community` under the CMM topic. + +Both search paths used one `ilike('%CMM Community%')`, which needs that exact +phrase contiguous in a SINGLE field. No article has it, so the search returned +nothing at all and the article looked absent rather than unfindable. Every word +must be found somewhere, in any order, across all three fields. + +Covers the plugin's own listing (`GET /api/knowledgebase?search=`, what the KB +page uses) and global search (`GET /api/search?q=`), which had the fault in two +different shapes: the listing did not split words, and global search split them +but never looked at the topic. +""" + +import pytest + +from shopdb.core.models import Application +from plugins.knowledgebase.models import KnowledgeBase + + +@pytest.fixture +def library(db): + """Articles whose identifying words are deliberately spread across fields.""" + topics = { + 'cmm': Application(appname='Zzcmm', isactive=True), + 'other': Application(appname='Zzunrelated', isactive=True), + 'retired': Application(appname='Zzretired Zzcmm', isactive=False), + } + db.session.add_all(topics.values()) + db.session.flush() + + articles = { + # The reported case: neither word is in the same field as the other. + 'split': KnowledgeBase( + shortdescription='Zzprotect viewer notes', linkurl='u1', + keywords='zzcommunity license', appid=topics['cmm'].appid, + isactive=True), + # Both words present but not adjacent, inside one field. + 'gapped': KnowledgeBase( + shortdescription='Zzpower outage plan alert zznotification', + linkurl='u2', keywords='', appid=None, isactive=True), + # Holds only one of the two words; must NOT come back. + 'halfmatch': KnowledgeBase( + shortdescription='Zzprotect viewer only', linkurl='u3', + keywords='license', appid=topics['other'].appid, isactive=True), + # Retired topic, matches both words. The existing rule still wins. + 'retired': KnowledgeBase( + shortdescription='Zzprotect viewer retired', linkurl='u4', + keywords='zzcommunity', appid=topics['retired'].appid, + isactive=True), + } + db.session.add_all(articles.values()) + db.session.commit() + return {'topics': topics, 'articles': articles} + + +def listed(client, search): + """Titles from the plugin's own listing, the one the KB page calls.""" + resp = client.get('/api/knowledgebase', query_string={'search': search}) + assert resp.status_code == 200, resp.get_json() + return [row['shortdescription'] for row in resp.get_json()['data']] + + +def globally(client, search): + """Knowledge base titles from global search.""" + resp = client.get('/api/search', query_string={'q': search}) + assert resp.status_code == 200, resp.get_json() + return [r['title'] for r in resp.get_json()['data']['results'] + if r['type'] == 'knowledgebase'] + + +# --- the reported case ------------------------------------------------------ + +def test_listing_finds_an_article_by_its_topic_and_its_keyword(client, library): + """'Zzcmm Zzcommunity': one word from the topic's name, one from the keywords. + + This is the bug as reported. A phrase match could satisfy neither field. + """ + assert 'Zzprotect viewer notes' in listed(client, 'Zzcmm Zzcommunity') + + +def test_global_search_finds_an_article_by_its_topic_and_its_keyword(client, library): + """Global search split the words but only ever looked at title and keywords, + so a word that only the topic could satisfy failed there too.""" + assert 'Zzprotect viewer notes' in globally(client, 'Zzcmm Zzcommunity') + + +# --- word order and adjacency ---------------------------------------------- + +def test_word_order_does_not_matter(client, library): + assert 'Zzprotect viewer notes' in listed(client, 'Zzcommunity Zzcmm') + + +def test_words_need_not_be_adjacent_within_one_field(client, library): + """'plan' and 'alert' sit between them in the title.""" + assert 'Zzpower outage plan alert zznotification' in listed( + client, 'Zzpower zznotification') + + +def test_the_whole_phrase_still_matches(client, library): + """Splitting must not break the searches that already worked.""" + assert 'Zzprotect viewer notes' in listed(client, 'Zzprotect viewer') + + +# --- what must still be excluded ------------------------------------------- + +def test_an_article_holding_only_one_word_is_not_returned(client, library): + """EVERY word must be found. ORing them would return most of the library and + bury the article the words actually describe.""" + found = listed(client, 'Zzprotect Zzcommunity') + assert 'Zzprotect viewer only' not in found + assert 'Zzprotect viewer notes' in found + + +def test_a_retired_topic_still_hides_the_article(client, library): + """Matching more words must not become a way past the retired rule.""" + assert 'Zzprotect viewer retired' not in listed(client, 'Zzprotect Zzcommunity') + assert 'Zzprotect viewer retired' not in globally(client, 'Zzprotect Zzcommunity') + + +def test_a_retired_topic_name_is_not_a_way_in(client, library): + """The retired application is named 'Zzretired Zzcmm'; its name matching a word + must not surface its articles.""" + assert listed(client, 'Zzretired') == []