feat: add banking app screen hook#369
Open
barredterra wants to merge 1 commit into
Open
Conversation
Comment on lines
+10
to
+17
| add_to_apps_screen = [ | ||
| { | ||
| "name": app_name, | ||
| "logo": "/assets/banking/images/logo.png", | ||
| "title": app_title, | ||
| "route": app_home, | ||
| } | ||
| ] |
There was a problem hiding this comment.
Missing
has_permission guard on apps-screen entry
Without a has_permission callable, the Banking app card is shown to every authenticated user on the apps screen — including users who have no banking-related roles or permissions. Frappe's add_to_apps_screen hook supports an optional has_permission dotted-path string (e.g. "banking.hooks.has_banking_permission") that lets you hide the tile for users who shouldn't see it.
595d2dd to
309a844
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
pre-commit run ruff --files banking/hooks.pypre-commit run ruff-format --files banking/hooks.pyGreptile Summary
This PR adds an apps-screen entry for the ALYF Banking app by introducing the
add_to_apps_screenhook and anapp_homevariable pointing to the v16 workspace sidebar route.app_home = "/desk/alyf-banking"to define the default landing route for the banking sidebar, and reuses it in the newadd_to_apps_screenlist so the banking tile routes correctly./assets/banking/images/logo.png) resolves to an existing asset, andapp_name/app_titleare used by reference so they stay in sync with the top-level declarations.Confidence Score: 4/5
Safe to merge; the change is small and isolated to hook registration with no impact on existing functionality.
The change is minimal — two new module-level declarations in
hooks.py. The logo asset already exists, variables are referenced correctly, and the target route matches theWorkspace Sidebarrecord. The only open question is whether the apps-screen tile should carry ahas_permissioncheck to avoid showing it to users without banking access.No files require special attention beyond the optional
has_permissionconsideration inbanking/hooks.py.Important Files Changed
app_homevariable andadd_to_apps_screenhook to register the banking app on Frappe's apps screen; logo path resolves correctly, no breaking changes to existing hooks.Sequence Diagram
sequenceDiagram actor User participant AppsScreen as Frappe Apps Screen participant Hook as add_to_apps_screen hook participant BankingApp as Banking App (/desk/alyf-banking) User->>AppsScreen: Open apps screen AppsScreen->>Hook: Load app entries Hook-->>AppsScreen: "{name:"banking", logo, title:"ALYF Banking", route:"/desk/alyf-banking"}" AppsScreen-->>User: Render Banking tile User->>AppsScreen: Click Banking tile AppsScreen->>BankingApp: Navigate to /desk/alyf-banking BankingApp-->>User: Workspace Sidebar (ALYF Banking)Reviews (1): Last reviewed commit: "feat: add banking app screen hook" | Re-trigger Greptile