refactor(permissions): return pypika criteria from query condition hooks

[netchampfaris]

What this does

Gameplan's permission_query_conditions hooks build the WHERE clause that limits which rows a user can see (spaces, discussions, pages, comments, drafts, etc.).

Until now each hook built a pypika criterion and then turned it into a SQL string itself, through a local criterion_sql() helper that called criterion.get_sql(...). This PR returns the criterion directly and removes that helper.

Why

criterion_sql() inlined values using pypika's own escaping, which only doubles single quotes. On MariaDB the backslash is also an escape character, so that escaping is not safe for arbitrary values.

Frappe now accepts a pypika term straight from these hooks and renders the values through the database driver's escaping (the same path the query builder already uses for bound parameters). So the hooks no longer need to render SQL themselves — they just describe the condition and let the framework render it safely.

The result is less code and safer value handling, with no change to who can see what.

Depends on

Frappe PR frappe/frappe#40408 — adds pypika support to permission_query_conditions. This PR cannot be merged or pass server tests until that change is available in the frappe branch Gameplan's CI installs.

Warning

Gameplan CI installs frappe with bench init (no --frappe-branch), which uses frappe's default stable branch. frappe#40408 targets develop. The server tests here will stay red until the pypika support lands in the branch CI actually uses (either by backporting #40408 or by pointing CI at the branch that has it). Kept as a draft for that reason.

no-docs

[github-actions]

UI Test Results

✅ All passed — 37/37 tests passed in 1m 47s.

Spec	Tests	✅	❌	⏭	⏱
comment.cy.js	1	1	0	0	13s
community-composer.cy.ts	3	3	0	0	7s
community-discussions-actions.cy.ts	1	1	0	0	6s
community-merge-url-healing.cy.ts	1	1	0	0	4s
community-mobile-home.cy.ts	1	1	0	0	3s
community-naming.cy.ts	3	3	0	0	5s
community-scoped-links.cy.ts	3	3	0	0	3s
community-shell.cy.ts	3	3	0	0	3s
community-smoke.cy.ts	3	3	0	0	3s
community-spaces-guardrails.cy.ts	2	2	0	0	3s
discussion.cy.js	1	1	0	0	13s
drafts-comment.cy.ts	2	2	0	0	7s
member-management.cy.ts	2	2	0	0	4s
mobile-more-pages.cy.ts	4	4	0	0	6s
new-discussion.cy.ts	2	2	0	0	10s
onboarding.cy.js	1	1	0	0	2s
page.cy.js	1	1	0	0	2s
project.cy.js	1	1	0	0	5s
search-privacy.cy.ts	1	1	0	0	3s
task.cy.js	1	1	0	0	6s
Total	37	37	0	0	1m 47s

_{Results for commit fe48588.}

[greptile-apps]

Confidence Score: 4/5

Safe to merge once frappe/frappe#40408 is available in the CI-installed branch; merging before that point would cause Frappe to receive pypika objects instead of strings from every permission hook, which could silently break row-level visibility for all doctypes.

The code change itself is logically correct — every hook preserves its None-for-admin fast path and the criterion objects built are identical to those that were previously serialised by criterion_sql(). The only risk is the hard runtime dependency on the upstream Frappe change; the PR is appropriately kept as a draft for that reason.

gameplan/permissions.py — the change is intentionally incomplete without frappe#40408; no internal logic issues found.

Important Files Changed

Filename	Overview
gameplan/permissions.py	Removes criterion_sql() and returns pypika criteria directly from all query-condition hooks; logic is unchanged but requires frappe#40408 to be available before merging.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Frappe list/get_all] -->|calls permission_query_conditions hook| B{hook function}
    B --> C[team_query_conditions]
    B --> D[project_query_conditions]
    B --> E[discussion / task query_conditions]
    B --> F[page_query_conditions]
    B --> G[comment_query_conditions]
    B --> H[draft_query_conditions]

    C -->|is_global_admin?| I[return None — no filter]
    C -->|else| J[return pypika Criterion]

    D -->|is_global_admin?| I
    D -->|else| J

    E -->|is_global_admin?| I
    E -->|else| J

    F -->|is_global_admin?| I
    F -->|else| J

    G -->|is_global_admin?| I
    G -->|else| J

    H --> J

    J -->|frappe#40408 required| K[Frappe renders SQL via DB driver escaping]
    I --> L[No WHERE clause appended]
    K --> M[Safe parameterised query]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Frappe list/get_all] -->|calls permission_query_conditions hook| B{hook function}
    B --> C[team_query_conditions]
    B --> D[project_query_conditions]
    B --> E[discussion / task query_conditions]
    B --> F[page_query_conditions]
    B --> G[comment_query_conditions]
    B --> H[draft_query_conditions]

    C -->|is_global_admin?| I[return None — no filter]
    C -->|else| J[return pypika Criterion]

    D -->|is_global_admin?| I
    D -->|else| J

    E -->|is_global_admin?| I
    E -->|else| J

    F -->|is_global_admin?| I
    F -->|else| J

    G -->|is_global_admin?| I
    G -->|else| J

    H --> J

    J -->|frappe#40408 required| K[Frappe renders SQL via DB driver escaping]
    I --> L[No WHERE clause appended]
    K --> M[Safe parameterised query]

Loading

_{Reviews (1): Last reviewed commit: "refactor(permissions): return pypika cri..." | Re-trigger Greptile}