From 6adaa815b7121288b3091616f1a7fda48cdae15b Mon Sep 17 00:00:00 2001 From: Sparsh Verma Date: Fri, 5 Jun 2026 12:27:10 +0530 Subject: [PATCH 1/3] fix: hide_search and hide_notification toggles for Frappe v16 - toggleSearchBar() now matches both the v15 navbar selector and the v16 sidebar selector (.body-sidebar .navbar-search-bar), and restores the element's display when the role no longer matches. - New toggleNotification() reads the previously-orphaned hide_notification field and toggles .sidebar-notification (v16) / .dropdown-notifications (v15). - Both wired into applyTheme() and the existing MutationObserver so they survive sidebar re-renders. - Added schema + data-layer unit tests and opt-in Playwright E2E tests (gated by DESK_THEME_E2E_SITE env var to keep standard CI fast). --- commitlint.config.js | 10 +- .../doctype/desk_theme/test_desk_theme.py | 181 ++++++++++++++++-- .../public/js/frappe_desk_theme.bundle.js | 53 ++++- 3 files changed, 220 insertions(+), 24 deletions(-) diff --git a/commitlint.config.js b/commitlint.config.js index 55f1dff..ce91a16 100644 --- a/commitlint.config.js +++ b/commitlint.config.js @@ -1,7 +1,7 @@ module.exports = { - extends: ["@commitlint/config-conventional"], - // Disable default 100-char (or 72-char) header length limit for commit messages - rules: { - "header-max-length": [0, "always", 100], - }, + extends: ["@commitlint/config-conventional"], + // Disable default 100-char (or 72-char) header length limit for commit messages + rules: { + "header-max-length": [0, "always", 100], + }, }; diff --git a/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py b/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py index 4153c63..ea3776b 100644 --- a/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py +++ b/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py @@ -1,20 +1,179 @@ # Copyright (c) 2025, Dhwani RIS and Contributors # See license.txt -# import frappe +import importlib.util +import os +import unittest + +import frappe +from frappe.auth import CookieManager, LoginManager from frappe.tests import IntegrationTestCase -# On IntegrationTestCase, the doctype test records and all -# link-field test record dependencies are recursively loaded -# Use these module variables to add/remove to/from that list -EXTRA_TEST_RECORD_DEPENDENCIES = [] # eg. ["User"] -IGNORE_TEST_RECORD_DEPENDENCIES = [] # eg. ["User"] +try: + from playwright.sync_api import sync_playwright +except ImportError: + sync_playwright = None -class IntegrationTestDeskTheme(IntegrationTestCase): - """ - Integration tests for DeskTheme. - Use this class for testing interactions between multiple components. +def _playwright_available() -> bool: + return importlib.util.find_spec("playwright") is not None + + +class TestDeskTheme(unittest.TestCase): + """Schema + data-layer tests for the Desk Theme hide_search / + hide_notification fields. + + Uses plain unittest.TestCase (not IntegrationTestCase) because these + tests don't insert any Desk Theme rows — they only read meta and + round-trip the existing singleton. Avoiding IntegrationTestCase skips + Frappe's transitive fixture loader (Email Account → ToDo → …) which + can fail on downstream sites that customise those doctypes.""" + + def test_doctype_exposes_hide_notification_and_hide_search_fields(self): + """Field schema is the contract the bundle JS reads against — the + bug history was: schema field exists, JS never consumed it. This + test fails fast if a future refactor removes either field, so the + client-side toggle code wouldn't silently regress.""" + meta = frappe.get_meta("Desk Theme") + hide_notification = meta.get_field("hide_notification") + hide_search = meta.get_field("hide_search") + + self.assertIsNotNone(hide_notification, "hide_notification field removed") + self.assertEqual(hide_notification.fieldtype, "Check") + + self.assertIsNotNone(hide_search, "hide_search field removed") + self.assertEqual(hide_search.fieldtype, "Table MultiSelect") + self.assertEqual(hide_search.options, "Has Role") + + def test_api_round_trips_both_fields(self): + """The bundle JS reads both fields from get_custom_theme via /api/method. + Round-tripping through frappe.get_doc proves the data layer carries + both values to the client; if either disappears here, the JS reads + undefined and silently no-ops.""" + theme = frappe.get_single("Desk Theme") + prev_notif = theme.hide_notification + prev_roles = [row.role for row in (theme.hide_search or [])] + + try: + theme.hide_notification = 1 + theme.set("hide_search", []) + theme.append("hide_search", {"role": "Administrator"}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + + reloaded = frappe.get_single("Desk Theme") + self.assertEqual(reloaded.hide_notification, 1) + self.assertEqual( + [row.role for row in reloaded.hide_search], + ["Administrator"], + ) + finally: + theme = frappe.get_single("Desk Theme") + theme.hide_notification = prev_notif + theme.set("hide_search", []) + for role in prev_roles: + theme.append("hide_search", {"role": role}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + + +@unittest.skipUnless(_playwright_available(), "playwright not installed") +@unittest.skipUnless(os.environ.get("DESK_THEME_E2E_SITE"), "DESK_THEME_E2E_SITE not set") +class E2EDeskThemeToggle(IntegrationTestCase): + """End-to-end browser test that exercises the JS bundle directly. + + Set DESK_THEME_E2E_SITE=http://bsebeamv16.localhost:8003 (a running bench + with the user `Administrator`) before invoking. Skipped otherwise so + standard CI runs aren't blocked on the bench dev server. + + Browser launch lives in setUp / tearDown (per-instance) rather than + setUpClass / tearDownClass so the test file conforms to the project's + snake_case naming policy. The extra ~2s per test is negligible given + these tests only run in opt-in E2E mode. """ - pass + def setUp(self): + super().setUp() + self._pw = sync_playwright().start() + self.browser = self._pw.chromium.launch(headless=True) + self.base = os.environ["DESK_THEME_E2E_SITE"].rstrip("/") + self._original_notif = frappe.db.get_single_value("Desk Theme", "hide_notification") + + def tearDown(self): + try: + theme = frappe.get_single("Desk Theme") + theme.hide_notification = self._original_notif + theme.set("hide_search", []) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + finally: + self.browser.close() + self._pw.stop() + super().tearDown() + + def _login_url(self) -> str: + """Mint a single-use SID for Administrator on the live site.""" + frappe.local.cookie_manager = CookieManager() + frappe.local.login_manager = LoginManager() + frappe.local.login_manager.login_as("Administrator") + sid = frappe.session.sid + return f"{self.base}/app?sid={sid}" + + def _open(self): + ctx = self.browser.new_context(viewport={"width": 1440, "height": 900}) + page = ctx.new_page() + page.goto(self._login_url(), wait_until="networkidle", timeout=60000) + page.goto(self.base + "/app/dashboard", wait_until="networkidle", timeout=60000) + # Wait for sidebar to render (polling — wait_for_selector is flaky on first paint) + for _ in range(60): + if page.evaluate( + "document.querySelectorAll('.body-sidebar .sidebar-item-container').length" + ) > 5: + break + page.wait_for_timeout(250) + return ctx, page + + def _set_theme(self, hide_notification: bool, hide_for_roles: list[str]): + theme = frappe.get_single("Desk Theme") + theme.hide_notification = 1 if hide_notification else 0 + theme.set("hide_search", []) + for role in hide_for_roles: + theme.append("hide_search", {"role": role}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + + def _is_hidden(self, page, selector: str) -> bool: + return page.evaluate( + f"""() => {{ + const el = document.querySelector('{selector}'); + if (!el) return null; + const s = getComputedStyle(el); + return s.display === 'none' || el.offsetHeight === 0; + }}""" + ) + + def test_hide_notification_flag_actually_hides_sidebar_bell(self): + # Off → bell visible + self._set_theme(hide_notification=False, hide_for_roles=[]) + ctx, page = self._open() + self.assertFalse(self._is_hidden(page, ".body-sidebar .sidebar-notification")) + ctx.close() + + # On → bell hidden + self._set_theme(hide_notification=True, hide_for_roles=[]) + ctx, page = self._open() + self.assertTrue(self._is_hidden(page, ".body-sidebar .sidebar-notification")) + ctx.close() + + def test_hide_search_for_role_actually_hides_sidebar_search(self): + # Administrator not in list → search visible + self._set_theme(hide_notification=False, hide_for_roles=[]) + ctx, page = self._open() + self.assertFalse(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) + ctx.close() + + # Administrator in list → search hidden + self._set_theme(hide_notification=False, hide_for_roles=["Administrator"]) + ctx, page = self._open() + self.assertTrue(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) + ctx.close() diff --git a/frappe_desk_theme/public/js/frappe_desk_theme.bundle.js b/frappe_desk_theme/public/js/frappe_desk_theme.bundle.js index 2436adb..d671798 100644 --- a/frappe_desk_theme/public/js/frappe_desk_theme.bundle.js +++ b/frappe_desk_theme/public/js/frappe_desk_theme.bundle.js @@ -677,6 +677,7 @@ class FrappeDeskTheme { this.setCSSVariables(); this.toggleSidebar(); this.toggleSearchBar(); + this.toggleNotification(); this.hideStandardMenu(); this.applyFixedSidebarBehavior(); this.performInitialSidebarLoginRedirect(); @@ -725,18 +726,53 @@ class FrappeDeskTheme { } /** - * Toggle search bar visibility based on user roles - * Hides search bar if current user's role matches hide_search configuration + * Toggle search bar visibility based on user roles. + * Hides search bar if current user's role matches hide_search configuration, + * otherwise restores its native display. + * + * Frappe v15 used `.input-group.search-bar.text-muted` in the top navbar. + * Frappe v16 moved the search affordance into the sidebar rail as + * `.body-sidebar .navbar-search-bar`. Both are queried so the setting + * works on either version. */ toggleSearchBar() { - const searchBar = document.querySelector(".input-group.search-bar.text-muted"); - if (!searchBar) { - return; - } + const selectors = [ + // v15 navbar search box + ".input-group.search-bar.text-muted", + // v16 sidebar search rail + ".body-sidebar .navbar-search-bar", + ]; + const shouldHide = this.getUserRoles(); + selectors.forEach((sel) => { + document.querySelectorAll(sel).forEach((el) => { + // Restore via empty string so the element falls back to its + // stylesheet display value when the role no longer matches. + el.style.display = shouldHide ? "none" : ""; + }); + }); + } - if (this.getUserRoles()) { - searchBar.style.display = "none"; + /** + * Toggle the desk notification bell based on the hide_notification flag. + * Acts on both the v15 navbar bell and the v16 sidebar bell so the same + * theme setting works regardless of Frappe version. + */ + toggleNotification() { + if (!this.themeData) { + return; } + const selectors = [ + // v16 sidebar notification + ".body-sidebar .sidebar-notification", + // v15 navbar notification + ".navbar-nav .dropdown-notifications", + ]; + const shouldHide = !!this.themeData.hide_notification; + selectors.forEach((sel) => { + document.querySelectorAll(sel).forEach((el) => { + el.style.display = shouldHide ? "none" : ""; + }); + }); } /** @@ -1106,6 +1142,7 @@ class FrappeDeskTheme { let footerTimeout; const observer = new MutationObserver(() => { this.toggleSearchBar(); + this.toggleNotification(); this.hideStandardMenu(); this.applyFixedSidebarBehavior(); this.performInitialSidebarLoginRedirect(); From 03ef14521ece421469321fe0a24e2481de83a002 Mon Sep 17 00:00:00 2001 From: Sparsh Verma Date: Fri, 5 Jun 2026 12:29:51 +0530 Subject: [PATCH 2/3] fix: conditional import for playwright removed for top import condition --- .../doctype/desk_theme/test_desk_theme.py | 311 +++++++++--------- 1 file changed, 156 insertions(+), 155 deletions(-) diff --git a/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py b/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py index ea3776b..c63afe9 100644 --- a/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py +++ b/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py @@ -8,172 +8,173 @@ import frappe from frappe.auth import CookieManager, LoginManager from frappe.tests import IntegrationTestCase - -try: - from playwright.sync_api import sync_playwright -except ImportError: - sync_playwright = None +from playwright.sync.api import sync_playwright def _playwright_available() -> bool: - return importlib.util.find_spec("playwright") is not None + return importlib.util.find_spec("playwright") is not None class TestDeskTheme(unittest.TestCase): - """Schema + data-layer tests for the Desk Theme hide_search / - hide_notification fields. - - Uses plain unittest.TestCase (not IntegrationTestCase) because these - tests don't insert any Desk Theme rows — they only read meta and - round-trip the existing singleton. Avoiding IntegrationTestCase skips - Frappe's transitive fixture loader (Email Account → ToDo → …) which - can fail on downstream sites that customise those doctypes.""" - - def test_doctype_exposes_hide_notification_and_hide_search_fields(self): - """Field schema is the contract the bundle JS reads against — the - bug history was: schema field exists, JS never consumed it. This - test fails fast if a future refactor removes either field, so the - client-side toggle code wouldn't silently regress.""" - meta = frappe.get_meta("Desk Theme") - hide_notification = meta.get_field("hide_notification") - hide_search = meta.get_field("hide_search") - - self.assertIsNotNone(hide_notification, "hide_notification field removed") - self.assertEqual(hide_notification.fieldtype, "Check") - - self.assertIsNotNone(hide_search, "hide_search field removed") - self.assertEqual(hide_search.fieldtype, "Table MultiSelect") - self.assertEqual(hide_search.options, "Has Role") - - def test_api_round_trips_both_fields(self): - """The bundle JS reads both fields from get_custom_theme via /api/method. - Round-tripping through frappe.get_doc proves the data layer carries - both values to the client; if either disappears here, the JS reads - undefined and silently no-ops.""" - theme = frappe.get_single("Desk Theme") - prev_notif = theme.hide_notification - prev_roles = [row.role for row in (theme.hide_search or [])] - - try: - theme.hide_notification = 1 - theme.set("hide_search", []) - theme.append("hide_search", {"role": "Administrator"}) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep - - reloaded = frappe.get_single("Desk Theme") - self.assertEqual(reloaded.hide_notification, 1) - self.assertEqual( - [row.role for row in reloaded.hide_search], - ["Administrator"], - ) - finally: - theme = frappe.get_single("Desk Theme") - theme.hide_notification = prev_notif - theme.set("hide_search", []) - for role in prev_roles: - theme.append("hide_search", {"role": role}) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep + """Schema + data-layer tests for the Desk Theme hide_search / + hide_notification fields. + + Uses plain unittest.TestCase (not IntegrationTestCase) because these + tests don't insert any Desk Theme rows — they only read meta and + round-trip the existing singleton. Avoiding IntegrationTestCase skips + Frappe's transitive fixture loader (Email Account → ToDo → …) which + can fail on downstream sites that customise those doctypes.""" + + def test_doctype_exposes_hide_notification_and_hide_search_fields(self): + """Field schema is the contract the bundle JS reads against — the + bug history was: schema field exists, JS never consumed it. This + test fails fast if a future refactor removes either field, so the + client-side toggle code wouldn't silently regress.""" + meta = frappe.get_meta("Desk Theme") + hide_notification = meta.get_field("hide_notification") + hide_search = meta.get_field("hide_search") + + self.assertIsNotNone(hide_notification, "hide_notification field removed") + self.assertEqual(hide_notification.fieldtype, "Check") + + self.assertIsNotNone(hide_search, "hide_search field removed") + self.assertEqual(hide_search.fieldtype, "Table MultiSelect") + self.assertEqual(hide_search.options, "Has Role") + + def test_api_round_trips_both_fields(self): + """The bundle JS reads both fields from get_custom_theme via /api/method. + Round-tripping through frappe.get_doc proves the data layer carries + both values to the client; if either disappears here, the JS reads + undefined and silently no-ops.""" + theme = frappe.get_single("Desk Theme") + prev_notif = theme.hide_notification + prev_roles = [row.role for row in (theme.hide_search or [])] + + try: + theme.hide_notification = 1 + theme.set("hide_search", []) + theme.append("hide_search", {"role": "Administrator"}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + + reloaded = frappe.get_single("Desk Theme") + self.assertEqual(reloaded.hide_notification, 1) + self.assertEqual( + [row.role for row in reloaded.hide_search], + ["Administrator"], + ) + finally: + theme = frappe.get_single("Desk Theme") + theme.hide_notification = prev_notif + theme.set("hide_search", []) + for role in prev_roles: + theme.append("hide_search", {"role": role}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep @unittest.skipUnless(_playwright_available(), "playwright not installed") -@unittest.skipUnless(os.environ.get("DESK_THEME_E2E_SITE"), "DESK_THEME_E2E_SITE not set") +@unittest.skipUnless( + os.environ.get("DESK_THEME_E2E_SITE"), "DESK_THEME_E2E_SITE not set" +) class E2EDeskThemeToggle(IntegrationTestCase): - """End-to-end browser test that exercises the JS bundle directly. - - Set DESK_THEME_E2E_SITE=http://bsebeamv16.localhost:8003 (a running bench - with the user `Administrator`) before invoking. Skipped otherwise so - standard CI runs aren't blocked on the bench dev server. - - Browser launch lives in setUp / tearDown (per-instance) rather than - setUpClass / tearDownClass so the test file conforms to the project's - snake_case naming policy. The extra ~2s per test is negligible given - these tests only run in opt-in E2E mode. - """ - - def setUp(self): - super().setUp() - self._pw = sync_playwright().start() - self.browser = self._pw.chromium.launch(headless=True) - self.base = os.environ["DESK_THEME_E2E_SITE"].rstrip("/") - self._original_notif = frappe.db.get_single_value("Desk Theme", "hide_notification") - - def tearDown(self): - try: - theme = frappe.get_single("Desk Theme") - theme.hide_notification = self._original_notif - theme.set("hide_search", []) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep - finally: - self.browser.close() - self._pw.stop() - super().tearDown() - - def _login_url(self) -> str: - """Mint a single-use SID for Administrator on the live site.""" - frappe.local.cookie_manager = CookieManager() - frappe.local.login_manager = LoginManager() - frappe.local.login_manager.login_as("Administrator") - sid = frappe.session.sid - return f"{self.base}/app?sid={sid}" - - def _open(self): - ctx = self.browser.new_context(viewport={"width": 1440, "height": 900}) - page = ctx.new_page() - page.goto(self._login_url(), wait_until="networkidle", timeout=60000) - page.goto(self.base + "/app/dashboard", wait_until="networkidle", timeout=60000) - # Wait for sidebar to render (polling — wait_for_selector is flaky on first paint) - for _ in range(60): - if page.evaluate( - "document.querySelectorAll('.body-sidebar .sidebar-item-container').length" - ) > 5: - break - page.wait_for_timeout(250) - return ctx, page - - def _set_theme(self, hide_notification: bool, hide_for_roles: list[str]): - theme = frappe.get_single("Desk Theme") - theme.hide_notification = 1 if hide_notification else 0 - theme.set("hide_search", []) - for role in hide_for_roles: - theme.append("hide_search", {"role": role}) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep - - def _is_hidden(self, page, selector: str) -> bool: - return page.evaluate( - f"""() => {{ + """End-to-end browser test that exercises the JS bundle directly. + + Set DESK_THEME_E2E_SITE=http://bsebeamv16.localhost:8003 (a running bench + with the user `Administrator`) before invoking. Skipped otherwise so + standard CI runs aren't blocked on the bench dev server. + + Browser launch lives in setUp / tearDown (per-instance) rather than + setUpClass / tearDownClass so the test file conforms to the project's + snake_case naming policy. The extra ~2s per test is negligible given + these tests only run in opt-in E2E mode. + """ + + def setUp(self): + super().setUp() + self._pw = sync_playwright().start() + self.browser = self._pw.chromium.launch(headless=True) + self.base = os.environ["DESK_THEME_E2E_SITE"].rstrip("/") + self._original_notif = frappe.db.get_single_value( + "Desk Theme", "hide_notification" + ) + + def tearDown(self): + try: + theme = frappe.get_single("Desk Theme") + theme.hide_notification = self._original_notif + theme.set("hide_search", []) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + finally: + self.browser.close() + self._pw.stop() + super().tearDown() + + def _login_url(self) -> str: + """Mint a single-use SID for Administrator on the live site.""" + frappe.local.cookie_manager = CookieManager() + frappe.local.login_manager = LoginManager() + frappe.local.login_manager.login_as("Administrator") + sid = frappe.session.sid + return f"{self.base}/app?sid={sid}" + + def _open(self): + ctx = self.browser.new_context(viewport={"width": 1440, "height": 900}) + page = ctx.new_page() + page.goto(self._login_url(), wait_until="networkidle", timeout=60000) + page.goto(self.base + "/app/dashboard", wait_until="networkidle", timeout=60000) + # Wait for sidebar to render (polling — wait_for_selector is flaky on first paint) + for _ in range(60): + if ( + page.evaluate( + "document.querySelectorAll('.body-sidebar .sidebar-item-container').length" + ) + > 5 + ): + break + page.wait_for_timeout(250) + return ctx, page + + def _set_theme(self, hide_notification: bool, hide_for_roles: list[str]): + theme = frappe.get_single("Desk Theme") + theme.hide_notification = 1 if hide_notification else 0 + theme.set("hide_search", []) + for role in hide_for_roles: + theme.append("hide_search", {"role": role}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + + def _is_hidden(self, page, selector: str) -> bool: + return page.evaluate(f"""() => {{ const el = document.querySelector('{selector}'); if (!el) return null; const s = getComputedStyle(el); return s.display === 'none' || el.offsetHeight === 0; - }}""" - ) - - def test_hide_notification_flag_actually_hides_sidebar_bell(self): - # Off → bell visible - self._set_theme(hide_notification=False, hide_for_roles=[]) - ctx, page = self._open() - self.assertFalse(self._is_hidden(page, ".body-sidebar .sidebar-notification")) - ctx.close() - - # On → bell hidden - self._set_theme(hide_notification=True, hide_for_roles=[]) - ctx, page = self._open() - self.assertTrue(self._is_hidden(page, ".body-sidebar .sidebar-notification")) - ctx.close() - - def test_hide_search_for_role_actually_hides_sidebar_search(self): - # Administrator not in list → search visible - self._set_theme(hide_notification=False, hide_for_roles=[]) - ctx, page = self._open() - self.assertFalse(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) - ctx.close() - - # Administrator in list → search hidden - self._set_theme(hide_notification=False, hide_for_roles=["Administrator"]) - ctx, page = self._open() - self.assertTrue(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) - ctx.close() + }}""") + + def test_hide_notification_flag_actually_hides_sidebar_bell(self): + # Off → bell visible + self._set_theme(hide_notification=False, hide_for_roles=[]) + ctx, page = self._open() + self.assertFalse(self._is_hidden(page, ".body-sidebar .sidebar-notification")) + ctx.close() + + # On → bell hidden + self._set_theme(hide_notification=True, hide_for_roles=[]) + ctx, page = self._open() + self.assertTrue(self._is_hidden(page, ".body-sidebar .sidebar-notification")) + ctx.close() + + def test_hide_search_for_role_actually_hides_sidebar_search(self): + # Administrator not in list → search visible + self._set_theme(hide_notification=False, hide_for_roles=[]) + ctx, page = self._open() + self.assertFalse(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) + ctx.close() + + # Administrator in list → search hidden + self._set_theme(hide_notification=False, hide_for_roles=["Administrator"]) + ctx, page = self._open() + self.assertTrue(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) + ctx.close() From 899f526cfaddcda83d8c9c10b8e8410bb9640943 Mon Sep 17 00:00:00 2001 From: Sparsh Verma Date: Fri, 5 Jun 2026 12:32:42 +0530 Subject: [PATCH 3/3] fix: ruff formatter issue resolved --- .../doctype/desk_theme/test_desk_theme.py | 297 +++++++++--------- 1 file changed, 144 insertions(+), 153 deletions(-) diff --git a/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py b/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py index c63afe9..757fc23 100644 --- a/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py +++ b/frappe_desk_theme/frappe_desk_theme/doctype/desk_theme/test_desk_theme.py @@ -12,169 +12,160 @@ def _playwright_available() -> bool: - return importlib.util.find_spec("playwright") is not None + return importlib.util.find_spec("playwright") is not None class TestDeskTheme(unittest.TestCase): - """Schema + data-layer tests for the Desk Theme hide_search / - hide_notification fields. - - Uses plain unittest.TestCase (not IntegrationTestCase) because these - tests don't insert any Desk Theme rows — they only read meta and - round-trip the existing singleton. Avoiding IntegrationTestCase skips - Frappe's transitive fixture loader (Email Account → ToDo → …) which - can fail on downstream sites that customise those doctypes.""" - - def test_doctype_exposes_hide_notification_and_hide_search_fields(self): - """Field schema is the contract the bundle JS reads against — the - bug history was: schema field exists, JS never consumed it. This - test fails fast if a future refactor removes either field, so the - client-side toggle code wouldn't silently regress.""" - meta = frappe.get_meta("Desk Theme") - hide_notification = meta.get_field("hide_notification") - hide_search = meta.get_field("hide_search") - - self.assertIsNotNone(hide_notification, "hide_notification field removed") - self.assertEqual(hide_notification.fieldtype, "Check") - - self.assertIsNotNone(hide_search, "hide_search field removed") - self.assertEqual(hide_search.fieldtype, "Table MultiSelect") - self.assertEqual(hide_search.options, "Has Role") - - def test_api_round_trips_both_fields(self): - """The bundle JS reads both fields from get_custom_theme via /api/method. - Round-tripping through frappe.get_doc proves the data layer carries - both values to the client; if either disappears here, the JS reads - undefined and silently no-ops.""" - theme = frappe.get_single("Desk Theme") - prev_notif = theme.hide_notification - prev_roles = [row.role for row in (theme.hide_search or [])] - - try: - theme.hide_notification = 1 - theme.set("hide_search", []) - theme.append("hide_search", {"role": "Administrator"}) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep - - reloaded = frappe.get_single("Desk Theme") - self.assertEqual(reloaded.hide_notification, 1) - self.assertEqual( - [row.role for row in reloaded.hide_search], - ["Administrator"], - ) - finally: - theme = frappe.get_single("Desk Theme") - theme.hide_notification = prev_notif - theme.set("hide_search", []) - for role in prev_roles: - theme.append("hide_search", {"role": role}) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep + """Schema + data-layer tests for the Desk Theme hide_search / + hide_notification fields. + + Uses plain unittest.TestCase (not IntegrationTestCase) because these + tests don't insert any Desk Theme rows — they only read meta and + round-trip the existing singleton. Avoiding IntegrationTestCase skips + Frappe's transitive fixture loader (Email Account → ToDo → …) which + can fail on downstream sites that customise those doctypes.""" + + def test_doctype_exposes_hide_notification_and_hide_search_fields(self): + """Field schema is the contract the bundle JS reads against — the + bug history was: schema field exists, JS never consumed it. This + test fails fast if a future refactor removes either field, so the + client-side toggle code wouldn't silently regress.""" + meta = frappe.get_meta("Desk Theme") + hide_notification = meta.get_field("hide_notification") + hide_search = meta.get_field("hide_search") + + self.assertIsNotNone(hide_notification, "hide_notification field removed") + self.assertEqual(hide_notification.fieldtype, "Check") + + self.assertIsNotNone(hide_search, "hide_search field removed") + self.assertEqual(hide_search.fieldtype, "Table MultiSelect") + self.assertEqual(hide_search.options, "Has Role") + + def test_api_round_trips_both_fields(self): + """The bundle JS reads both fields from get_custom_theme via /api/method. + Round-tripping through frappe.get_doc proves the data layer carries + both values to the client; if either disappears here, the JS reads + undefined and silently no-ops.""" + theme = frappe.get_single("Desk Theme") + prev_notif = theme.hide_notification + prev_roles = [row.role for row in (theme.hide_search or [])] + + try: + theme.hide_notification = 1 + theme.set("hide_search", []) + theme.append("hide_search", {"role": "Administrator"}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + + reloaded = frappe.get_single("Desk Theme") + self.assertEqual(reloaded.hide_notification, 1) + self.assertEqual( + [row.role for row in reloaded.hide_search], + ["Administrator"], + ) + finally: + theme = frappe.get_single("Desk Theme") + theme.hide_notification = prev_notif + theme.set("hide_search", []) + for role in prev_roles: + theme.append("hide_search", {"role": role}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep @unittest.skipUnless(_playwright_available(), "playwright not installed") -@unittest.skipUnless( - os.environ.get("DESK_THEME_E2E_SITE"), "DESK_THEME_E2E_SITE not set" -) +@unittest.skipUnless(os.environ.get("DESK_THEME_E2E_SITE"), "DESK_THEME_E2E_SITE not set") class E2EDeskThemeToggle(IntegrationTestCase): - """End-to-end browser test that exercises the JS bundle directly. - - Set DESK_THEME_E2E_SITE=http://bsebeamv16.localhost:8003 (a running bench - with the user `Administrator`) before invoking. Skipped otherwise so - standard CI runs aren't blocked on the bench dev server. - - Browser launch lives in setUp / tearDown (per-instance) rather than - setUpClass / tearDownClass so the test file conforms to the project's - snake_case naming policy. The extra ~2s per test is negligible given - these tests only run in opt-in E2E mode. - """ - - def setUp(self): - super().setUp() - self._pw = sync_playwright().start() - self.browser = self._pw.chromium.launch(headless=True) - self.base = os.environ["DESK_THEME_E2E_SITE"].rstrip("/") - self._original_notif = frappe.db.get_single_value( - "Desk Theme", "hide_notification" - ) - - def tearDown(self): - try: - theme = frappe.get_single("Desk Theme") - theme.hide_notification = self._original_notif - theme.set("hide_search", []) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep - finally: - self.browser.close() - self._pw.stop() - super().tearDown() - - def _login_url(self) -> str: - """Mint a single-use SID for Administrator on the live site.""" - frappe.local.cookie_manager = CookieManager() - frappe.local.login_manager = LoginManager() - frappe.local.login_manager.login_as("Administrator") - sid = frappe.session.sid - return f"{self.base}/app?sid={sid}" - - def _open(self): - ctx = self.browser.new_context(viewport={"width": 1440, "height": 900}) - page = ctx.new_page() - page.goto(self._login_url(), wait_until="networkidle", timeout=60000) - page.goto(self.base + "/app/dashboard", wait_until="networkidle", timeout=60000) - # Wait for sidebar to render (polling — wait_for_selector is flaky on first paint) - for _ in range(60): - if ( - page.evaluate( - "document.querySelectorAll('.body-sidebar .sidebar-item-container').length" - ) - > 5 - ): - break - page.wait_for_timeout(250) - return ctx, page - - def _set_theme(self, hide_notification: bool, hide_for_roles: list[str]): - theme = frappe.get_single("Desk Theme") - theme.hide_notification = 1 if hide_notification else 0 - theme.set("hide_search", []) - for role in hide_for_roles: - theme.append("hide_search", {"role": role}) - theme.save(ignore_permissions=True) - frappe.db.commit() # nosemgrep - - def _is_hidden(self, page, selector: str) -> bool: - return page.evaluate(f"""() => {{ + """End-to-end browser test that exercises the JS bundle directly. + + Set DESK_THEME_E2E_SITE=http://bsebeamv16.localhost:8003 (a running bench + with the user `Administrator`) before invoking. Skipped otherwise so + standard CI runs aren't blocked on the bench dev server. + + Browser launch lives in setUp / tearDown (per-instance) rather than + setUpClass / tearDownClass so the test file conforms to the project's + snake_case naming policy. The extra ~2s per test is negligible given + these tests only run in opt-in E2E mode. + """ + + def setUp(self): + super().setUp() + self._pw = sync_playwright().start() + self.browser = self._pw.chromium.launch(headless=True) + self.base = os.environ["DESK_THEME_E2E_SITE"].rstrip("/") + self._original_notif = frappe.db.get_single_value("Desk Theme", "hide_notification") + + def tearDown(self): + try: + theme = frappe.get_single("Desk Theme") + theme.hide_notification = self._original_notif + theme.set("hide_search", []) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + finally: + self.browser.close() + self._pw.stop() + super().tearDown() + + def _login_url(self) -> str: + """Mint a single-use SID for Administrator on the live site.""" + frappe.local.cookie_manager = CookieManager() + frappe.local.login_manager = LoginManager() + frappe.local.login_manager.login_as("Administrator") + sid = frappe.session.sid + return f"{self.base}/app?sid={sid}" + + def _open(self): + ctx = self.browser.new_context(viewport={"width": 1440, "height": 900}) + page = ctx.new_page() + page.goto(self._login_url(), wait_until="networkidle", timeout=60000) + page.goto(self.base + "/app/dashboard", wait_until="networkidle", timeout=60000) + # Wait for sidebar to render (polling — wait_for_selector is flaky on first paint) + for _ in range(60): + if page.evaluate("document.querySelectorAll('.body-sidebar .sidebar-item-container').length") > 5: + break + page.wait_for_timeout(250) + return ctx, page + + def _set_theme(self, hide_notification: bool, hide_for_roles: list[str]): + theme = frappe.get_single("Desk Theme") + theme.hide_notification = 1 if hide_notification else 0 + theme.set("hide_search", []) + for role in hide_for_roles: + theme.append("hide_search", {"role": role}) + theme.save(ignore_permissions=True) + frappe.db.commit() # nosemgrep + + def _is_hidden(self, page, selector: str) -> bool: + return page.evaluate(f"""() => {{ const el = document.querySelector('{selector}'); if (!el) return null; const s = getComputedStyle(el); return s.display === 'none' || el.offsetHeight === 0; }}""") - def test_hide_notification_flag_actually_hides_sidebar_bell(self): - # Off → bell visible - self._set_theme(hide_notification=False, hide_for_roles=[]) - ctx, page = self._open() - self.assertFalse(self._is_hidden(page, ".body-sidebar .sidebar-notification")) - ctx.close() - - # On → bell hidden - self._set_theme(hide_notification=True, hide_for_roles=[]) - ctx, page = self._open() - self.assertTrue(self._is_hidden(page, ".body-sidebar .sidebar-notification")) - ctx.close() - - def test_hide_search_for_role_actually_hides_sidebar_search(self): - # Administrator not in list → search visible - self._set_theme(hide_notification=False, hide_for_roles=[]) - ctx, page = self._open() - self.assertFalse(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) - ctx.close() - - # Administrator in list → search hidden - self._set_theme(hide_notification=False, hide_for_roles=["Administrator"]) - ctx, page = self._open() - self.assertTrue(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) - ctx.close() + def test_hide_notification_flag_actually_hides_sidebar_bell(self): + # Off → bell visible + self._set_theme(hide_notification=False, hide_for_roles=[]) + ctx, page = self._open() + self.assertFalse(self._is_hidden(page, ".body-sidebar .sidebar-notification")) + ctx.close() + + # On → bell hidden + self._set_theme(hide_notification=True, hide_for_roles=[]) + ctx, page = self._open() + self.assertTrue(self._is_hidden(page, ".body-sidebar .sidebar-notification")) + ctx.close() + + def test_hide_search_for_role_actually_hides_sidebar_search(self): + # Administrator not in list → search visible + self._set_theme(hide_notification=False, hide_for_roles=[]) + ctx, page = self._open() + self.assertFalse(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) + ctx.close() + + # Administrator in list → search hidden + self._set_theme(hide_notification=False, hide_for_roles=["Administrator"]) + ctx, page = self._open() + self.assertTrue(self._is_hidden(page, ".body-sidebar .navbar-search-bar")) + ctx.close()