Skip to content

chore: make formgenerator better #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
harshtandiya merged 7 commits into from
Dec 24, 2025
Merged

chore: make formgenerator better #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9bd7011
feat: add `User Forms` module and update modules list
harshtandiya Dec 24, 2025
81c605e
feat: enhance form generation with error handling and permissions man…
harshtandiya Dec 24, 2025
bbc1bd2
fix: error handling
harshtandiya Dec 24, 2025
4c088a6
refactor: remove debug print statement from create_form function
harshtandiya Dec 24, 2025
3433fb5
chore: update Python version in CI workflow to 3.12
harshtandiya Dec 24, 2025
5823e4e
chore: update Python version in CI workflow to 3.14
harshtandiya Dec 24, 2025
91b062e
chore: update Node.js version in CI workflow to 24
harshtandiya Dec 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,12 @@ jobs:
- name: Setup Python
uses: actions/setup-python@v4
with:
python-version: '3.10'
python-version: "3.14"

- name: Setup Node
uses: actions/setup-node@v3
with:
node-version: 18
node-version: 24
check-latest: true

- name: Cache pip
Expand Down Expand Up @@ -96,7 +96,7 @@ jobs:
bench --site test_site install-app forms_pro
bench build
env:
CI: 'Yes'
CI: "Yes"

- name: Run Tests
working-directory: /home/runner/frappe-bench
Expand Down
3 changes: 2 additions & 1 deletion forms_pro/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
Forms Pro
Forms Pro
User Forms
Empty file added forms_pro/user_forms/__init__.py
View file
Open in desktop
Empty file.
110 changes: 83 additions & 27 deletions forms_pro/utils/form_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import frappe
from frappe import _
from frappe.share import add_docshare

FORMS_PRO_ROLE = "Forms Pro User"

Expand All @@ -7,10 +9,19 @@
def create_form_with_doctype(team_id: str, doctype: str):
roles = frappe.get_roles(frappe.session.user)
if FORMS_PRO_ROLE not in roles:
frappe.throw("You are not authorized to create a form")

form_generator = FormGenerator(team_id=team_id, linked_doctype=doctype)
form_generator.generate()
frappe.throw(
_("You are not authorized to create a form"),
frappe.PermissionError,
)

try:
form_generator = FormGenerator(team_id=team_id, linked_doctype=doctype)
form_generator.generate()
except Exception as e:
frappe.log_error(f"Error creating form with doctype: {doctype} - {e}")
frappe.throw(
_("Error creating form with doctype: {0} - {1}").format(doctype, e),
)

