From eccf26e05f5e9d46c0087a2ebc45abdf9839035e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 25 Mar 2025 15:21:08 +0100 Subject: [PATCH 1/4] refactor(minor): Backups Page - Don't cache page as it shows outdated backups in the list - Check for backups to retain properly by bundling them by "backup" rather than individual files - Use cached system settings & new Path library to de-noise - Optimize hourly job that deletes > limit_backup num of files (cherry picked from commit c3b544389654263e3ebc7e14936ba230feef342b) --- frappe/desk/page/backups/backups.py | 112 +++++++++++++++------------- 1 file changed, 62 insertions(+), 50 deletions(-) diff --git a/frappe/desk/page/backups/backups.py b/frappe/desk/page/backups/backups.py index d428835fa2..e6a726d7a1 100644 --- a/frappe/desk/page/backups/backups.py +++ b/frappe/desk/page/backups/backups.py @@ -1,82 +1,94 @@ import datetime -import os +from collections import defaultdict +from pathlib import Path import frappe from frappe import _ -from frappe.utils import cint, get_site_path, get_url +from frappe.utils import get_site_path, get_url from frappe.utils.data import convert_utc_to_system_timezone +def get_time(path: Path): + return convert_utc_to_system_timezone( + datetime.datetime.fromtimestamp(path.stat().st_mtime, tz=datetime.UTC) + ).strftime("%a %b %d %H:%M %Y") + + +def get_encrytion_status(path: Path): + return "-enc" in path.name + + +def get_size(path: Path): + size = path.stat().st_size + mbase = 1024 * 1024 + + if size > mbase: + return f"{size / mbase:.1f}M" + + return f"{size / 1024:.1f}K" + + def get_context(context): +<<<<<<< HEAD def get_time(path): dt = os.path.getmtime(path) return convert_utc_to_system_timezone( datetime.datetime.fromtimestamp(dt, tz=datetime.timezone.utc) ).strftime("%a %b %d %H:%M %Y") +======= + context.no_cache = True + backup_limit = frappe.get_system_settings("backup_limit") +>>>>>>> c3b5443896 (refactor(minor): Backups Page) - def get_encrytion_status(path): - if "-enc" in path: - return True - - def get_size(path): - size = os.path.getsize(path) - if size > 1048576: - return f"{float(size) / 1048576:.1f}M" - else: - return f"{float(size) / 1024:.1f}K" - - path = get_site_path("private", "backups") - files = [x for x in os.listdir(path) if os.path.isfile(os.path.join(path, x))] - backup_limit = get_scheduled_backup_limit() - - files = [ + backups_path = Path(get_site_path("private", "backups")) + backup_files = [ ( - "/backups/" + _file, - get_time(os.path.join(path, _file)), - get_encrytion_status(os.path.join(path, _file)), - get_size(os.path.join(path, _file)), + "/backups/" + x.relative_to(backups_path).as_posix(), + get_time(x), + get_encrytion_status(x), + get_size(x), ) - for _file in files - if _file.endswith("sql.gz") + for x in backups_path.iterdir() + if x.is_file() and x.name.endswith("sql.gz") ] - files.sort(key=lambda x: x[1], reverse=True) - return {"files": files[:backup_limit]} + backup_files.sort(key=lambda x: x[1], reverse=True) + + return {"files": backup_files[:backup_limit]} -def get_scheduled_backup_limit(): - backup_limit = frappe.db.get_singles_value("System Settings", "backup_limit") - return cint(backup_limit) +def cleanup_old_backups(backups: dict[str, list[Path]], limit: int): + backups_to_delete = len(backups) - limit + if backups_to_delete > 0: + backups = dict( + sorted(backups.items(), key=lambda x: max(y.stat().st_ctime for y in x[1]), reverse=True) + ) -def cleanup_old_backups(site_path, files, limit): - backup_paths = [] - for f in files: - if f.endswith("sql.gz"): - _path = os.path.abspath(os.path.join(site_path, f)) - backup_paths.append(_path) - - backup_paths = sorted(backup_paths, key=os.path.getctime) - files_to_delete = len(backup_paths) - limit - - for idx in range(0, files_to_delete): - f = os.path.basename(backup_paths[idx]) - files.remove(f) - - os.remove(backup_paths[idx]) + for b_files in list(backups.values())[-backups_to_delete:]: + for b_file in b_files: + b_file.unlink() def delete_downloadable_backups(): - path = get_site_path("private", "backups") - files = [x for x in os.listdir(path) if os.path.isfile(os.path.join(path, x))] - backup_limit = get_scheduled_backup_limit() + path = Path(get_site_path("private", "backups")) + backups = defaultdict(list) - if len(files) > backup_limit: - cleanup_old_backups(path, files, backup_limit) + for x in path.iterdir(): + if not x.is_file(): + continue + + # Based on the naming convention of the backup files defined in frappe.utils.backups + backup_name = x.name.rsplit("-" + frappe.local.site.replace(".", "_"), maxsplit=1)[0] + backups[backup_name].append(x) + + backup_limit = frappe.get_system_settings("backup_limit") + + cleanup_old_backups(backups, backup_limit) @frappe.whitelist() -def schedule_files_backup(user_email): +def schedule_files_backup(user_email: str): from frappe.utils.background_jobs import enqueue, get_jobs frappe.only_for("System Manager") From 3e0a526f70a81094f74542cc691e31569790ec32 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 25 Mar 2025 16:16:54 +0100 Subject: [PATCH 2/4] fix(health_report): Pass _dict to backups context (cherry picked from commit 26f04ac3138e34a681d913cf228679ec93ec5c24) --- .../desk/doctype/system_health_report/system_health_report.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/desk/doctype/system_health_report/system_health_report.py b/frappe/desk/doctype/system_health_report/system_health_report.py index 9596ebec5c..22ad295fb0 100644 --- a/frappe/desk/doctype/system_health_report/system_health_report.py +++ b/frappe/desk/doctype/system_health_report/system_health_report.py @@ -316,7 +316,7 @@ class SystemHealthReport(Document): self.backups_size = get_directory_size("private", "backups") / (1024 * 1024) self.private_files_size = get_directory_size("private", "files") / (1024 * 1024) self.public_files_size = get_directory_size("public", "files") / (1024 * 1024) - self.onsite_backups = len(get_context({}).get("files", [])) + self.onsite_backups = len(get_context(frappe._dict()).get("files", [])) @health_check("Users") def fetch_user_stats(self): From 98a807af89ea046046aa9e8d720086fea05e5eac Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 25 Mar 2025 16:35:41 +0100 Subject: [PATCH 3/4] fix(ux): Scroll to respective field in system settings via action (cherry picked from commit 4b597571b4322358aa5f1e35c5feb8465634a448) --- frappe/desk/page/backups/backups.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/desk/page/backups/backups.js b/frappe/desk/page/backups/backups.js index 08289cab2d..a7c0aed281 100644 --- a/frappe/desk/page/backups/backups.js +++ b/frappe/desk/page/backups/backups.js @@ -6,7 +6,9 @@ frappe.pages["backups"].on_page_load = function (wrapper) { }); page.add_inner_button(__("Set Number of Backups"), function () { - frappe.set_route("Form", "System Settings"); + frappe.set_route("Form", "System Settings").then(() => { + cur_frm.scroll_to_field("backup_limit"); + }); }); page.add_inner_button(__("Download Files Backup"), function () { From 2a91be31581c19270c998bf8cb51fde0fb23a36b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 16 Apr 2025 13:26:14 +0200 Subject: [PATCH 4/4] chore: Resolve conflicts --- frappe/desk/page/backups/backups.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/frappe/desk/page/backups/backups.py b/frappe/desk/page/backups/backups.py index e6a726d7a1..3e1c1fb20f 100644 --- a/frappe/desk/page/backups/backups.py +++ b/frappe/desk/page/backups/backups.py @@ -29,17 +29,8 @@ def get_size(path: Path): def get_context(context): -<<<<<<< HEAD - def get_time(path): - dt = os.path.getmtime(path) - return convert_utc_to_system_timezone( - datetime.datetime.fromtimestamp(dt, tz=datetime.timezone.utc) - ).strftime("%a %b %d %H:%M %Y") -======= context.no_cache = True backup_limit = frappe.get_system_settings("backup_limit") ->>>>>>> c3b5443896 (refactor(minor): Backups Page) - backups_path = Path(get_site_path("private", "backups")) backup_files = [ (