Skip to content

Commit

Permalink
Merge pull request #25362 from frappe/mergify/bp/version-15-hotfix/pr…
Browse files Browse the repository at this point in the history
…-25348

perf: cap max count on list views (backport #25348)
  • Loading branch information
ankush committed Mar 12, 2024
2 parents 785b5c2 + 2539eff commit ba0643e
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 29 deletions.
2 changes: 1 addition & 1 deletion cypress/integration/list_paging.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@ context("List Paging", () => {
cy.get(".list-paging-area .list-count").should("contain.text", "500 of");
cy.get(".list-paging-area .btn-more").click();

cy.get(".list-paging-area .list-count").should("contain.text", "1000 of");
cy.get(".list-paging-area .list-count").should("contain.text", "1,000 of");
});
});
22 changes: 16 additions & 6 deletions frappe/desk/reportview.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
from frappe.model.base_document import get_controller
from frappe.model.db_query import DatabaseQuery
from frappe.model.utils import is_virtual_doctype
from frappe.utils import add_user_info, format_duration
from frappe.utils import add_user_info, cint, format_duration
from frappe.utils.data import sbool


@frappe.whitelist()
Expand Down Expand Up @@ -51,13 +52,22 @@ def get_count() -> int:

if is_virtual_doctype(args.doctype):
controller = get_controller(args.doctype)
data = controller.get_count(args)
count = controller.get_count(args)
else:
distinct = "distinct " if args.distinct == "true" else ""
args.fields = [f"count({distinct}`tab{args.doctype}`.name) as total_count"]
data = execute(**args)[0].get("total_count")
args.distinct = sbool(args.distinct)
distinct = "distinct " if args.distinct else ""
args.limit = cint(args.limit)
fieldname = f"{distinct}`tab{args.doctype}`.name"

if args.limit:
args.fields = [fieldname]
partial_query = execute(**args, run=0)
count = frappe.db.sql(f"""select count(*) from ( {partial_query} ) p""")[0][0]
else:
args.fields = [f"count({fieldname}) as total_count"]
count = execute(**args)[0].get("total_count")

return data
return count


def execute(doctype, *args, **kwargs):
Expand Down
2 changes: 2 additions & 0 deletions frappe/public/js/frappe/db.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ frappe.db = {
},
count: function (doctype, args = {}) {
let filters = args.filters || {};
let limit = args.limit;

// has a filter with childtable?
const distinct =
Expand All @@ -111,6 +112,7 @@ frappe.db = {
filters,
fields,
distinct,
limit,
});
},
get_link_options(doctype, txt = "", filters = {}) {
Expand Down
50 changes: 41 additions & 9 deletions frappe/public/js/frappe/list/list_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList {
this.process_document_refreshes.bind(this),
2000
);
this.count_upper_bound = 1001;
}

has_permissions() {
Expand Down Expand Up @@ -500,9 +501,9 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList {

freeze() {
if (this.list_view_settings && !this.list_view_settings.disable_count) {
this.$result
.find(".list-count")
.html(`<span>${__("Refreshing", null, "Document count in list view")}...</span>`);
this.get_count_element().html(
`<span>${__("Refreshing", null, "Document count in list view")}...</span>`
);
}
}

Expand Down Expand Up @@ -616,11 +617,33 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList {
}

render_count() {
if (!this.list_view_settings.disable_count) {
this.get_count_str().then((str) => {
this.$result.find(".list-count").html(`<span>${str}</span>`);
});
}
if (this.list_view_settings.disable_count) return;

let me = this;
let $count = this.get_count_element();
this.get_count_str().then((count) => {
$count.html(`<span>${count}</span>`);
if (this.count_upper_bound) {
$count.attr(
"title",
__(
"The count shown is an estimated count. Click here to see the accurate count."
)
);
$count.tooltip({ delay: { show: 600, hide: 100 }, trigger: "hover" });
$count.on("click", () => {
me.count_upper_bound = 0;
$count.off("click");
$count.tooltip("disable");
me.freeze();
me.render_count();
});
}
});
}

get_count_element() {
return this.$result.find(".list-count");
}

get_header_html() {
Expand Down Expand Up @@ -946,12 +969,21 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList {
return frappe.db
.count(this.doctype, {
filters: this.get_filters_for_args(),
limit: this.count_upper_bound,
})
.then((total_count) => {
this.total_count = total_count || current_count;
this.count_without_children =
count_without_children !== current_count ? count_without_children : undefined;
let str = __("{0} of {1}", [current_count, this.total_count]);

let count_str;
if (this.total_count === this.count_upper_bound) {
count_str = `${format_number(this.total_count - 1, null, 0)}+`;
} else {
count_str = format_number(this.total_count, null, 0);
}

let str = __("{0} of {1}", [format_number(current_count, null, 0), count_str]);
if (this.count_without_children) {
str = __("{0} of {1} ({2} rows with children)", [
count_without_children,
Expand Down
1 change: 1 addition & 0 deletions frappe/public/js/frappe/views/file/file_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ frappe.views.FileView = class FileView extends frappe.views.ListView {
} else {
super.render();
this.render_header();
this.render_count();
}
}

Expand Down
15 changes: 5 additions & 10 deletions frappe/public/js/frappe/views/reports/report_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,19 +221,14 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView {
this.setup_datatable(this.data);
}

render_count() {
if (this.list_view_settings?.disable_count) {
return;
}
let $list_count = this.$paging_area.find(".list-count");
if (!$list_count.length) {
$list_count = $("<span>")
get_count_element() {
let $count = this.$paging_area.find(".list-count");
if (!$count.length) {
$count = $("<span>")
.addClass("text-muted list-count")
.prependTo(this.$paging_area.find(".level-right"));
}
this.get_count_str().then((str) => {
$list_count.text(str);
});
return $count;
}

on_update(data) {
Expand Down
21 changes: 18 additions & 3 deletions frappe/tests/test_db_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,7 @@ def test_get_count(self):
"distinct": "false",
}
)
list_filter_response = execute_cmd("frappe.desk.reportview.get_count")
count = execute_cmd("frappe.desk.reportview.get_count")
frappe.local.form_dict = frappe._dict(
{
"doctype": "DocType",
Expand All @@ -1204,8 +1204,8 @@ def test_get_count(self):
}
)
dict_filter_response = execute_cmd("frappe.desk.reportview.get_count")
self.assertIsInstance(list_filter_response, int)
self.assertEqual(list_filter_response, dict_filter_response)
self.assertIsInstance(count, int)
self.assertEqual(count, dict_filter_response)

# test with child table filter
frappe.local.form_dict = frappe._dict(
Expand All @@ -1226,6 +1226,21 @@ def test_get_count(self):
)[0][0]
self.assertEqual(child_filter_response, current_value)

# test with limit
limit = 2
frappe.local.form_dict = frappe._dict(
{
"doctype": "DocType",
"filters": [["DocType", "is_virtual", "=", 1]],
"fields": [],
"distinct": "false",
"limit": limit,
}
)
count = execute_cmd("frappe.desk.reportview.get_count")
self.assertIsInstance(count, int)
self.assertLessEqual(count, limit)

def test_reportview_get(self):
user = frappe.get_doc("User", "test@example.com")
add_child_table_to_blog_post()
Expand Down

0 comments on commit ba0643e

Please sign in to comment.