From 7d091c19575a28f28803f2062a840d9c88d81432 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 19 Oct 2020 12:04:03 +0530 Subject: [PATCH 1/8] fix: Move attachment limit validation to file uploader --- frappe/public/js/frappe/file_uploader/index.js | 6 ++++++ frappe/public/js/frappe/form/form.js | 6 +----- frappe/public/js/frappe/form/sidebar/attachments.js | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/frappe/public/js/frappe/file_uploader/index.js b/frappe/public/js/frappe/file_uploader/index.js index 62a7bff822..5336cd7c52 100644 --- a/frappe/public/js/frappe/file_uploader/index.js +++ b/frappe/public/js/frappe/file_uploader/index.js @@ -15,7 +15,13 @@ export default class FileUploader { allow_multiple, as_dataurl, disable_file_browser, + frm } = {}) { + if (frm && frm.attachments.max_reached()) { + frappe.throw(__("Maximum Attachment Limit for this record reached.")); + return; + } + if (!wrapper) { this.make_dialog(); } else { diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index bb9e8c22d1..90b628f269 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -232,14 +232,10 @@ frappe.ui.form.Form = class FrappeForm { throw "attach error"; } - if(me.attachments.max_reached()) { - frappe.msgprint(__("Maximum Attachment Limit for this record reached.")); - throw "attach error"; - } - new frappe.ui.FileUploader({ doctype: me.doctype, docname: me.docname, + frm: me, files: dataTransfer.files, folder: 'Home/Attachments', on_success(file_doc) { diff --git a/frappe/public/js/frappe/form/sidebar/attachments.js b/frappe/public/js/frappe/form/sidebar/attachments.js index 165527e281..dc0c839737 100644 --- a/frappe/public/js/frappe/form/sidebar/attachments.js +++ b/frappe/public/js/frappe/form/sidebar/attachments.js @@ -140,7 +140,6 @@ frappe.ui.form.Attachments = Class.extend({ }); }, new_attachment: function(fieldname) { - var me = this; if (this.dialog) { // remove upload dialog this.dialog.$wrapper.remove(); @@ -149,6 +148,7 @@ frappe.ui.form.Attachments = Class.extend({ new frappe.ui.FileUploader({ doctype: this.frm.doctype, docname: this.frm.docname, + frm: this.frm, folder: 'Home/Attachments', on_success: (file_doc) => { this.attachment_uploaded(file_doc); From b33461bb53f05233017d27f19d83846c59e235e4 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 19 Oct 2020 12:04:57 +0530 Subject: [PATCH 2/8] feat: Add server-side validation for attachment limit --- frappe/core/doctype/file/file.py | 21 +++++++++++++++++++++ frappe/exceptions.py | 1 + 2 files changed, 22 insertions(+) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index b8bed89a4d..9b0b3f3fd1 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -93,6 +93,7 @@ class File(Document): self.set_is_private() self.set_file_name() self.validate_duplicate_entry() + self.validate_attachment_limit() self.validate_folder() if not self.file_url and not self.flags.ignore_file_validate: @@ -140,6 +141,26 @@ class File(Document): if self.file_url and (self.is_private != self.file_url.startswith('/private')): frappe.throw(_('Invalid file URL. Please contact System Administrator.')) + def validate_attachment_limit(self): + attachment_limit = 0 + if self.attached_to_doctype and self.attached_to_name: + attachment_limit = frappe.get_meta(self.attached_to_doctype).max_attachments + + if attachment_limit: + current_attachment_count = len(frappe.get_all('File', filters={ + 'attached_to_doctype': self.attached_to_doctype, + 'attached_to_name': self.attached_to_name, + }, limit=(attachment_limit + 1))) + + if current_attachment_count >= attachment_limit: + frappe.throw(_("Attachment Limit reached for {0} {1}.").format( + self.attached_to_doctype, + self.attached_to_name + ), + exc=frappe.exceptions.AttachmentLimitReached, + title=_('Maximum Attachment Limit Reached') + ) + def set_folder_name(self): """Make parent folders if not exists based on reference doctype and name""" if self.attached_to_doctype and not self.folder: diff --git a/frappe/exceptions.py b/frappe/exceptions.py index 267f5410af..81fccf5880 100644 --- a/frappe/exceptions.py +++ b/frappe/exceptions.py @@ -106,6 +106,7 @@ class InvalidDates(ValidationError): pass class DataTooLongException(ValidationError): pass class FileAlreadyAttachedException(Exception): pass class DocumentAlreadyRestored(Exception): pass +class AttachmentLimitReached(Exception): pass # OAuth exceptions class InvalidAuthorizationHeader(CSRFTokenError): pass class InvalidAuthorizationPrefix(CSRFTokenError): pass From 23efc9919ab24c58d60256a02fb067e5e11ff9d0 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 19 Oct 2020 12:37:02 +0530 Subject: [PATCH 3/8] fix: Update validation message --- frappe/core/doctype/file/file.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 9b0b3f3fd1..bb7765e055 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -153,12 +153,12 @@ class File(Document): }, limit=(attachment_limit + 1))) if current_attachment_count >= attachment_limit: - frappe.throw(_("Attachment Limit reached for {0} {1}.").format( + frappe.throw(_("Maximum Attachment Limit reached for {0} {1}.").format( self.attached_to_doctype, self.attached_to_name ), exc=frappe.exceptions.AttachmentLimitReached, - title=_('Maximum Attachment Limit Reached') + title=_('Attachment Limit Reached') ) def set_folder_name(self): From a2471ace211831c644454f13df8a4b384b251af9 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 19 Oct 2020 12:43:14 +0530 Subject: [PATCH 4/8] fix: Update attachment limit --- frappe/public/js/frappe/file_uploader/index.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/file_uploader/index.js b/frappe/public/js/frappe/file_uploader/index.js index 5336cd7c52..36de853b8f 100644 --- a/frappe/public/js/frappe/file_uploader/index.js +++ b/frappe/public/js/frappe/file_uploader/index.js @@ -18,7 +18,10 @@ export default class FileUploader { frm } = {}) { if (frm && frm.attachments.max_reached()) { - frappe.throw(__("Maximum Attachment Limit for this record reached.")); + frappe.throw({ + title: __("Attachment Limit Reached"), + message: __("Maximum attachment limit for this record reached."), + }); return; } From 42fc8fb25c8ec59d99468d99a316518aff62f158 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 19 Oct 2020 13:34:04 +0530 Subject: [PATCH 5/8] fix: Convert limit to integer --- frappe/core/doctype/file/file.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index bb7765e055..a8a999ff9a 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -144,13 +144,13 @@ class File(Document): def validate_attachment_limit(self): attachment_limit = 0 if self.attached_to_doctype and self.attached_to_name: - attachment_limit = frappe.get_meta(self.attached_to_doctype).max_attachments + attachment_limit = cint(frappe.get_meta(self.attached_to_doctype).max_attachments) if attachment_limit: current_attachment_count = len(frappe.get_all('File', filters={ 'attached_to_doctype': self.attached_to_doctype, 'attached_to_name': self.attached_to_name, - }, limit=(attachment_limit + 1))) + }, limit=attachment_limit + 1)) if current_attachment_count >= attachment_limit: frappe.throw(_("Maximum Attachment Limit reached for {0} {1}.").format( From e64483ad89b7d6af385256aaccb9a8acbcff3b0a Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 19 Oct 2020 13:35:16 +0530 Subject: [PATCH 6/8] test: Add test for attachment limit validation --- frappe/core/doctype/file/test_file.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index 85397ea1ee..cb44d33781 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -160,6 +160,29 @@ class TestSameContent(unittest.TestCase): def test_saved_content(self): self.assertFalse(os.path.exists(get_files_path(self.dup_filename))) + def test_attachment_limit(self): + doctype, docname = make_test_doc() + from frappe.custom.doctype.property_setter.property_setter import make_property_setter + limit_property = make_property_setter('ToDo', None, 'max_attachments', 1, 'int', for_doctype=True) + _file1 = frappe.get_doc({ + "doctype": "File", + "file_name": 'test-attachment', + "attached_to_doctype": doctype, + "attached_to_name": docname, + "content": 'test'}) + + _file1.insert() + + _file2 = frappe.get_doc({ + "doctype": "File", + "file_name": 'test-attachment', + "attached_to_doctype": doctype, + "attached_to_name": docname, + "content": 'test2'}) + + self.assertRaises(frappe.exceptions.AttachmentLimitReached, _file2.insert) + limit_property.delete() + frappe.clear_cache(doctype='ToDo') def tearDown(self): # File gets deleted on rollback, so blank From a7264405b5a09ac5f7d4121e914a570a5a6de2b6 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Tue, 20 Oct 2020 10:27:16 +0530 Subject: [PATCH 7/8] style: Fix formatting --- frappe/core/doctype/file/file.py | 6 +++--- frappe/core/doctype/file/test_file.py | 14 ++++++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index a8a999ff9a..74c6c3f22e 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -153,9 +153,9 @@ class File(Document): }, limit=attachment_limit + 1)) if current_attachment_count >= attachment_limit: - frappe.throw(_("Maximum Attachment Limit reached for {0} {1}.").format( - self.attached_to_doctype, - self.attached_to_name + frappe.throw( + _("Maximum Attachment Limit reached for {0} {1}.").format( + self.attached_to_doctype, self.attached_to_name ), exc=frappe.exceptions.AttachmentLimitReached, title=_('Attachment Limit Reached') diff --git a/frappe/core/doctype/file/test_file.py b/frappe/core/doctype/file/test_file.py index cb44d33781..e627558680 100644 --- a/frappe/core/doctype/file/test_file.py +++ b/frappe/core/doctype/file/test_file.py @@ -164,23 +164,25 @@ class TestSameContent(unittest.TestCase): doctype, docname = make_test_doc() from frappe.custom.doctype.property_setter.property_setter import make_property_setter limit_property = make_property_setter('ToDo', None, 'max_attachments', 1, 'int', for_doctype=True) - _file1 = frappe.get_doc({ + file1 = frappe.get_doc({ "doctype": "File", "file_name": 'test-attachment', "attached_to_doctype": doctype, "attached_to_name": docname, - "content": 'test'}) + "content": 'test' + }) - _file1.insert() + file1.insert() - _file2 = frappe.get_doc({ + file2 = frappe.get_doc({ "doctype": "File", "file_name": 'test-attachment', "attached_to_doctype": doctype, "attached_to_name": docname, - "content": 'test2'}) + "content": 'test2' + }) - self.assertRaises(frappe.exceptions.AttachmentLimitReached, _file2.insert) + self.assertRaises(frappe.exceptions.AttachmentLimitReached, file2.insert) limit_property.delete() frappe.clear_cache(doctype='ToDo') From 1685afc49eed63a370ec98880e6c34266ad1e5db Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 26 Oct 2020 13:53:01 +0530 Subject: [PATCH 8/8] fix: Update limit validation message --- frappe/core/doctype/file/file.py | 4 ++-- .../public/js/frappe/file_uploader/index.js | 9 ++------- .../js/frappe/form/sidebar/attachments.js | 20 +++++++++++-------- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/frappe/core/doctype/file/file.py b/frappe/core/doctype/file/file.py index 74c6c3f22e..48247860b3 100755 --- a/frappe/core/doctype/file/file.py +++ b/frappe/core/doctype/file/file.py @@ -154,8 +154,8 @@ class File(Document): if current_attachment_count >= attachment_limit: frappe.throw( - _("Maximum Attachment Limit reached for {0} {1}.").format( - self.attached_to_doctype, self.attached_to_name + _("Maximum Attachment Limit of {0} has been reached for {1} {2}.").format( + frappe.bold(attachment_limit), self.attached_to_doctype, self.attached_to_name ), exc=frappe.exceptions.AttachmentLimitReached, title=_('Attachment Limit Reached') diff --git a/frappe/public/js/frappe/file_uploader/index.js b/frappe/public/js/frappe/file_uploader/index.js index 36de853b8f..646f60715a 100644 --- a/frappe/public/js/frappe/file_uploader/index.js +++ b/frappe/public/js/frappe/file_uploader/index.js @@ -17,13 +17,8 @@ export default class FileUploader { disable_file_browser, frm } = {}) { - if (frm && frm.attachments.max_reached()) { - frappe.throw({ - title: __("Attachment Limit Reached"), - message: __("Maximum attachment limit for this record reached."), - }); - return; - } + + frm && frm.attachments.max_reached(true); if (!wrapper) { this.make_dialog(); diff --git a/frappe/public/js/frappe/form/sidebar/attachments.js b/frappe/public/js/frappe/form/sidebar/attachments.js index dc0c839737..56b484e7c4 100644 --- a/frappe/public/js/frappe/form/sidebar/attachments.js +++ b/frappe/public/js/frappe/form/sidebar/attachments.js @@ -16,15 +16,19 @@ frappe.ui.form.Attachments = Class.extend({ this.add_attachment_wrapper = this.parent.find(".add_attachment").parent(); this.attachments_label = this.parent.find(".attachments-label"); }, - max_reached: function() { - // no of attachments - var n = Object.keys(this.get_attachments()).length; - - // button if the number of attachments is less than max - if(n < this.frm.meta.max_attachments || !this.frm.meta.max_attachments) { - return false; + max_reached: function(raise_exception=false) { + const attachment_count = Object.keys(this.get_attachments()).length; + const attachment_limit = this.frm.meta.max_attachments; + if (attachment_limit && attachment_count >= attachment_limit) { + if (raise_exception) { + frappe.throw({ + title: __("Attachment Limit Reached"), + message: __("Maximum attachment limit of {0} has been reached.", [cstr(attachment_limit).bold()]), + }); + } + return true; } - return true; + return false; }, refresh: function() { var me = this;