Conversation
46cebb1 to
8ec4a62
Compare
c689b9e to
ecafda8
Compare
ecafda8 to
48d788b
Compare
There was a problem hiding this comment.
Pull request overview
Refactors and expands several user-facing frappe.throw(...) messages across the Press API to be more descriptive and (in some cases) include documentation links.
Changes:
- Updated multiple API validation/permission error messages to be more actionable and consistent.
- Added documentation links to some error messages (Google OAuth, SaaS login flow, bench operations, asset store credentials).
- Minor cleanup/formatting (e.g., removed legacy encoding header, adjusted function formatting).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| press/api/google.py | Improves Google OAuth-related error messages; adds doc link for missing credentials. |
| press/api/email.py | Makes analytics request validation error more specific. |
| press/api/developer/saas.py | Makes SaaS login flow errors more actionable; adds “Learn more” links. |
| press/api/dashboard.py | Improves tag duplication error message. |
| press/api/central.py | Refines signup/trial validation errors and fixes a typo in “Country field”. |
| press/api/billing.py | Tweaks invoice download permission error wording. |
| press/api/bench.py | Refines multiple bench/deploy/dependency validation messages; adds doc links. |
| press/api/assets.py | Improves asset upload/build-token errors; adds doc link for build token requirement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| frappe.throw( | ||
| "Sorry, you cannot login with this method as 2FA is enabled. Please visit https://frappecloud.com/dashboard to login." | ||
| "Sorry, you cannot login with this method as 2FA is enabled. Please visit https://frappecloud.com/dashboard to login.<a href='https://docs.frappe.io/cloud/two-factor-authentication-2fa' target='_blank'>Learn more</a>" | ||
| ) |
There was a problem hiding this comment.
This message embeds a target="_blank" external link. Add rel="noopener noreferrer" to prevent reverse-tabnabbing, and add a space before the tag so the rendered text doesn’t run together ("login.<a ...").
| frappe.throw( | ||
| "Only SaaS sites are allowed to login to Frappe Cloud via this method.<a href='https://docs.frappe.io/cloud/simplified-login-to-frappe-cloud-dashboard' target='_blank'>Learn more</a>" | ||
| ) |
There was a problem hiding this comment.
This message embeds a target="_blank" external link. Add rel="noopener noreferrer" to prevent reverse-tabnabbing, and add a space before the tag so the rendered text doesn’t run together ("method.<a ...").
press/api/bench.py
Outdated
| if len(rg.dependencies) != len(dependencies): | ||
| frappe.throw("Need all required dependencies") | ||
| frappe.throw( | ||
| "All required dependencies must be provided. <a href='https://docs.frappe.io/cloud/common-issues/required-app-not-found' target='_blank'>Learn more</a>" |
There was a problem hiding this comment.
Changing this exception text will break existing API tests that assert on the old substring (e.g. press/api/tests/test_bench.py expects "Need all required dependencies"). Update the tests accordingly, or keep the previous phrase in the new message so assertions remain stable.
| "All required dependencies must be provided. <a href='https://docs.frappe.io/cloud/common-issues/required-app-not-found' target='_blank'>Learn more</a>" | |
| "Need all required dependencies. All required dependencies must be provided. <a href='https://docs.frappe.io/cloud/common-issues/required-app-not-found' target='_blank'>Learn more</a>" |
| if not re.match(r"^\d+\.\d+\.*\d*$", new["value"]): | ||
| frappe.throw(f"Invalid version for {new['key']}") | ||
| frappe.throw( | ||
| f"Invalid version format for {new['key']}. Expected X.Y.Z (e.g., 14.0.0). <a href='https://docs.frappe.io/cloud/common-issues/incompatible-app-version' target='_blank'>Learn more</a>" |
There was a problem hiding this comment.
The validation uses regex ^\d+.\d+.\d$, which accepts versions like "3.6" as well as "3.6.0". The updated error message claims it expects X.Y.Z, which is inaccurate given the current validation; either tighten the regex to require 3 segments or adjust the message to match what’s accepted.
| f"Invalid version format for {new['key']}. Expected X.Y.Z (e.g., 14.0.0). <a href='https://docs.frappe.io/cloud/common-issues/incompatible-app-version' target='_blank'>Learn more</a>" | |
| f"Invalid version format for {new['key']}. Expected X.Y or X.Y.Z (e.g., 14.0 or 14.0.0). <a href='https://docs.frappe.io/cloud/common-issues/incompatible-app-version' target='_blank'>Learn more</a>" |
| frappe.throw( | ||
| "Bench can only be deployed by the bench owner. <a href='https://docs.frappe.io/cloud/role-permissions' target='_blank'>Learn more</a>", | ||
| exc=frappe.PermissionError, | ||
| ) |
There was a problem hiding this comment.
New/updated error messages in this file embed external links with target="_blank". Add rel="noopener noreferrer" to these anchors to prevent reverse-tabnabbing (applies to other similar links introduced in this file).
press/api/assets.py
Outdated
| if not build_token: | ||
| frappe.throw("Build token is required to access asset store credentials", frappe.PermissionError) | ||
| frappe.throw( | ||
| "Build token is required to access asset store credentials. <a href='https://docs.frappe.io/cloud/local-fc-setup' target='_blank'>Learn more</a>", |
There was a problem hiding this comment.
This error message embeds an external link with target="_blank". Add rel="noopener noreferrer" to the anchor to prevent reverse-tabnabbing.
| "Build token is required to access asset store credentials. <a href='https://docs.frappe.io/cloud/local-fc-setup' target='_blank'>Learn more</a>", | |
| "Build token is required to access asset store credentials. <a href='https://docs.frappe.io/cloud/local-fc-setup' target='_blank' rel='noopener noreferrer'>Learn more</a>", |
| if team_name and not team_enabled: | ||
| frappe.throw(_("Account {0} has been deactivated").format(email)) | ||
| frappe.throw( | ||
| _("Account {0} has been deactivated. Please contact support to reactivate your account. ").format( |
There was a problem hiding this comment.
The translated deactivation error message has a trailing space at the end of the sentence, which can lead to awkward formatting (e.g., double-spaces when concatenated/rendered). Trim the trailing whitespace in the string literal.
| _("Account {0} has been deactivated. Please contact support to reactivate your account. ").format( | |
| _("Account {0} has been deactivated. Please contact support to reactivate your account.").format( |
| frappe.throw("google_credentials not found in site_config.json") | ||
| frappe.throw( | ||
| "Google credentials not found in site_config.json. Please configure Google OAuth credentials for your site. " | ||
| '<a href="https://docs.frappe.io/framework/user/en/integration/google_drive" target="_blank">Learn more</a>' |
There was a problem hiding this comment.
This error message embeds an external link with target="_blank". To avoid reverse-tabnabbing, add rel="noopener noreferrer" to the anchor (this file includes similar external links).
| '<a href="https://docs.frappe.io/framework/user/en/integration/google_drive" target="_blank">Learn more</a>' | |
| '<a href="https://docs.frappe.io/framework/user/en/integration/google_drive" target="_blank" rel="noopener noreferrer">Learn more</a>' |
| frappe.throw( | ||
| "Invalid dependencies: " | ||
| + ", ".join(diff) | ||
| + ". <a href='https://docs.frappe.io/cloud/common-issues/incompatible-dependency-version' target='_blank'>Learn more</a>" | ||
| ) |
There was a problem hiding this comment.
Fix tests for this as well.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.