From 299831d209b0568efcecf80df09aee5e84c21161 Mon Sep 17 00:00:00 2001 From: marination Date: Wed, 16 Nov 2022 19:24:12 +0530 Subject: [PATCH 01/18] fix: server method to return evaluated dict of perms for a document --- frappe/client.py | 9 +++++++++ .../js/frappe/views/kanban/kanban_board.bundle.js | 10 ++++++---- frappe/public/js/frappe/views/kanban/kanban_view.js | 1 + 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/frappe/client.py b/frappe/client.py index 404617b68c..afbb0df5ee 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -302,6 +302,15 @@ def has_permission(doctype, docname, perm_type="read"): # perm_type can be one of read, write, create, submit, cancel, report return {"has_permission": frappe.has_permission(doctype, perm_type.lower(), docname)} +@frappe.whitelist() +def get_doc_permissions(doctype, docname): + """Returns an evaluated document permissions dict like `{"read":1, "write":1}` + + :param doctype: DocType of the document to be evaluated + :param docname: `name` of the document to be evaluated + """ + doc = frappe.get_doc(doctype, docname) + return {"permissions": frappe.permissions.get_doc_permissions(doc)} @frappe.whitelist() def get_password(doctype, name, fieldname): diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index 11e7fba198..971081f5b9 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -333,12 +333,13 @@ frappe.provide("frappe.views"); if (self.$kanban_board.length === 0) { self.$kanban_board = $(frappe.render_template("kanban_board")); + // add column self.$kanban_board.appendTo(self.wrapper); } self.$filter_area = self.cur_list.$page.find(".active-tag-filters"); bind_events(); - setup_sortable(); + setup_sortable(); // column } function make_columns() { @@ -355,7 +356,7 @@ frappe.provide("frappe.views"); bind_clickdrag(); } - function setup_sortable() { + function setup_sortable() { // drag column var sortable = new Sortable(self.$kanban_board.get(0), { group: "columns", animation: 150, @@ -518,7 +519,7 @@ frappe.provide("frappe.views"); function init() { make_dom(); - setup_sortable(); + // setup_sortable(); // drag card make_cards(); store.watch((state, getters) => { return state.cards; @@ -535,6 +536,7 @@ frappe.provide("frappe.views"); indicator: frappe.scrub(column.indicator, "-"), }) ).appendTo(wrapper); + // add task, archive self.$kanban_cards = self.$kanban_column.find(".kanban-cards"); } @@ -564,7 +566,7 @@ frappe.provide("frappe.views"); } } - function setup_sortable() { + function setup_sortable() { // drag card Sortable.create(self.$kanban_cards.get(0), { group: "cards", animation: 150, diff --git a/frappe/public/js/frappe/views/kanban/kanban_view.js b/frappe/public/js/frappe/views/kanban/kanban_view.js index 7eccd7ebf8..f1be1d2849 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_view.js +++ b/frappe/public/js/frappe/views/kanban/kanban_view.js @@ -56,6 +56,7 @@ frappe.views.KanbanView = class KanbanView extends frappe.views.ListView { this.card_meta = this.get_card_meta(); this.page_length = 0; + // frappe run serially get/set perms > push menu items > get_board this.menu_items.push( ...[ { From 8d31e702e82e9e9d204908b16bd997595bbfb956 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 17 Nov 2022 18:46:45 +0530 Subject: [PATCH 02/18] fix: Kanban Board Menu Items Accessibility via perms - Save Filters and Delete Board btns render if 'write' and 'delete' perms exist on Kanban Board - `Create Kanban Board` renders only if 'create' perms exist on Kanban Board - Bind board permisions to `board_perms` property of board object - Get perms via backend call at board initialization, as frontend does not have document object (only doctype meta and perms are cached) --- .../public/js/frappe/list/list_view_select.js | 16 +++-- .../views/kanban/kanban_board.bundle.js | 2 +- .../js/frappe/views/kanban/kanban_view.js | 68 ++++++++++++------- 3 files changed, 55 insertions(+), 31 deletions(-) diff --git a/frappe/public/js/frappe/list/list_view_select.js b/frappe/public/js/frappe/list/list_view_select.js index 5265ace340..daa16432b5 100644 --- a/frappe/public/js/frappe/list/list_view_select.js +++ b/frappe/public/js/frappe/list/list_view_select.js @@ -191,12 +191,16 @@ frappe.views.ListViewSelect = class ListViewSelect { ); }); - this.page.add_custom_menu_item( - kanban_switcher, - __("Create New Kanban Board"), - () => frappe.views.KanbanView.show_kanban_dialog(this.doctype), - true - ); + let perms = this.list_view.board_perms; + let can_create = perms ? perms.create : true; + if (can_create) { + this.page.add_custom_menu_item( + kanban_switcher, + __("Create New Kanban Board"), + () => frappe.views.KanbanView.show_kanban_dialog(this.doctype), + true + ); + } } get_page_name() { diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index 971081f5b9..ae11c0fdc0 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -519,7 +519,7 @@ frappe.provide("frappe.views"); function init() { make_dom(); - // setup_sortable(); // drag card + setup_sortable(); // drag card make_cards(); store.watch((state, getters) => { return state.cards; diff --git a/frappe/public/js/frappe/views/kanban/kanban_view.js b/frappe/public/js/frappe/views/kanban/kanban_view.js index f1be1d2849..506d4ee1b2 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_view.js +++ b/frappe/public/js/frappe/views/kanban/kanban_view.js @@ -56,33 +56,53 @@ frappe.views.KanbanView = class KanbanView extends frappe.views.ListView { this.card_meta = this.get_card_meta(); this.page_length = 0; - // frappe run serially get/set perms > push menu items > get_board - this.menu_items.push( - ...[ - { - label: __("Save filters"), - action: () => { - this.save_kanban_board_filters(); - }, - }, - { - label: __("Delete Kanban Board"), - action: () => { - frappe.confirm("Are you sure you want to proceed?", () => { - frappe.db.delete_doc("Kanban Board", this.board_name).then(() => { - frappe.show_alert(`Kanban Board ${this.board_name} deleted.`); - frappe.set_route("List", this.doctype, "List"); - }); - }); - }, - }, - ] - ); - - return this.get_board(); + return frappe.run_serially([ + () => this.set_board_perms_and_push_menu_items(), + () => this.get_board(), + ]); }); } + set_board_perms_and_push_menu_items() { + // needs server-side call as client-side document instance is absent before kanban render + return frappe.call({ + method: "frappe.client.get_doc_permissions", + args: { + doctype: "Kanban Board", + docname: this.board_name + }, + callback: (result) => { + this.board_perms = result.message.permissions || {}; + this.push_menu_items(); + }, + }); + } + + push_menu_items() { + if (this.board_perms.write) { + this.menu_items.push({ + label: __("Save filters"), + action: () => { + this.save_kanban_board_filters(); + } + }); + } + + if (this.board_perms.delete) { + this.menu_items.push({ + label: __("Delete Kanban Board"), + action: () => { + frappe.confirm("Are you sure you want to proceed?", () => { + frappe.db.delete_doc("Kanban Board", this.board_name).then(() => { + frappe.show_alert(`Kanban Board ${this.board_name} deleted.`); + frappe.set_route("List", this.doctype, "List"); + }); + }); + }, + }); + } + } + setup_paging_area() { // pass } From dcbfcdf8b9098ef723dec4ca9566cd4bc18bac63 Mon Sep 17 00:00:00 2001 From: marination Date: Wed, 23 Nov 2022 20:41:04 +0530 Subject: [PATCH 03/18] fix: Check perms on Kanban Column actions - Check Column options access (archive, indicators) - Check column dragability access (allow if write access to board) - Check card dragability access (allow if write access to board) - Hide "Add Column" if no write access to board - Avoid board update on load without write access --- .../views/kanban/kanban_board.bundle.js | 34 ++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index ae11c0fdc0..52eb912c78 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -297,6 +297,7 @@ frappe.provide("frappe.views"); self.wrapper = opts.wrapper; self.cur_list = opts.cur_list; self.board_name = opts.board_name; + self.board_perms = self.cur_list.board_perms; self.update = function (cards) { // update cards internally @@ -325,7 +326,11 @@ frappe.provide("frappe.views"); store.watch((state, getters) => { return state.empty_state; }, show_empty_state); - store.dispatch("update_order"); + + if (self.board_perms.write) { + // If write access to Board, update Kanban cards order on load + store.dispatch("update_order"); + } } function prepare() { @@ -347,7 +352,7 @@ frappe.provide("frappe.views"); var columns = store.state.columns; columns.filter(is_active_column).map(function (col) { - frappe.views.KanbanBoardColumn(col, self.$kanban_board); + frappe.views.KanbanBoardColumn(col, self.$kanban_board, self.board_perms); }); } @@ -356,7 +361,10 @@ frappe.provide("frappe.views"); bind_clickdrag(); } - function setup_sortable() { // drag column + function setup_sortable() { + // If no write access, editing board (by dragging column) should be blocked + if (!self.board_perms.write) return; + var sortable = new Sortable(self.$kanban_board.get(0), { group: "columns", animation: 150, @@ -372,6 +380,12 @@ frappe.provide("frappe.views"); } function bind_add_column() { + if (!self.board_perms.write) { + // If no write access, editing board (by adding column) should be blocked + self.$kanban_board.find(".add-new-column").hide(); + return; + } + var $add_new_column = self.$kanban_board.find(".add-new-column"), $compose_column = $add_new_column.find(".compose-column"), $compose_column_form = $add_new_column.find(".compose-column-form").hide(); @@ -513,7 +527,7 @@ frappe.provide("frappe.views"); return self; }; - frappe.views.KanbanBoardColumn = function (column, wrapper) { + frappe.views.KanbanBoardColumn = function (column, wrapper, board_perms) { var self = {}; var filtered_cards = []; @@ -566,7 +580,10 @@ frappe.provide("frappe.views"); } } - function setup_sortable() { // drag card + function setup_sortable() { + // If no write access, editing board (by dragging card) should be blocked + if (!board_perms.write) return; + Sortable.create(self.$kanban_cards.get(0), { group: "cards", animation: 150, @@ -641,6 +658,12 @@ frappe.provide("frappe.views"); } function bind_options() { + if (!board_perms.write) { + // If no write access, column options should be hidden + self.$kanban_column.find(".column-options").hide(); + return; + } + self.$kanban_column .find(".column-options .dropdown-menu") .on("click", "[data-action]", function () { @@ -654,6 +677,7 @@ frappe.provide("frappe.views"); store.dispatch("set_indicator", { column, color }); } }); + get_column_indicators(function (indicators) { let html = `
  • ${indicators .map((indicator) => { From 77f3a74ecaa357a69b4cc5d067b5f59d43f26f2d Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 24 Nov 2022 14:08:05 +0530 Subject: [PATCH 04/18] fix: Hide Sort Selector in Kanban - It just re-renders the board that gets the card order from the db and makes no difference to the card order even temporarily --- frappe/public/js/frappe/views/kanban/kanban_view.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frappe/public/js/frappe/views/kanban/kanban_view.js b/frappe/public/js/frappe/views/kanban/kanban_view.js index 506d4ee1b2..693326b062 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_view.js +++ b/frappe/public/js/frappe/views/kanban/kanban_view.js @@ -69,7 +69,7 @@ frappe.views.KanbanView = class KanbanView extends frappe.views.ListView { method: "frappe.client.get_doc_permissions", args: { doctype: "Kanban Board", - docname: this.board_name + docname: this.board_name, }, callback: (result) => { this.board_perms = result.message.permissions || {}; @@ -84,7 +84,7 @@ frappe.views.KanbanView = class KanbanView extends frappe.views.ListView { label: __("Save filters"), action: () => { this.save_kanban_board_filters(); - } + }, }); } @@ -124,6 +124,7 @@ frappe.views.KanbanView = class KanbanView extends frappe.views.ListView { this.hide_sidebar = true; this.hide_page_form = true; this.hide_card_layout = true; + this.hide_sort_selector = true; super.setup_page(); } From 0981f593ffa249eaa0d7732b1d58243c95fbfa14 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 24 Nov 2022 20:30:24 +0530 Subject: [PATCH 05/18] fix: Allow card actions (adding/dragging) if access to reference doctype - allow user to update kanban column cards order even without write access --- frappe/desk/doctype/kanban_board/kanban_board.py | 16 +++++++++++----- .../frappe/views/kanban/kanban_board.bundle.js | 14 +++----------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/frappe/desk/doctype/kanban_board/kanban_board.py b/frappe/desk/doctype/kanban_board/kanban_board.py index 83f0f46df0..c9eb48564d 100644 --- a/frappe/desk/doctype/kanban_board/kanban_board.py +++ b/frappe/desk/doctype/kanban_board/kanban_board.py @@ -88,6 +88,9 @@ def update_order(board_name, order): """Save the order of cards in columns""" board = frappe.get_doc("Kanban Board", board_name) doctype = board.reference_doctype + + frappe.has_permission(doctype, "write", throw=True) + fieldname = board.field_name order_dict = json.loads(order) @@ -103,8 +106,7 @@ def update_order(board_name, order): if column.column_name == col_name: column.order = json.dumps(cards) - board.save() - return board, updated_cards + return board.save(ignore_permissions=True), updated_cards @frappe.whitelist() @@ -114,6 +116,9 @@ def update_order_for_single_card( """Save the order of cards in columns""" board = frappe.get_doc("Kanban Board", board_name) doctype = board.reference_doctype + + frappe.has_permission(doctype, "write", throw=True) + fieldname = board.field_name old_index = frappe.parse_json(old_index) new_index = frappe.parse_json(new_index) @@ -130,7 +135,7 @@ def update_order_for_single_card( # save updated order board.columns[from_col_idx].order = frappe.as_json(from_col_order) board.columns[to_col_idx].order = frappe.as_json(to_col_order) - board.save() + board.save(ignore_permissions=True) # update changed value in doc frappe.set_value(doctype, docname, fieldname, to_colname) @@ -151,13 +156,14 @@ def get_kanban_column_order_and_index(board, colname): def add_card(board_name, docname, colname): board = frappe.get_doc("Kanban Board", board_name) + frappe.has_permission(board.reference_doctype, "write", throw=True) + col_order, col_idx = get_kanban_column_order_and_index(board, colname) col_order.insert(0, docname) board.columns[col_idx].order = frappe.as_json(col_order) - board.save() - return board + return board.save(ignore_permissions=True) @frappe.whitelist() diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index 52eb912c78..f8782e9f21 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -326,11 +326,7 @@ frappe.provide("frappe.views"); store.watch((state, getters) => { return state.empty_state; }, show_empty_state); - - if (self.board_perms.write) { - // If write access to Board, update Kanban cards order on load - store.dispatch("update_order"); - } + store.dispatch("update_order"); } function prepare() { @@ -338,13 +334,12 @@ frappe.provide("frappe.views"); if (self.$kanban_board.length === 0) { self.$kanban_board = $(frappe.render_template("kanban_board")); - // add column self.$kanban_board.appendTo(self.wrapper); } self.$filter_area = self.cur_list.$page.find(".active-tag-filters"); bind_events(); - setup_sortable(); // column + setup_sortable(); } function make_columns() { @@ -533,7 +528,7 @@ frappe.provide("frappe.views"); function init() { make_dom(); - setup_sortable(); // drag card + setup_sortable(); make_cards(); store.watch((state, getters) => { return state.cards; @@ -581,9 +576,6 @@ frappe.provide("frappe.views"); } function setup_sortable() { - // If no write access, editing board (by dragging card) should be blocked - if (!board_perms.write) return; - Sortable.create(self.$kanban_cards.get(0), { group: "cards", animation: 150, From d6bdd636dcc358a308ec72acade69af2875915b6 Mon Sep 17 00:00:00 2001 From: marination Date: Fri, 25 Nov 2022 12:09:36 +0530 Subject: [PATCH 06/18] fix: Check Reference Doctype perms & control indicator change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Don’t change indicator on filter change if user can’t write to board. They can’t save filters - Invoke `update_order` on Kanban board init() only if user has `write` access to reference doctype (non-deliberate invocation) - All deliberate invocations of `update_order` via UI actions are blocked/hidden without `write` access - Remove elements with no access instead of hiding to avoid inspect element hacks - Card Actions: Block card dragging if no `write` access to reference doctype - Card Actions: Block card adding if no `create` access to reference doctype --- frappe/client.py | 2 ++ .../views/kanban/kanban_board.bundle.js | 22 ++++++++++++++++--- .../js/frappe/views/kanban/kanban_view.js | 2 ++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/frappe/client.py b/frappe/client.py index afbb0df5ee..4dc118ea06 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -302,6 +302,7 @@ def has_permission(doctype, docname, perm_type="read"): # perm_type can be one of read, write, create, submit, cancel, report return {"has_permission": frappe.has_permission(doctype, perm_type.lower(), docname)} + @frappe.whitelist() def get_doc_permissions(doctype, docname): """Returns an evaluated document permissions dict like `{"read":1, "write":1}` @@ -312,6 +313,7 @@ def get_doc_permissions(doctype, docname): doc = frappe.get_doc(doctype, docname) return {"permissions": frappe.permissions.get_doc_permissions(doc)} + @frappe.whitelist() def get_password(doctype, name, fieldname): """Return a password type property. Only applicable for System Managers diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index f8782e9f21..651b94f28f 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -326,7 +326,12 @@ frappe.provide("frappe.views"); store.watch((state, getters) => { return state.empty_state; }, show_empty_state); - store.dispatch("update_order"); + + if (frappe.model.can_write(store.state.doctype)) { + // Check for reference doctype access before initiating + // non-deliberate action + store.dispatch("update_order"); + } } function prepare() { @@ -377,7 +382,7 @@ frappe.provide("frappe.views"); function bind_add_column() { if (!self.board_perms.write) { // If no write access, editing board (by adding column) should be blocked - self.$kanban_board.find(".add-new-column").hide(); + self.$kanban_board.find(".add-new-column").remove(); return; } @@ -576,6 +581,9 @@ frappe.provide("frappe.views"); } function setup_sortable() { + // Block card dragging/record editing without 'write' access + if (!frappe.model.can_write(store.state.doctype)) return; + Sortable.create(self.$kanban_cards.get(0), { group: "cards", animation: 150, @@ -609,6 +617,14 @@ frappe.provide("frappe.views"); var $wrapper = self.$kanban_column; var $btn_add = $wrapper.find(".add-card"); var $new_card_area = $wrapper.find(".new-card-area"); + + if (!frappe.model.can_create(store.state.doctype)) { + // Block record/card creation without 'create' access + $btn_add.remove(); + $new_card_area.remove(); + return; + } + var $textarea = $new_card_area.find("textarea"); //Add card button @@ -652,7 +668,7 @@ frappe.provide("frappe.views"); function bind_options() { if (!board_perms.write) { // If no write access, column options should be hidden - self.$kanban_column.find(".column-options").hide(); + self.$kanban_column.find(".column-options").remove(); return; } diff --git a/frappe/public/js/frappe/views/kanban/kanban_view.js b/frappe/public/js/frappe/views/kanban/kanban_view.js index 693326b062..f0b0cb8adf 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_view.js +++ b/frappe/public/js/frappe/views/kanban/kanban_view.js @@ -151,6 +151,8 @@ frappe.views.KanbanView = class KanbanView extends frappe.views.ListView { render_list() {} on_filter_change() { + if (!this.board_perms.write) return; // avoid misleading ux + if (JSON.stringify(this.board.filters_array) !== JSON.stringify(this.filter_area.get())) { this.page.set_indicator(__("Not Saved"), "orange"); } else { From 203e73dacb3677209dcf32ca6bb8f9bd0d7a1c09 Mon Sep 17 00:00:00 2001 From: marination Date: Fri, 25 Nov 2022 19:35:55 +0530 Subject: [PATCH 07/18] chore: Change card cursor to default from `grab` if no `write` access to Ref. Doctype --- .../js/frappe/views/kanban/kanban_board.bundle.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index 651b94f28f..fc93f2a910 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -362,7 +362,7 @@ frappe.provide("frappe.views"); } function setup_sortable() { - // If no write access, editing board (by dragging column) should be blocked + // If no write access to board, editing board (by dragging column) should be blocked if (!self.board_perms.write) return; var sortable = new Sortable(self.$kanban_board.get(0), { @@ -381,7 +381,7 @@ frappe.provide("frappe.views"); function bind_add_column() { if (!self.board_perms.write) { - // If no write access, editing board (by adding column) should be blocked + // If no write access to board, editing board (by adding column) should be blocked self.$kanban_board.find(".add-new-column").remove(); return; } @@ -581,7 +581,7 @@ frappe.provide("frappe.views"); } function setup_sortable() { - // Block card dragging/record editing without 'write' access + // Block card dragging/record editing without 'write' access to reference doctype if (!frappe.model.can_write(store.state.doctype)) return; Sortable.create(self.$kanban_cards.get(0), { @@ -619,7 +619,7 @@ frappe.provide("frappe.views"); var $new_card_area = $wrapper.find(".new-card-area"); if (!frappe.model.can_create(store.state.doctype)) { - // Block record/card creation without 'create' access + // Block record/card creation without 'create' access to reference doctype $btn_add.remove(); $new_card_area.remove(); return; @@ -667,7 +667,7 @@ frappe.provide("frappe.views"); function bind_options() { if (!board_perms.write) { - // If no write access, column options should be hidden + // If no write access to board, column options should be hidden self.$kanban_column.find(".column-options").remove(); return; } @@ -721,6 +721,11 @@ frappe.provide("frappe.views"); }; self.$card = $(frappe.render_template("kanban_card", opts)).appendTo(wrapper); + + if (!frappe.model.can_write(card.doctype)) { + // Undraggable card without 'write' access to reference doctype + self.$card.find(".kanban-card-body").css("cursor", "default"); + } } function get_doc_content(card) { From 5d0ad77d62c682ed77ebc4e9f2e954cb058d6626 Mon Sep 17 00:00:00 2001 From: marination Date: Fri, 23 Dec 2022 00:44:53 +0530 Subject: [PATCH 08/18] fix: Hide Menu if empty & render columns without state change - 'Refresh' btn is always present in dropdown menu. Show menu if any other action is also present. - Without `update_order` state is not changed, and thus columns are not rendered on `init()` - If we are not updating order in the db, render columns with whatever information we have currently --- .../js/frappe/views/kanban/kanban_board.bundle.js | 6 ++++-- frappe/public/js/frappe/views/kanban/kanban_view.js | 10 ++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index fc93f2a910..703f49f205 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -328,9 +328,11 @@ frappe.provide("frappe.views"); }, show_empty_state); if (frappe.model.can_write(store.state.doctype)) { - // Check for reference doctype access before initiating - // non-deliberate action + // Check for reference doctype access before trying to modify it's value store.dispatch("update_order"); + } else { + // Render columns without state change + make_columns(); } } diff --git a/frappe/public/js/frappe/views/kanban/kanban_view.js b/frappe/public/js/frappe/views/kanban/kanban_view.js index 7ba15d7270..fb15131394 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_view.js +++ b/frappe/public/js/frappe/views/kanban/kanban_view.js @@ -45,6 +45,16 @@ frappe.views.KanbanView = class KanbanView extends frappe.views.ListView { }); } + init() { + return super.init().then(() => { + let menu_length = this.page.menu.find(".dropdown-item").length; + if (menu_length === 1) { + // Only 'Refresh' (hidden) is present (always), dropdown is visibly empty + this.page.hide_menu(); + } + }); + } + setup_defaults() { return super.setup_defaults().then(() => { let get_board_name = () => { From ef75e0f9b847726b41c59e5e60d998fbe4997cfe Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 26 Dec 2022 16:01:11 +0530 Subject: [PATCH 09/18] fix: Dispatch `update_order` always. Handle perm-wise action in backend - Currently, `update_order` is also responsible for fetching fresh column order from db as well updating card order - In the .py file, return board doc if no write permission as that is used to render thr right columns order - Without it, stale order (before moving columns) is restored as there is no db call getting columns that were last stored --- frappe/desk/doctype/kanban_board/kanban_board.py | 6 ++++-- .../public/js/frappe/views/kanban/kanban_board.bundle.js | 8 +------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/frappe/desk/doctype/kanban_board/kanban_board.py b/frappe/desk/doctype/kanban_board/kanban_board.py index c9eb48564d..e3257e25be 100644 --- a/frappe/desk/doctype/kanban_board/kanban_board.py +++ b/frappe/desk/doctype/kanban_board/kanban_board.py @@ -88,13 +88,15 @@ def update_order(board_name, order): """Save the order of cards in columns""" board = frappe.get_doc("Kanban Board", board_name) doctype = board.reference_doctype + updated_cards = [] - frappe.has_permission(doctype, "write", throw=True) + if not frappe.has_permission(doctype, "write"): + # Return board data from db + return board, updated_cards fieldname = board.field_name order_dict = json.loads(order) - updated_cards = [] for col_name, cards in order_dict.items(): for card in cards: column = frappe.get_value(doctype, {"name": card}, fieldname) diff --git a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js index 703f49f205..1810101820 100644 --- a/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js +++ b/frappe/public/js/frappe/views/kanban/kanban_board.bundle.js @@ -327,13 +327,7 @@ frappe.provide("frappe.views"); return state.empty_state; }, show_empty_state); - if (frappe.model.can_write(store.state.doctype)) { - // Check for reference doctype access before trying to modify it's value - store.dispatch("update_order"); - } else { - // Render columns without state change - make_columns(); - } + store.dispatch("update_order"); } function prepare() { From d5c4af53d8d1475f9740b95b9b75faf153ed5ce0 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 27 Dec 2022 17:00:00 +0530 Subject: [PATCH 10/18] chore: Cleaner Cypress commands - Clearer APIs `add_role` and `remove_role` --- cypress/support/commands.js | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 20de7508c0..5862d85564 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -371,6 +371,42 @@ Cypress.Commands.add("update_doc", (doctype, docname, args) => { }); }); +Cypress.Commands.add("add_role", (user, role) => { + cy.window() + .its("frappe") + .then((frappe) => { + const session_user = frappe.session.user; + add_remove_role("add", user, role, session_user); + }); +}); + +Cypress.Commands.add("remove_role", (user, role) => { + cy.window() + .its("frappe") + .then((frappe) => { + const session_user = frappe.session.user; + add_remove_role("remove", user, role, session_user); + }); +}); + +const add_remove_role = (action, user, role, session_user) => { + if (session_user !== "Administrator") { + cy.call("logout"); + cy.login("Administrator"); + } + + cy.call("frappe.tests.ui_test_helpers.add_remove_role", { + action: action, + user: user, + role: role, + }); + + if (session_user !== "Administrator") { + cy.call("logout"); + cy.login(session_user); + } +}; + Cypress.Commands.add("open_list_filter", () => { cy.get(".filter-section .filter-button").click(); cy.wait(300); From 49143922c5100e8cba8876ce90bf353fbd94d40a Mon Sep 17 00:00:00 2001 From: marination Date: Wed, 28 Dec 2022 23:00:00 +0530 Subject: [PATCH 11/18] chore: Kanban and ToDo UI test helpers - `create_admin_kanban` and `create_todo` UI test helpers - `switch_to_user` Cypress command: logs out and logs in as specified user - Used `remove_role` in permissions test - Used `switch_to_user` command in test helper --- cypress/integration/permissions.js | 20 ++---------------- cypress/support/commands.js | 11 ++++++---- frappe/tests/ui_test_helpers.py | 34 ++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/cypress/integration/permissions.js b/cypress/integration/permissions.js index 7a13239771..4e371cc17f 100644 --- a/cypress/integration/permissions.js +++ b/cypress/integration/permissions.js @@ -1,16 +1,7 @@ context("Permissions API", () => { before(() => { cy.visit("/login"); - - cy.login("Administrator"); - cy.call("frappe.tests.ui_test_helpers.add_remove_role", { - action: "remove", - user: "frappe@example.com", - role: "System Manager", - }); - cy.call("logout"); - - cy.login("frappe@example.com"); + cy.remove_role("frappe@example.com", "System Manager"); cy.visit("/app"); }); @@ -44,14 +35,7 @@ context("Permissions API", () => { }); after(() => { - cy.call("logout"); - - cy.login("Administrator"); - cy.call("frappe.tests.ui_test_helpers.add_remove_role", { - action: "add", - user: "frappe@example.com", - role: "System Manager", - }); + cy.add_role("frappe@example.com", "System Manager"); cy.call("logout"); }); }); diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 5862d85564..0a25ff5cab 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -371,6 +371,11 @@ Cypress.Commands.add("update_doc", (doctype, docname, args) => { }); }); +Cypress.Commands.add("switch_to_user", (user) => { + cy.call("logout"); + cy.login(user); +}); + Cypress.Commands.add("add_role", (user, role) => { cy.window() .its("frappe") @@ -391,8 +396,7 @@ Cypress.Commands.add("remove_role", (user, role) => { const add_remove_role = (action, user, role, session_user) => { if (session_user !== "Administrator") { - cy.call("logout"); - cy.login("Administrator"); + cy.switch_to_user("Administrator"); } cy.call("frappe.tests.ui_test_helpers.add_remove_role", { @@ -402,8 +406,7 @@ const add_remove_role = (action, user, role, session_user) => { }); if (session_user !== "Administrator") { - cy.call("logout"); - cy.login(session_user); + cy.switch_to_user(session_user); } }; diff --git a/frappe/tests/ui_test_helpers.py b/frappe/tests/ui_test_helpers.py index 05d3dedad6..16a5688cec 100644 --- a/frappe/tests/ui_test_helpers.py +++ b/frappe/tests/ui_test_helpers.py @@ -581,6 +581,40 @@ def create_kanban(): ).insert() +@whitelist_for_tests +def create_todo(description): + frappe.get_doc({"doctype": "ToDo", "description": description}).insert() + + +@whitelist_for_tests +def create_admin_kanban(): + if not frappe.db.exists("Kanban Board", "Admin Kanban"): + frappe.get_doc( + { + "doctype": "Kanban Board", + "name": "Admin Kanban", + "owner": "Administrator", + "kanban_board_name": "Admin Kanban", + "reference_doctype": "ToDo", + "field_name": "status", + "private": 0, + "show_labels": 0, + "columns": [ + { + "column_name": "Open", + "status": "Active", + "indicator": "Gray", + }, + { + "column_name": "Closed", + "status": "Active", + "indicator": "Gray", + }, + ], + } + ).insert() + + @whitelist_for_tests def add_remove_role(action, user, role): user_doc = frappe.get_doc("User", user) From 5b319f0830a870655e7f53e1341c1a6072e26ada Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 29 Dec 2022 23:11:30 +0530 Subject: [PATCH 12/18] test: Check if Kanban Board is not editable by non-system user who does not own the board - User should only be able to view the board, and drag the cards - No column actions, no board actions --- cypress/integration/kanban.js | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/cypress/integration/kanban.js b/cypress/integration/kanban.js index 1b7e45ac55..04a72a9436 100644 --- a/cypress/integration/kanban.js +++ b/cypress/integration/kanban.js @@ -1,6 +1,6 @@ context("Kanban Board", () => { before(() => { - cy.login(); + cy.login("frappe@example.com"); cy.visit("/app"); }); @@ -96,4 +96,36 @@ context("Kanban Board", () => { .first() .should("not.contain", "ID:"); }); + + it("Checks if Kanban Board edits are blocked for non-System Manager and non-owner of the Board", () => { + // create admin kanban board + cy.call("frappe.tests.ui_test_helpers.create_todo", { description: "Frappe User ToDo" }); + + cy.switch_to_user("Administrator"); + cy.call("frappe.tests.ui_test_helpers.create_admin_kanban"); + // remove sys manager + cy.remove_role("frappe@example.com", "System Manager"); + + cy.switch_to_user("frappe@example.com"); + + cy.visit("/app/todo/view/kanban/Admin Kanban"); + + // Menu button should be hidden (dropdown for 'Save Filters' and 'Delete Kanban Board') + cy.get(".no-list-sidebar .menu-btn-group .btn-default[data-original-title='Menu']").should( + "have.length", + 0 + ); + // Kanban Columns should be visible (read-only) + cy.get(".kanban .kanban-column").should("have.length", 2); + // User should be able to add card (has access to ToDo) + cy.get(".kanban .add-card").should("have.length", 2); + // Column actions should be hidden (dropdown for 'Archive' and indicators) + cy.get(".kanban .column-options").should("have.length", 0); + + cy.add_role("frappe@example.com", "System Manager"); + }); + + after(() => { + cy.call("logout"); + }); }); From 3e7a572aa277b94a2842babeb51ae1fd5142eb2b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 4 Jan 2023 11:04:16 +0530 Subject: [PATCH 13/18] refactor: Deprecate db.set_value with None as docname --- frappe/database/database.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 5b775ccc10..26bb4c59d9 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -32,7 +32,7 @@ from frappe.model.utils.link_count import flush_local_link_count from frappe.query_builder.functions import Count from frappe.utils import cast as cast_fieldtype from frappe.utils import cint, get_datetime, get_table_name, getdate, now, sbool -from frappe.utils.deprecations import deprecated +from frappe.utils.deprecations import deprecated, deprecation_warning IFNULL_PATTERN = re.compile(r"ifnull\(", flags=re.IGNORECASE) INDEX_PATTERN = re.compile(r"\s*\([^)]+\)\s*") @@ -837,6 +837,11 @@ class Database: is_single_doctype = not (dn and dt != dn) to_update = field if isinstance(field, dict) else {field: val} + if dn is None: + deprecation_warning( + "Calling db.set_value with no document name assumes a single doctype. This behaviour will be removed in version 15. Use db.set_single_value instead." + ) + if update_modified: modified = modified or now() modified_by = modified_by or frappe.session.user From e00023deb171abf9f1a29fb9e3737b570a0df766 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 4 Jan 2023 11:13:53 +0530 Subject: [PATCH 14/18] refactor: replace bad usage of db.set_value --- frappe/core/doctype/domain/domain.py | 4 ++-- frappe/core/doctype/user/test_user.py | 20 +++++++++---------- frappe/desk/page/setup_wizard/setup_wizard.py | 4 ++-- .../dropbox_settings/dropbox_settings.py | 2 +- .../doctype/google_drive/google_drive.py | 11 +++++----- .../google_settings/test_google_settings.py | 12 +++++------ .../patches/v10_0/set_default_locking_time.py | 2 +- .../patches/v11_0/set_dropbox_file_backup.py | 2 +- .../v12_0/set_default_password_reset_limit.py | 2 +- .../v13_0/set_first_day_of_the_week.py | 2 +- .../v14_0/set_document_expiry_default.py | 3 +-- .../update_auto_account_deletion_duration.py | 2 +- frappe/translate.py | 2 +- frappe/twofactor.py | 2 +- frappe/utils/install.py | 2 +- frappe/utils/scheduler.py | 2 +- .../test_personal_data_deletion_request.py | 2 +- .../website_settings/google_indexing.py | 3 +-- 18 files changed, 37 insertions(+), 42 deletions(-) diff --git a/frappe/core/doctype/domain/domain.py b/frappe/core/doctype/domain/domain.py index e0b0e80982..1f5dbfa22e 100644 --- a/frappe/core/doctype/domain/domain.py +++ b/frappe/core/doctype/domain/domain.py @@ -85,8 +85,8 @@ class Domain(Document): def set_default_portal_role(self): """Set default portal role based on domain""" if self.data.get("default_portal_role"): - frappe.db.set_value( - "Portal Settings", None, "default_role", self.data.get("default_portal_role") + frappe.db.set_single_value( + "Portal Settings", "default_role", self.data.get("default_portal_role") ) def setup_properties(self): diff --git a/frappe/core/doctype/user/test_user.py b/frappe/core/doctype/user/test_user.py index ef4b171e8e..0c5badb21f 100644 --- a/frappe/core/doctype/user/test_user.py +++ b/frappe/core/doctype/user/test_user.py @@ -27,9 +27,9 @@ test_records = frappe.get_test_records("User") class TestUser(FrappeTestCase): def tearDown(self): # disable password strength test - frappe.db.set_value("System Settings", "System Settings", "enable_password_policy", 0) - frappe.db.set_value("System Settings", "System Settings", "minimum_password_score", "") - frappe.db.set_value("System Settings", "System Settings", "password_reset_limit", 3) + frappe.db.set_single_value("System Settings", "enable_password_policy", 0) + frappe.db.set_single_value("System Settings", "minimum_password_score", "") + frappe.db.set_single_value("System Settings", "password_reset_limit", 3) frappe.set_user("Administrator") def test_user_type(self): @@ -111,7 +111,7 @@ class TestUser(FrappeTestCase): self.assertEqual(frappe.db.get_value("User", "xxxtest@example.com"), None) - frappe.db.set_value("Website Settings", "Website Settings", "_test", "_test_val") + frappe.db.set_single_value("Website Settings", "_test", "_test_val") self.assertEqual(frappe.db.get_value("Website Settings", None, "_test"), "_test_val") self.assertEqual( frappe.db.get_value("Website Settings", "Website Settings", "_test"), "_test_val" @@ -179,15 +179,15 @@ class TestUser(FrappeTestCase): def test_password_strength(self): # Test Password without Password Strength Policy - frappe.db.set_value("System Settings", "System Settings", "enable_password_policy", 0) + frappe.db.set_single_value("System Settings", "enable_password_policy", 0) # password policy is disabled, test_password_strength should be ignored result = test_password_strength("test_password") self.assertFalse(result.get("feedback", None)) # Test Password with Password Strenth Policy Set - frappe.db.set_value("System Settings", "System Settings", "enable_password_policy", 1) - frappe.db.set_value("System Settings", "System Settings", "minimum_password_score", 2) + frappe.db.set_single_value("System Settings", "enable_password_policy", 1) + frappe.db.set_single_value("System Settings", "minimum_password_score", 2) # Score 1; should now fail result = test_password_strength("bee2ve") @@ -275,7 +275,7 @@ class TestUser(FrappeTestCase): def test_rate_limiting_for_reset_password(self): # Allow only one reset request for a day - frappe.db.set_value("System Settings", "System Settings", "password_reset_limit", 1) + frappe.db.set_single_value("System Settings", "password_reset_limit", 1) frappe.db.commit() url = get_url() @@ -443,9 +443,7 @@ class TestUser(FrappeTestCase): def test_reset_password_link_expiry(self): new_password = "new_password" # set the reset password expiry to 1 second - frappe.db.set_value( - "System Settings", "System Settings", "reset_password_link_expiry_duration", 1 - ) + frappe.db.set_single_value("System Settings", "reset_password_link_expiry_duration", 1) frappe.set_user("testpassword@example.com") test_user = frappe.get_doc("User", "testpassword@example.com") test_user.reset_password() diff --git a/frappe/desk/page/setup_wizard/setup_wizard.py b/frappe/desk/page/setup_wizard/setup_wizard.py index 408776fcb9..4d4d76207a 100755 --- a/frappe/desk/page/setup_wizard/setup_wizard.py +++ b/frappe/desk/page/setup_wizard/setup_wizard.py @@ -267,10 +267,10 @@ def add_all_roles_to(name): def disable_future_access(): frappe.db.set_default("desktop:home_page", "workspace") - frappe.db.set_value("System Settings", "System Settings", "setup_complete", 1) + frappe.db.set_single_value("System Settings", "setup_complete", 1) # Enable onboarding after install - frappe.db.set_value("System Settings", "System Settings", "enable_onboarding", 1) + frappe.db.set_single_value("System Settings", "enable_onboarding", 1) if not frappe.flags.in_test: # remove all roles and add 'Administrator' to prevent future access diff --git a/frappe/integrations/doctype/dropbox_settings/dropbox_settings.py b/frappe/integrations/doctype/dropbox_settings/dropbox_settings.py index dc9db2ccda..e6998a9d6d 100644 --- a/frappe/integrations/doctype/dropbox_settings/dropbox_settings.py +++ b/frappe/integrations/doctype/dropbox_settings/dropbox_settings.py @@ -393,7 +393,7 @@ def dropbox_auth_finish(return_access_token=False): def set_dropbox_access_token(access_token): - frappe.db.set_value("Dropbox Settings", None, "dropbox_access_token", access_token) + frappe.db.set_single_value("Dropbox Settings", "dropbox_access_token", access_token) frappe.db.commit() diff --git a/frappe/integrations/doctype/google_drive/google_drive.py b/frappe/integrations/doctype/google_drive/google_drive.py index 6ea1294cb0..9315e868fe 100644 --- a/frappe/integrations/doctype/google_drive/google_drive.py +++ b/frappe/integrations/doctype/google_drive/google_drive.py @@ -54,7 +54,7 @@ def authorize_access(reauthorize=False, code=None): if not oauth_code or reauthorize: if reauthorize: - frappe.db.set_value("Google Drive", None, "backup_folder_id", "") + frappe.db.set_single_value("Google Drive", "backup_folder_id", "") return oauth_obj.get_authentication_url( { "redirect": f"/app/Form/{quote('Google Drive')}", @@ -62,8 +62,7 @@ def authorize_access(reauthorize=False, code=None): ) r = oauth_obj.authorize(oauth_code) - frappe.db.set_value( - "Google Drive", + frappe.db.set_single_value( "Google Drive", {"authorization_code": oauth_code, "refresh_token": r.get("refresh_token")}, ) @@ -95,7 +94,7 @@ def check_for_folder_in_google_drive(): try: folder = google_drive.files().create(body=file_metadata, fields="id").execute() - frappe.db.set_value("Google Drive", None, "backup_folder_id", folder.get("id")) + frappe.db.set_single_value("Google Drive", "backup_folder_id", folder.get("id")) frappe.db.commit() except HttpError as e: frappe.throw( @@ -120,7 +119,7 @@ def check_for_folder_in_google_drive(): for f in google_drive_folders.get("files"): if f.get("name") == account.backup_folder_name: - frappe.db.set_value("Google Drive", None, "backup_folder_id", f.get("id")) + frappe.db.set_single_value("Google Drive", "backup_folder_id", f.get("id")) frappe.db.commit() backup_folder_exists = True break @@ -186,7 +185,7 @@ def upload_system_backup_to_google_drive(): send_email(False, "Google Drive", "Google Drive", "email", error_status=e) set_progress(3, "Uploading successful.") - frappe.db.set_value("Google Drive", None, "last_backup_on", frappe.utils.now_datetime()) + frappe.db.set_single_value("Google Drive", "last_backup_on", frappe.utils.now_datetime()) send_email(True, "Google Drive", "Google Drive", "email") return _("Google Drive Backup Successful.") diff --git a/frappe/integrations/doctype/google_settings/test_google_settings.py b/frappe/integrations/doctype/google_settings/test_google_settings.py index d67d7cf40b..d4bb830779 100644 --- a/frappe/integrations/doctype/google_settings/test_google_settings.py +++ b/frappe/integrations/doctype/google_settings/test_google_settings.py @@ -17,24 +17,24 @@ class TestGoogleSettings(FrappeTestCase): def test_picker_disabled(self): """Google Drive Picker should be disabled if it is not enabled in Google Settings.""" - frappe.db.set_value("Google Settings", None, "enable", 1) - frappe.db.set_value("Google Settings", None, "google_drive_picker_enabled", 0) + frappe.db.set_single_value("Google Settings", "enable", 1) + frappe.db.set_single_value("Google Settings", "google_drive_picker_enabled", 0) settings = get_file_picker_settings() self.assertEqual(settings, {}) def test_google_disabled(self): """Google Drive Picker should be disabled if Google integration is not enabled.""" - frappe.db.set_value("Google Settings", None, "enable", 0) - frappe.db.set_value("Google Settings", None, "google_drive_picker_enabled", 1) + frappe.db.set_single_value("Google Settings", "enable", 0) + frappe.db.set_single_value("Google Settings", "google_drive_picker_enabled", 1) settings = get_file_picker_settings() self.assertEqual(settings, {}) def test_picker_enabled(self): """If picker is enabled, get_file_picker_settings should return the credentials.""" - frappe.db.set_value("Google Settings", None, "enable", 1) - frappe.db.set_value("Google Settings", None, "google_drive_picker_enabled", 1) + frappe.db.set_single_value("Google Settings", "enable", 1) + frappe.db.set_single_value("Google Settings", "google_drive_picker_enabled", 1) settings = get_file_picker_settings() self.assertEqual(True, settings.get("enabled", False)) diff --git a/frappe/patches/v10_0/set_default_locking_time.py b/frappe/patches/v10_0/set_default_locking_time.py index 7c9f3ceff1..4eb120873d 100644 --- a/frappe/patches/v10_0/set_default_locking_time.py +++ b/frappe/patches/v10_0/set_default_locking_time.py @@ -6,4 +6,4 @@ import frappe def execute(): frappe.reload_doc("core", "doctype", "system_settings") - frappe.db.set_value("System Settings", None, "allow_login_after_fail", 60) + frappe.db.set_single_value("System Settings", "allow_login_after_fail", 60) diff --git a/frappe/patches/v11_0/set_dropbox_file_backup.py b/frappe/patches/v11_0/set_dropbox_file_backup.py index 396491e8b3..5770c55bf4 100644 --- a/frappe/patches/v11_0/set_dropbox_file_backup.py +++ b/frappe/patches/v11_0/set_dropbox_file_backup.py @@ -6,4 +6,4 @@ def execute(): frappe.reload_doctype("Dropbox Settings") check_dropbox_enabled = cint(frappe.db.get_single_value("Dropbox Settings", "enabled")) if check_dropbox_enabled == 1: - frappe.db.set_value("Dropbox Settings", None, "file_backup", 1) + frappe.db.set_single_value("Dropbox Settings", "file_backup", 1) diff --git a/frappe/patches/v12_0/set_default_password_reset_limit.py b/frappe/patches/v12_0/set_default_password_reset_limit.py index 9e29e75c36..98a397b606 100644 --- a/frappe/patches/v12_0/set_default_password_reset_limit.py +++ b/frappe/patches/v12_0/set_default_password_reset_limit.py @@ -6,4 +6,4 @@ import frappe def execute(): frappe.reload_doc("core", "doctype", "system_settings", force=1) - frappe.db.set_value("System Settings", None, "password_reset_limit", 3) + frappe.db.set_single_value("System Settings", "password_reset_limit", 3) diff --git a/frappe/patches/v13_0/set_first_day_of_the_week.py b/frappe/patches/v13_0/set_first_day_of_the_week.py index 165ec3c42b..a48ca09672 100644 --- a/frappe/patches/v13_0/set_first_day_of_the_week.py +++ b/frappe/patches/v13_0/set_first_day_of_the_week.py @@ -5,4 +5,4 @@ def execute(): frappe.reload_doctype("System Settings") # setting first_day_of_the_week value as "Monday" to avoid breaking change # because before the configuration was introduced, system used to consider "Monday" as start of the week - frappe.db.set_value("System Settings", "System Settings", "first_day_of_the_week", "Monday") + frappe.db.set_single_value("System Settings", "first_day_of_the_week", "Monday") diff --git a/frappe/patches/v14_0/set_document_expiry_default.py b/frappe/patches/v14_0/set_document_expiry_default.py index 59a9db6c4d..0b193bcd9b 100644 --- a/frappe/patches/v14_0/set_document_expiry_default.py +++ b/frappe/patches/v14_0/set_document_expiry_default.py @@ -2,8 +2,7 @@ import frappe def execute(): - frappe.db.set_value( - "System Settings", + frappe.db.set_single_value( "System Settings", {"document_share_key_expiry": 30, "allow_older_web_view_links": 1}, ) diff --git a/frappe/patches/v14_0/update_auto_account_deletion_duration.py b/frappe/patches/v14_0/update_auto_account_deletion_duration.py index 6b01816092..ed7b3f21fa 100644 --- a/frappe/patches/v14_0/update_auto_account_deletion_duration.py +++ b/frappe/patches/v14_0/update_auto_account_deletion_duration.py @@ -3,4 +3,4 @@ import frappe def execute(): days = frappe.db.get_single_value("Website Settings", "auto_account_deletion") - frappe.db.set_value("Website Settings", None, "auto_account_deletion", days * 24) + frappe.db.set_single_value("Website Settings", "auto_account_deletion", days * 24) diff --git a/frappe/translate.py b/frappe/translate.py index 0d6e49114e..cb71510735 100644 --- a/frappe/translate.py +++ b/frappe/translate.py @@ -1176,7 +1176,7 @@ def rename_language(old_name, new_name): language_in_system_settings = frappe.db.get_single_value("System Settings", "language") if language_in_system_settings == old_name: - frappe.db.set_value("System Settings", "System Settings", "language", new_name) + frappe.db.set_single_value("System Settings", "language", new_name) frappe.db.sql( """update `tabUser` set language=%(new_name)s where language=%(old_name)s""", diff --git a/frappe/twofactor.py b/frappe/twofactor.py index e432e539b0..8ad02f0b5a 100644 --- a/frappe/twofactor.py +++ b/frappe/twofactor.py @@ -446,7 +446,7 @@ def should_remove_barcode_image(barcode): def disable(): - frappe.db.set_value("System Settings", None, "enable_two_factor_auth", 0) + frappe.db.set_single_value("System Settings", "enable_two_factor_auth", 0) @frappe.whitelist() diff --git a/frappe/utils/install.py b/frappe/utils/install.py index cd15c957f9..caac5744e8 100644 --- a/frappe/utils/install.py +++ b/frappe/utils/install.py @@ -169,7 +169,7 @@ def before_tests(): if not int(frappe.db.get_single_value("System Settings", "setup_complete") or 0): complete_setup_wizard() - frappe.db.set_value("Website Settings", "Website Settings", "disable_signup", 0) + frappe.db.set_single_value("Website Settings", "disable_signup", 0) frappe.db.commit() frappe.clear_cache() diff --git a/frappe/utils/scheduler.py b/frappe/utils/scheduler.py index 6a8cf8b5a9..03c1b37a43 100755 --- a/frappe/utils/scheduler.py +++ b/frappe/utils/scheduler.py @@ -121,7 +121,7 @@ def is_scheduler_disabled() -> bool: def toggle_scheduler(enable): - frappe.db.set_value("System Settings", None, "enable_scheduler", 1 if enable else 0) + frappe.db.set_single_value("System Settings", "enable_scheduler", int(enable)) def enable_scheduler(): diff --git a/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py b/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py index 7861d865b7..a892b6f379 100644 --- a/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py +++ b/frappe/website/doctype/personal_data_deletion_request/test_personal_data_deletion_request.py @@ -59,7 +59,7 @@ class TestPersonalDataDeletionRequest(FrappeTestCase): self.assertFalse(frappe.db.exists("Personal Data Deletion Request", self.delete_request.name)) def test_process_auto_request(self): - frappe.db.set_value("Website Settings", None, "auto_account_deletion", "1") + frappe.db.set_single_value("Website Settings", "auto_account_deletion", "1") date_time_obj = datetime.strptime( self.delete_request.creation, "%Y-%m-%d %H:%M:%S.%f" ) + timedelta(hours=-2) diff --git a/frappe/website/doctype/website_settings/google_indexing.py b/frappe/website/doctype/website_settings/google_indexing.py index 2606206056..88560448c1 100644 --- a/frappe/website/doctype/website_settings/google_indexing.py +++ b/frappe/website/doctype/website_settings/google_indexing.py @@ -31,8 +31,7 @@ def authorize_access(reauthorize=False, code=None): ) res = oauth_obj.authorize(oauth_code) - frappe.db.set_value( - "Website Settings", + frappe.db.set_single_value( "Website Settings", {"indexing_authorization_code": oauth_code, "indexing_refresh_token": res.get("refresh_token")}, ) From d0f880e5c5bdd1c303971b8dd37f44f41219a951 Mon Sep 17 00:00:00 2001 From: Alfredo Altamirano <8353891+Ahuahuachi@users.noreply.github.com> Date: Wed, 4 Jan 2023 01:03:45 -0600 Subject: [PATCH 15/18] fix: Improve JSON export format readability (#19429) * Improve JSON export format readability * fix: Enable ensure_ascii flag on export_json * style: fmt [skip ci] Co-authored-by: Alfredo Altamirano Co-authored-by: Ankush Menat --- frappe/__init__.py | 17 ++++++++++++++--- frappe/core/doctype/data_import/data_import.py | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 54f27ec5b8..da6dde08d8 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1907,7 +1907,7 @@ def get_value(*args, **kwargs): return db.get_value(*args, **kwargs) -def as_json(obj: dict | list, indent=1, separators=None) -> str: +def as_json(obj: dict | list, indent=1, separators=None, ensure_ascii=True) -> str: from frappe.utils.response import json_handler if separators is None: @@ -1915,13 +1915,24 @@ def as_json(obj: dict | list, indent=1, separators=None) -> str: try: return json.dumps( - obj, indent=indent, sort_keys=True, default=json_handler, separators=separators + obj, + indent=indent, + sort_keys=True, + default=json_handler, + separators=separators, + ensure_ascii=ensure_ascii, ) except TypeError: # this would break in case the keys are not all os "str" type - as defined in the JSON # adding this to ensure keys are sorted (expected behaviour) sorted_obj = dict(sorted(obj.items(), key=lambda kv: str(kv[0]))) - return json.dumps(sorted_obj, indent=indent, default=json_handler, separators=separators) + return json.dumps( + sorted_obj, + indent=indent, + default=json_handler, + separators=separators, + ensure_ascii=ensure_ascii, + ) def are_emails_muted(): diff --git a/frappe/core/doctype/data_import/data_import.py b/frappe/core/doctype/data_import/data_import.py index c055524fd1..afa6f3d0fa 100644 --- a/frappe/core/doctype/data_import/data_import.py +++ b/frappe/core/doctype/data_import/data_import.py @@ -260,7 +260,7 @@ def export_json(doctype, path, filters=None, or_filters=None, name=None, order_b path = os.path.join("..", path) with open(path, "w") as outfile: - outfile.write(frappe.as_json(out)) + outfile.write(frappe.as_json(out, ensure_ascii=False)) def export_csv(doctype, path): From cfc009f21ae12c89091774676141783c5ba6c62d Mon Sep 17 00:00:00 2001 From: gavin Date: Thu, 5 Jan 2023 10:50:23 +0530 Subject: [PATCH 16/18] feat: Useful Meta.__repr__ (#19479) --- frappe/core/doctype/docfield/docfield.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/frappe/core/doctype/docfield/docfield.py b/frappe/core/doctype/docfield/docfield.py index 9fe55df5fe..96dbfee4cb 100644 --- a/frappe/core/doctype/docfield/docfield.py +++ b/frappe/core/doctype/docfield/docfield.py @@ -29,3 +29,12 @@ class DocField(Document): if self.fieldtype == "Select": options = self.options or "" return [d for d in options.split("\n") if d] + + def __repr__(self): + unsaved = "unsaved" if not self.name else "" + doctype = self.__class__.__name__ + + docstatus = f" docstatus={self.docstatus}" if self.docstatus else "" + parent = f" parent={self.parent}" if getattr(self, "parent", None) else "" + + return f"<{self.fieldtype}{doctype}: {self.fieldname}{docstatus}{parent}{unsaved}>" From c26b40114000a7111efc6838279c016e10dd9fff Mon Sep 17 00:00:00 2001 From: gavin Date: Thu, 5 Jan 2023 11:46:53 +0530 Subject: [PATCH 17/18] chore: Remove duplicated Oauth2 whitelisted APIs (#19465) Maintain routes via the `override_whitelisted_methods` hook --- frappe/hooks.py | 9 +++++++++ frappe/www/login.py | 33 +-------------------------------- 2 files changed, 10 insertions(+), 32 deletions(-) diff --git a/frappe/hooks.py b/frappe/hooks.py index beff7d2de2..9ad95e796b 100644 --- a/frappe/hooks.py +++ b/frappe/hooks.py @@ -356,6 +356,7 @@ global_search_doctypes = { } override_whitelisted_methods = { + # Legacy File APIs "frappe.core.doctype.file.file.download_file": "download_file", "frappe.core.doctype.file.file.unzip_file": "frappe.core.api.file.unzip_file", "frappe.core.doctype.file.file.get_attached_images": "frappe.core.api.file.get_attached_images", @@ -365,6 +366,14 @@ override_whitelisted_methods = { "frappe.core.doctype.file.file.create_new_folder": "frappe.core.api.file.create_new_folder", "frappe.core.doctype.file.file.move_file": "frappe.core.api.file.move_file", "frappe.core.doctype.file.file.zip_files": "frappe.core.api.file.zip_files", + # Legacy (& Consistency) OAuth2 APIs + "frappe.www.login.login_via_google": "frappe.integrations.oauth2_logins.login_via_google", + "frappe.www.login.login_via_github": "frappe.integrations.oauth2_logins.login_via_github", + "frappe.www.login.login_via_facebook": "frappe.integrations.oauth2_logins.login_via_facebook", + "frappe.www.login.login_via_frappe": "frappe.integrations.oauth2_logins.login_via_frappe", + "frappe.www.login.login_via_office365": "frappe.integrations.oauth2_logins.login_via_office365", + "frappe.www.login.login_via_salesforce": "frappe.integrations.oauth2_logins.login_via_salesforce", + "frappe.www.login.login_via_fairlogin": "frappe.integrations.oauth2_logins.login_via_fairlogin", } ignore_links_on_delete = [ diff --git a/frappe/www/login.py b/frappe/www/login.py index 347c0fe8d3..97ceb01c6e 100644 --- a/frappe/www/login.py +++ b/frappe/www/login.py @@ -11,13 +11,7 @@ from frappe.rate_limiter import rate_limit from frappe.utils import cint, get_url from frappe.utils.html_utils import get_icon_html from frappe.utils.jinja import guess_is_path -from frappe.utils.oauth import ( - get_oauth2_authorize_url, - get_oauth_keys, - login_via_oauth2, - login_via_oauth2_id_token, - redirect_post_login, -) +from frappe.utils.oauth import get_oauth2_authorize_url, get_oauth_keys, redirect_post_login from frappe.utils.password import get_decrypted_password from frappe.website.utils import get_home_page @@ -108,31 +102,6 @@ def get_context(context): return context -@frappe.whitelist(allow_guest=True) -def login_via_google(code: str, state: str): - login_via_oauth2("google", code, state, decoder=decoder_compat) - - -@frappe.whitelist(allow_guest=True) -def login_via_github(code: str, state: str): - login_via_oauth2("github", code, state) - - -@frappe.whitelist(allow_guest=True) -def login_via_facebook(code: str, state: str): - login_via_oauth2("facebook", code, state, decoder=decoder_compat) - - -@frappe.whitelist(allow_guest=True) -def login_via_frappe(code: str, state: str): - login_via_oauth2("frappe", code, state, decoder=decoder_compat) - - -@frappe.whitelist(allow_guest=True) -def login_via_office365(code: str, state: str): - login_via_oauth2_id_token("office_365", code, state, decoder=decoder_compat) - - @frappe.whitelist(allow_guest=True) def login_via_token(login_token: str): sid = frappe.cache().get_value(f"login_token:{login_token}", expires=True) From 28375e8e6e5e1dddf1f98c824feab4362b6266ce Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 5 Jan 2023 13:04:08 +0530 Subject: [PATCH 18/18] refactor: deprecate db.set_value on singles completely (#19481) --- frappe/database/database.py | 94 ++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 33 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 26bb4c59d9..5b8be7d6e2 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -689,13 +689,30 @@ class Database: def get_list(*args, **kwargs): return frappe.get_list(*args, **kwargs) + @staticmethod + def _get_update_dict( + fieldname: str | dict, value: Any, *, modified: str, modified_by: str, update_modified: bool + ) -> dict[str, Any]: + """Create update dict that represents column-values to be updated.""" + update_dict = fieldname if isinstance(fieldname, dict) else {fieldname: value} + + if update_modified: + modified = modified or now() + modified_by = modified_by or frappe.session.user + update_dict.update({"modified": modified, "modified_by": modified_by}) + + return update_dict + def set_single_value( self, doctype: str, fieldname: str | dict, value: str | int | None = None, - *args, - **kwargs, + *, + modified=None, + modified_by=None, + update_modified=True, + debug=False, ): """Set field value of Single DocType. @@ -708,7 +725,23 @@ class Database: # Update the `deny_multiple_sessions` field in System Settings DocType. company = frappe.db.set_single_value("System Settings", "deny_multiple_sessions", True) """ - return self.set_value(doctype, doctype, fieldname, value, *args, **kwargs) + + to_update = self._get_update_dict( + fieldname, value, modified=modified, modified_by=modified_by, update_modified=update_modified + ) + + frappe.db.delete( + "Singles", filters={"field": ("in", tuple(to_update)), "doctype": doctype}, debug=debug + ) + + singles_data = ((doctype, key, sbool(value)) for key, value in to_update.items()) + frappe.qb.into("Singles").columns("doctype", "field", "value").insert(*singles_data).run( + debug=debug + ) + frappe.clear_document_cache(doctype, doctype) + + if doctype in self.value_cache: + del self.value_cache[doctype] def get_single_value(self, doctype, fieldname, cache=True): """Get property of Single DocType. Cache locally by default @@ -834,45 +867,40 @@ class Database: :param update_modified: default True. Set as false, if you don't want to update the timestamp. :param debug: Print the query in the developer / js console. """ - is_single_doctype = not (dn and dt != dn) - to_update = field if isinstance(field, dict) else {field: val} - if dn is None: + if _is_single_doctype := not (dn and dt != dn): deprecation_warning( - "Calling db.set_value with no document name assumes a single doctype. This behaviour will be removed in version 15. Use db.set_single_value instead." + "Calling db.set_value on single doctype is deprecated. This behaviour will be removed in version 15. Use db.set_single_value instead." ) - - if update_modified: - modified = modified or now() - modified_by = modified_by or frappe.session.user - to_update.update({"modified": modified, "modified_by": modified_by}) - - if is_single_doctype: - frappe.db.delete( - "Singles", filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug + self.set_single_value( + doctype=dt, + fieldname=field, + value=val, + debug=debug, + update_modified=update_modified, + modified=modified, + modified_by=modified_by, ) + return - singles_data = ((dt, key, sbool(value)) for key, value in to_update.items()) - query = ( - frappe.qb.into("Singles").columns("doctype", "field", "value").insert(*singles_data) - ).run(debug=debug) - frappe.clear_document_cache(dt, dt) + to_update = self._get_update_dict( + field, val, modified=modified, modified_by=modified_by, update_modified=update_modified + ) + query = frappe.qb.engine.build_conditions(table=dt, filters=dn, update=True) + + if isinstance(dn, str): + frappe.clear_document_cache(dt, dn) else: - query = frappe.qb.engine.build_conditions(table=dt, filters=dn, update=True) + # TODO: Fix this; doesn't work rn - gavin@frappe.io + # frappe.cache().hdel_keys(dt, "document_cache") + # Workaround: clear all document caches + frappe.cache().delete_value("document_cache") - if isinstance(dn, str): - frappe.clear_document_cache(dt, dn) - else: - # TODO: Fix this; doesn't work rn - gavin@frappe.io - # frappe.cache().hdel_keys(dt, "document_cache") - # Workaround: clear all document caches - frappe.cache().delete_value("document_cache") + for column, value in to_update.items(): + query = query.set(column, value) - for column, value in to_update.items(): - query = query.set(column, value) - - query.run(debug=debug) + query.run(debug=debug) if dt in self.value_cache: del self.value_cache[dt]