From ae88fca3efd97e040ace8f9d4d3e078d4366a738 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Oct 2021 10:59:59 +0530 Subject: [PATCH 1/5] refactor: split remove_app in smaller functions --- frappe/installer.py | 72 +++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 25 deletions(-) diff --git a/frappe/installer.py b/frappe/installer.py index e1b67ee10c..fff445d9e4 100755 --- a/frappe/installer.py +++ b/frappe/installer.py @@ -4,6 +4,7 @@ import json import os import sys +from typing import List import frappe from frappe.defaults import _clear_cache @@ -230,9 +231,28 @@ def remove_app(app_name, dry_run=False, yes=False, no_backup=False, force=False) scheduled_backup(ignore_files=True) frappe.flags.in_uninstall = True + + drop_doctypes = _delete_modules(app_name, dry_run=dry_run) + _delete_doctypes(drop_doctypes, dry_run=dry_run) + + if not dry_run: + remove_from_installed_apps(app_name) + frappe.db.commit() + + click.secho(f"Uninstalled App {app_name} from Site {site}", fg="green") + frappe.flags.in_uninstall = False + + +def _delete_modules(app_name: str, dry_run: bool) -> List[str]: + """ Delete modules belonging to the app and all related doctypes. + + Note: All record linked linked to Module Def are also deleted. + + Returns: list of deleted doctypes.""" drop_doctypes = [] modules = frappe.get_all("Module Def", filters={"app_name": app_name}, pluck="name") + for module_name in modules: print(f"Deleting Module '{module_name}'") @@ -247,40 +267,42 @@ def remove_app(app_name, dry_run=False, yes=False, no_backup=False, force=False) if not doctype.issingle: drop_doctypes.append(doctype.name) - linked_doctypes = frappe.get_all( - "DocField", filters={"fieldtype": "Link", "options": "Module Def"}, fields=["parent"] - ) - ordered_doctypes = ["Workspace", "Report", "Page", "Web Form"] - all_doctypes_with_linked_modules = ordered_doctypes + [ - doctype.parent - for doctype in linked_doctypes - if doctype.parent not in ordered_doctypes - ] - doctypes_with_linked_modules = [ - x for x in all_doctypes_with_linked_modules if frappe.db.exists("DocType", x) - ] - for doctype in doctypes_with_linked_modules: - for record in frappe.get_all(doctype, filters={"module": module_name}, pluck="name"): - print(f"* removing {doctype} '{record}'...") - if not dry_run: - frappe.delete_doc(doctype, record, ignore_on_trash=True, force=True) + _delete_linked_documents(module_name, dry_run=dry_run) print(f"* removing Module Def '{module_name}'...") if not dry_run: frappe.delete_doc("Module Def", module_name, ignore_on_trash=True, force=True) - for doctype in set(drop_doctypes): + return drop_doctypes + + +def _delete_linked_documents(module_name: str, dry_run: bool) -> None: + """Deleted all records linked with module def""" + linked_doctypes = frappe.get_all( + "DocField", filters={"fieldtype": "Link", "options": "Module Def"}, fields=["parent"] + ) + ordered_doctypes = ["Workspace", "Report", "Page", "Web Form"] + all_doctypes_with_linked_modules = ordered_doctypes + [ + doctype.parent + for doctype in linked_doctypes + if doctype.parent not in ordered_doctypes + ] + doctypes_with_linked_modules = [ + x for x in all_doctypes_with_linked_modules if frappe.db.exists("DocType", x) + ] + for doctype in doctypes_with_linked_modules: + for record in frappe.get_all(doctype, filters={"module": module_name}, pluck="name"): + print(f"* removing {doctype} '{record}'...") + if not dry_run: + frappe.delete_doc(doctype, record, ignore_on_trash=True, force=True) + + +def _delete_doctypes(doctypes: List[str], dry_run: bool) -> None: + for doctype in set(doctypes): print(f"* dropping Table for '{doctype}'...") if not dry_run: frappe.db.sql_ddl(f"drop table `tab{doctype}`") - if not dry_run: - remove_from_installed_apps(app_name) - frappe.db.commit() - - click.secho(f"Uninstalled App {app_name} from Site {site}", fg="green") - frappe.flags.in_uninstall = False - def post_install(rebuild_website=False): from frappe.website.utils import clear_website_cache From 412aa9f5cc3c3ce008605499d915f0cde00f51e9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Oct 2021 11:33:54 +0530 Subject: [PATCH 2/5] fix: fieldname for linkfield hardcoded to "module" Remove_app assumed that all link fileds linking to "Module Def" are called "module", this causes uninstall failure and leaves bench in pseudo-irrecoverable state. (requiring some manual cleanup) --- frappe/installer.py | 54 +++++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/frappe/installer.py b/frappe/installer.py index fff445d9e4..1ad003cdbf 100755 --- a/frappe/installer.py +++ b/frappe/installer.py @@ -4,7 +4,8 @@ import json import os import sys -from typing import List +from collections import OrderedDict +from typing import List, Dict import frappe from frappe.defaults import _clear_cache @@ -253,6 +254,7 @@ def _delete_modules(app_name: str, dry_run: bool) -> List[str]: modules = frappe.get_all("Module Def", filters={"app_name": app_name}, pluck="name") + doctype_link_field_map = _get_module_linked_doctype_field_map() for module_name in modules: print(f"Deleting Module '{module_name}'") @@ -267,7 +269,7 @@ def _delete_modules(app_name: str, dry_run: bool) -> List[str]: if not doctype.issingle: drop_doctypes.append(doctype.name) - _delete_linked_documents(module_name, dry_run=dry_run) + _delete_linked_documents(module_name, doctype_link_field_map, dry_run=dry_run) print(f"* removing Module Def '{module_name}'...") if not dry_run: @@ -276,26 +278,44 @@ def _delete_modules(app_name: str, dry_run: bool) -> List[str]: return drop_doctypes -def _delete_linked_documents(module_name: str, dry_run: bool) -> None: +def _delete_linked_documents( + module_name: str, + doctype_linkfield_map: Dict[str, str], + dry_run: bool + ) -> None: + """Deleted all records linked with module def""" - linked_doctypes = frappe.get_all( - "DocField", filters={"fieldtype": "Link", "options": "Module Def"}, fields=["parent"] - ) - ordered_doctypes = ["Workspace", "Report", "Page", "Web Form"] - all_doctypes_with_linked_modules = ordered_doctypes + [ - doctype.parent - for doctype in linked_doctypes - if doctype.parent not in ordered_doctypes - ] - doctypes_with_linked_modules = [ - x for x in all_doctypes_with_linked_modules if frappe.db.exists("DocType", x) - ] - for doctype in doctypes_with_linked_modules: - for record in frappe.get_all(doctype, filters={"module": module_name}, pluck="name"): + for doctype, fieldname in doctype_linkfield_map.items(): + for record in frappe.get_all(doctype, filters={fieldname: module_name}, pluck="name"): print(f"* removing {doctype} '{record}'...") if not dry_run: frappe.delete_doc(doctype, record, ignore_on_trash=True, force=True) +def _get_module_linked_doctype_field_map() -> Dict[str, str]: + """ Get all the doctypes which have module linked with them. + + returns ordered dictionary with doctype->link field mapping.""" + + # Hardcoded to change order of deletion + ordered_doctypes = [ + ("Workspace", "module"), + ("Report", "module"), + ("Page", "module"), + ("Web Form", "module") + ] + doctype_to_field_map = OrderedDict(ordered_doctypes) + + linked_doctypes = frappe.get_all( + "DocField", filters={"fieldtype": "Link", "options": "Module Def"}, fields=["parent", "fieldname"] + ) + existing_linked_doctypes = [d for d in linked_doctypes if frappe.db.exists("DocType", d.parent)] + + for d in existing_linked_doctypes: + if d.parent not in doctype_to_field_map: + doctype_to_field_map[d.parent] = d.fieldname + + return doctype_to_field_map + def _delete_doctypes(doctypes: List[str], dry_run: bool) -> None: for doctype in set(doctypes): From aa372b099001e1fe9e2966c246f62923c098e7bd Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Oct 2021 12:20:47 +0530 Subject: [PATCH 3/5] fix: change order of deletion Previous order was: 1. Delete DocTypes 2. Delete linked records 3. Drop tables Now if linked records belonged to deleted doctypes it causes uninstallation failure. Changed order to: 1. Delete linked records 2. Delete DocTypes 3. Drop tables --- frappe/installer.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frappe/installer.py b/frappe/installer.py index 1ad003cdbf..c38ee55e65 100755 --- a/frappe/installer.py +++ b/frappe/installer.py @@ -264,9 +264,9 @@ def _delete_modules(app_name: str, dry_run: bool) -> List[str]: print(f"* removing DocType '{doctype.name}'...") if not dry_run: - frappe.delete_doc("DocType", doctype.name, ignore_on_trash=True) - - if not doctype.issingle: + if doctype.issingle: + frappe.delete_doc("DocType", doctype.name, ignore_on_trash=True) + else: drop_doctypes.append(doctype.name) _delete_linked_documents(module_name, doctype_link_field_map, dry_run=dry_run) @@ -311,7 +311,8 @@ def _get_module_linked_doctype_field_map() -> Dict[str, str]: existing_linked_doctypes = [d for d in linked_doctypes if frappe.db.exists("DocType", d.parent)] for d in existing_linked_doctypes: - if d.parent not in doctype_to_field_map: + # DocType deletion is handled separately in the end + if d.parent not in doctype_to_field_map and d.parent != "DocType": doctype_to_field_map[d.parent] = d.fieldname return doctype_to_field_map @@ -321,6 +322,7 @@ def _delete_doctypes(doctypes: List[str], dry_run: bool) -> None: for doctype in set(doctypes): print(f"* dropping Table for '{doctype}'...") if not dry_run: + frappe.delete_doc("DocType", doctype, ignore_on_trash=True) frappe.db.sql_ddl(f"drop table `tab{doctype}`") From 70fcac8acf5e8e1dc0708f1794e676e75e9529e2 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Oct 2021 17:10:14 +0530 Subject: [PATCH 4/5] refactor: change _delete_modules for testability --- frappe/installer.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/installer.py b/frappe/installer.py index c38ee55e65..9df2019f1b 100755 --- a/frappe/installer.py +++ b/frappe/installer.py @@ -233,7 +233,9 @@ def remove_app(app_name, dry_run=False, yes=False, no_backup=False, force=False) frappe.flags.in_uninstall = True - drop_doctypes = _delete_modules(app_name, dry_run=dry_run) + modules = frappe.get_all("Module Def", filters={"app_name": app_name}, pluck="name") + + drop_doctypes = _delete_modules(modules, dry_run=dry_run) _delete_doctypes(drop_doctypes, dry_run=dry_run) if not dry_run: @@ -244,7 +246,7 @@ def remove_app(app_name, dry_run=False, yes=False, no_backup=False, force=False) frappe.flags.in_uninstall = False -def _delete_modules(app_name: str, dry_run: bool) -> List[str]: +def _delete_modules(modules: List[str], dry_run: bool) -> List[str]: """ Delete modules belonging to the app and all related doctypes. Note: All record linked linked to Module Def are also deleted. @@ -252,8 +254,6 @@ def _delete_modules(app_name: str, dry_run: bool) -> List[str]: Returns: list of deleted doctypes.""" drop_doctypes = [] - modules = frappe.get_all("Module Def", filters={"app_name": app_name}, pluck="name") - doctype_link_field_map = _get_module_linked_doctype_field_map() for module_name in modules: print(f"Deleting Module '{module_name}'") From 35b242fe271b58a54756f6f2a75ce6e8bc110b73 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Oct 2021 17:33:18 +0530 Subject: [PATCH 5/5] test: remove app unit tests --- frappe/tests/test_commands.py | 49 ++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_commands.py b/frappe/tests/test_commands.py index ee184843ad..c048e23949 100644 --- a/frappe/tests/test_commands.py +++ b/frappe/tests/test_commands.py @@ -13,7 +13,7 @@ import glob # imports - module imports import frappe import frappe.recorder -from frappe.installer import add_to_installed_apps +from frappe.installer import add_to_installed_apps, remove_app from frappe.utils import add_to_date, get_bench_relative_path, now from frappe.utils.backups import fetch_latest_backups @@ -465,3 +465,50 @@ class TestCommands(BaseTestCommands): self.execute("bench --site {site} set-admin-password test2") self.assertEqual(self.returncode, 0) self.assertEqual(check_password('Administrator', 'test2'), 'Administrator') + + +class RemoveAppUnitTests(unittest.TestCase): + def test_delete_modules(self): + from frappe.installer import ( + _delete_doctypes, + _delete_modules, + _get_module_linked_doctype_field_map, + ) + + test_module = frappe.new_doc("Module Def") + + test_module.update({"module_name": "RemoveThis", "app_name": "frappe"}) + test_module.save() + + module_def_linked_doctype = frappe.get_doc({ + "doctype": "DocType", + "name": "Doctype linked with module def", + "module": "RemoveThis", + "custom": 1, + "fields": [{ + "label": "Modulen't", + "fieldname": "notmodule", + "fieldtype": "Link", + "options": "Module Def" + }] + }).insert() + + doctype_to_link_field_map = _get_module_linked_doctype_field_map() + + self.assertIn("Report", doctype_to_link_field_map) + self.assertIn(module_def_linked_doctype.name, doctype_to_link_field_map) + self.assertEqual(doctype_to_link_field_map[module_def_linked_doctype.name], "notmodule") + self.assertNotIn("DocType", doctype_to_link_field_map) + + doctypes_to_delete = _delete_modules([test_module.module_name], dry_run=False) + self.assertEqual(len(doctypes_to_delete), 1) + + _delete_doctypes(doctypes_to_delete, dry_run=False) + self.assertFalse(frappe.db.exists("Module Def", test_module.module_name)) + self.assertFalse(frappe.db.exists("DocType", module_def_linked_doctype.name)) + + def test_dry_run(self): + """Check if dry run in not destructive.""" + + # nothing to assert, if this fails rest of the test suite will crumble. + remove_app("frappe", dry_run=True, yes=True, no_backup=True)