return {
"doctype": form_generator.doctype.name,
Expand All @@ -22,10 +33,19 @@ def create_form_with_doctype(team_id: str, doctype: str):
def create_form(team_id: str):
roles = frappe.get_roles(frappe.session.user)
if FORMS_PRO_ROLE not in roles:
frappe.throw("You are not authorized to create a form")

form_generator = FormGenerator(team_id=team_id)
form_generator.generate()
frappe.throw(
_("You are not authorized to create a form"),
frappe.PermissionError,
)

try:
form_generator = FormGenerator(team_id=team_id)
form_generator.generate()
except Exception as e:
frappe.log_error(f"Error creating form: {e}")
frappe.throw(
_("Error creating form: {0}").format(e),
)

Comment on lines +41 to 49
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix error handling issues.

The error handling has several issues:

  1. Catching bare Exception is too broad and can mask unexpected errors. Consider catching specific exceptions.
  2. Line 35 is unreachable because frappe.throw() on Line 34 raises an exception.
  3. The error is both logged and thrown, which may result in duplicate error reporting.
🔎 Proposed fix 
-    try:
-        form_generator = FormGenerator(team_id=team_id)
-        form_generator.generate()
-    except Exception as e:
-        frappe.log_error(f"Error creating form: {e}")
-        frappe.throw(f"Error creating form: {e}")
-        print(frappe.get_traceback())
+    form_generator = FormGenerator(team_id=team_id)
+    form_generator.generate()

If error handling is required for specific exceptions, catch those explicitly and remove the unreachable print statement.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Ruff (0.14.10)

32-32: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents 
In forms_pro/utils/form_generator.py around lines 29 to 36, replace the current
broad try/except that catches Exception, logs, calls frappe.throw (making the
following print unreachable) and prints the traceback; instead catch only the
specific exceptions you expect from FormGenerator (e.g., ValueError, KeyError or
a custom FormGenerationError), call frappe.log_error with the full traceback and
context when one of those specific exceptions occurs, then re-raise the original
exception (using plain raise) so the framework handles it — do not call
frappe.throw and remove the unreachable print(frappe.get_traceback()) to avoid
duplicate reporting.

return {
"doctype": form_generator.doctype.name,
Expand All @@ -47,46 +67,82 @@ def __init__(
def generate(self) -> None:
self._initialize_doctype()
self._initialize_form_document()
frappe.clear_cache()

def _initialize_doctype(self) -> None:
if self.doctype:
return

placeholder_doctype = frappe.new_doc("DocType")
placeholder_doctype.name = self._generate_doctype_name()
placeholder_doctype.module = "Forms Pro"
placeholder_doctype.module = "User Forms"
placeholder_doctype.custom = True
placeholder_doctype.track_changes = True
placeholder_doctype.track_views = True
placeholder_doctype.index_web_pages_for_search = False
placeholder_doctype.insert(ignore_permissions=True)

self.doctype = placeholder_doctype
# self.set_placeholder_doctype_fields()
placeholder_doctype.is_submittable = 0
placeholder_doctype.istable = 0
placeholder_doctype.editable_grid = 0
placeholder_doctype.issingle = 0
placeholder_doctype.is_tree = 0
placeholder_doctype.is_virtual = 0

# Add permissions
placeholder_doctype.append(
"permissions",
{
"create": 1,
"delete": 1,
"email": 1,
"export": 1,
"print": 1,
"read": 1,
"report": 1,
"role": "System Manager",
"share": 1,
"write": 1,
"submit": 0,
},
)

# Add fields
placeholder_doctype.append("fields", {"fieldtype": "Section Break"})

# def set_placeholder_doctype_fields(self) -> None:
# fields = [
# {
# "label": "Is Form Pro DocType",
# "fieldname": "is_forms_pro_doctype",
# "fieldtype": "Check",
# "reqd": 1,
# "read_only": 1,
# "default": 1,
# }
# ]
placeholder_doctype.insert(ignore_permissions=True)

# for field in fields:
# self.doctype.append("fields", field)
add_docshare(
"DocType",
placeholder_doctype.name,
read=1,
write=1,
share=1,
submit=0,
flags={
"ignore_share_permission": True,
},
)
Comment on lines +113 to +123
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Search for add_docshare definition in the codebase
rg "def add_docshare" -B 2 -A 10

Repository: BuildWithHussain/forms_pro

Length of output: 196

🏁 Script executed:

# Also search for imports of add_docshare to understand where it comes from
rg "add_docshare" --type py -B 2 -A 2 | head -50

Repository: BuildWithHussain/forms_pro

Length of output: 196

🏁 Script executed:

# Check the specific file to see the import statement
cat -n forms_pro/utils/form_generator.py | head -30

Repository: BuildWithHussain/forms_pro

Length of output: 196

🌐 Web query:

Frappe add_docshare function parameters default user

💡 Result:

The add_docshare function signature in Frappe is commonly called as:

add_docshare(doctype, name, user=None, everyone=None, read=0, write=0, submit=0, share=0).

  • user defaults to None (pass a user email/ID to share with a specific user). [1][2]

Sources:

  1. Frappe forum example showing usage with user and write flag. [1]
  2. Frappe repository share.py (function definition / implementation). [2]

🌐 Web query:

frappe.share.add_docshare documentation user parameter

💡 Result:

The user parameter is the user identifier (the User.name / email string) to share the document with — e.g. frappe.session.user or each user from a User Group (pass a string like "user@example.com"). Examples and usage in the Frappe forum show calls such as add_docshare(doctype, name, user=frappe.session.user, ...) and looping over User Group members passing user.user. [1][2]

Sources:

  • Frappe forum example using add_docshare with user parameter. [1]
  • Frappe forum custom-script example calling frappe.share.call with user: frappe.session.user. [2]

Add user=frappe.session.user to the add_docshare call.

The add_docshare function defaults the user parameter to None, not the current user. To share the DocType with the user who created it, explicitly pass user=frappe.session.user to the function call at line 114.

🤖 Prompt for AI Agents 
In forms_pro/utils/form_generator.py around lines 113 to 123, the add_docshare
call is not passing the creating user so the share will not be tied to the
session user; update the add_docshare invocation to include
user=frappe.session.user (i.e., add the user keyword argument to the existing
parameter list) so the DocType is shared with the current session user; ensure
frappe.session is referenced where this function runs.


# self.doctype.save(ignore_permissions=True)
self.doctype = placeholder_doctype

def _initialize_form_document(self) -> None:
form_document = frappe.new_doc("Form")
form_document.linked_doctype = self.doctype.name
form_document.title = "Untitled Form"
form_document.linked_team_id = self.team_id
form_document.document_type = "System"
form_document.insert(ignore_permissions=True)

add_docshare(
"Form",
form_document.name,
read=1,
write=1,
share=1,
submit=0,
flags={
"ignore_share_permission": True,
},
)
self.form_document = form_document

def _generate_doctype_name(self) -> str:
Expand Down
58 changes: 56 additions & 2 deletions forms_pro/utils/test_form_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def test_generate_creates_doctype_when_none_provided(self):
# Assertions
self.assertIsNotNone(form_generator.doctype)
self.assertTrue(form_generator.doctype.name.startswith("formspro_"))
self.assertEqual(form_generator.doctype.module, "Forms Pro")
self.assertEqual(form_generator.doctype.module, "User Forms")
self.assertTrue(form_generator.doctype.custom)
self.assertTrue(form_generator.doctype.track_changes)
self.assertTrue(form_generator.doctype.track_views)
Expand Down Expand Up @@ -133,7 +133,7 @@ def test_generate_complete_flow(self):

# Verify DocType properties
self.assertTrue(form_generator.doctype.name.startswith("formspro_"))
self.assertEqual(form_generator.doctype.module, "Forms Pro")
self.assertEqual(form_generator.doctype.module, "User Forms")
self.assertTrue(form_generator.doctype.custom)

def test_generate_with_existing_doctype_complete_flow(self):
Expand All @@ -157,3 +157,57 @@ def test_generate_with_existing_doctype_complete_flow(self):

# Verify Form document exists in database
self.assertTrue(frappe.db.exists("Form", form_generator.form_document.name))

def test_generate_creates_doctype_docshare(self):
"""Test that generate() creates DocShare for the DocType with correct permissions"""
frappe.set_user(self.test_user)
form_generator = FormGenerator(team_id=self.test_team)

# Call generate method
form_generator.generate()

# Query DocShare for the DocType
docshare = frappe.db.get_value(
"DocShare",
{
"share_doctype": "DocType",
"share_name": form_generator.doctype.name,
"user": self.test_user,
},
["read", "write", "share", "submit"],
as_dict=True,
)

# Assertions
self.assertIsNotNone(docshare, "DocShare should exist for DocType")
self.assertEqual(docshare.read, 1, "DocShare should have read permission")
self.assertEqual(docshare.write, 1, "DocShare should have write permission")
self.assertEqual(docshare.share, 1, "DocShare should have share permission")
self.assertEqual(docshare.submit, 0, "DocShare should not have submit permission")

def test_generate_creates_form_docshare(self):
"""Test that generate() creates DocShare for the Form document with correct permissions"""
frappe.set_user(self.test_user)
form_generator = FormGenerator(team_id=self.test_team)

# Call generate method
form_generator.generate()

# Query DocShare for the Form document
docshare = frappe.db.get_value(
"DocShare",
{
"share_doctype": "Form",
"share_name": form_generator.form_document.name,
"user": self.test_user,
},
["read", "write", "share", "submit"],
as_dict=True,
)

# Assertions
self.assertIsNotNone(docshare, "DocShare should exist for Form document")
self.assertEqual(docshare.read, 1, "DocShare should have read permission")
self.assertEqual(docshare.write, 1, "DocShare should have write permission")
self.assertEqual(docshare.share, 1, "DocShare should have share permission")
self.assertEqual(docshare.submit, 0, "DocShare should not have submit permission")