diff --git a/erpnext/www/support/index.py b/erpnext/www/support/index.py index 83fb8959d61..ddbb3af7c7f 100644 --- a/erpnext/www/support/index.py +++ b/erpnext/www/support/index.py @@ -1,4 +1,5 @@ import frappe +from frappe.query_builder.functions import Count, Max def get_context(context): @@ -30,25 +31,27 @@ def get_context(context): def get_favorite_articles_by_page_view(): - return frappe.db.sql( - """ - SELECT - t1.name as name, - t1.title as title, - t1.content as content, - t1.route as route, - t1.category as category, - count(t1.route) as count - FROM `tabHelp Article` AS t1 - INNER JOIN - `tabWeb Page View` AS t2 - ON t1.route = t2.path - WHERE t1.published = 1 - GROUP BY route - ORDER BY count DESC - LIMIT 6; - """, - as_dict=True, + ha = frappe.qb.DocType("Help Article") + wpv = frappe.qb.DocType("Web Page View") + return ( + frappe.qb.from_(ha) + .inner_join(wpv) + .on(ha.route == wpv.path) + .select( + # route is the unique page URL, so there is one published article per route: Max() just + # returns that row's columns while keeping the GROUP BY route valid on postgres + Max(ha.name).as_("name"), + Max(ha.title).as_("title"), + Max(ha.content).as_("content"), + ha.route, + Max(ha.category).as_("category"), + Count(ha.route).as_("count"), + ) + .where(ha.published == 1) + .groupby(ha.route) + .orderby(Count(ha.route), order=frappe.qb.desc) + .limit(6) + .run(as_dict=True) ) diff --git a/erpnext/www/support/test_support_index.py b/erpnext/www/support/test_support_index.py new file mode 100644 index 00000000000..52ed2133bc0 --- /dev/null +++ b/erpnext/www/support/test_support_index.py @@ -0,0 +1,89 @@ +# Copyright (c) 2024, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import frappe +from frappe.utils import random_string + +from erpnext.tests.utils import ERPNextTestSuite +from erpnext.www.support.index import get_favorite_articles_by_page_view + + +class TestSupportIndex(ERPNextTestSuite): + def make_help_category(self): + category_name = "_Test Support Category " + random_string(8) + category = frappe.get_doc( + { + "doctype": "Help Category", + "category_name": category_name, + "published": 1, + } + ).insert(ignore_permissions=True) + return category.name + + def make_help_article(self, category, route, title, content, published=1): + article = frappe.get_doc( + { + "doctype": "Help Article", + "title": title, + "category": category, + "content": content, + "route": route, + "published": published, + } + ).insert(ignore_permissions=True) + return article.name + + def seed_page_views(self, path, count): + # Web Page View is in_create/read_only; insert the minimal row the + # converted JOIN reads (path == Help Article.route) directly. + for _ in range(count): + view = frappe.new_doc("Web Page View") + view.path = path + view.is_unique = "1" + view.flags.name_set = True + view.name = frappe.generate_hash("wpv", 12) + view.db_insert() + + def test_favorite_articles_ordered_by_page_view_count(self): + category = self.make_help_category() + + # Distinct, collision-free routes so other published articles in the DB + # can't masquerade as ours. + route_hi = "support-hi-" + random_string(10) + route_lo = "support-lo-" + random_string(10) + + name_hi = self.make_help_article( + category, route_hi, "High Views Article", "
High views content
" + ) + name_lo = self.make_help_article(category, route_lo, "Low Views Article", "Low views content
") + + # More views on route_hi than route_lo: a broken Count/GROUP BY/ORDER BY + # would not reproduce these exact counts or this ordering. + self.seed_page_views(route_hi, 3) + self.seed_page_views(route_lo, 1) + + results = get_favorite_articles_by_page_view() + + by_route = {row.route: row for row in results if row.route in (route_hi, route_lo)} + + # Both of our routes are surfaced by the INNER JOIN on route == path. + self.assertIn(route_hi, by_route, "High-viewed route missing from results") + self.assertIn(route_lo, by_route, "Low-viewed route missing from results") + + # Count(route) reflects the real number of seeded Web Page View rows. + self.assertEqual(by_route[route_hi]["count"], 3) + self.assertEqual(by_route[route_lo]["count"], 1) + + # Max()-wrapped columns carry the article's own data (one row per route). + self.assertEqual(by_route[route_hi].name, name_hi) + self.assertEqual(by_route[route_hi].title, "High Views Article") + self.assertEqual(by_route[route_hi].category, category) + self.assertEqual(by_route[route_lo].name, name_lo) + + # ORDER BY count desc: the higher-viewed route precedes the lower one. + ordered_routes = [row.route for row in results if row.route in (route_hi, route_lo)] + self.assertEqual( + ordered_routes, + [route_hi, route_lo], + "Results not ordered by page-view count descending", + )