chore: make formgenerator better #7
Conversation
…agement - Added error handling in create_form to log and throw exceptions. - Updated DocType module name to "User Forms". - Implemented permissions management for DocType and Form documents using DocShare. - Added tests to verify DocShare creation with correct permissions for both DocType and Form.
|
Warning Rate limit exceeded@harshtandiya has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModule list now includes "User Forms". FormGenerator gained an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FormGenerator
participant FrappeCache as "Frappe Cache"
participant DocTypeService as "DocType"
participant DocShareService as "DocShare"
participant FormDoc as "Form Document"
Client->>FormGenerator: generate(team_id, linked_doctype?)
FormGenerator->>FrappeCache: clear_cache()
FormGenerator->>DocTypeService: _generate_doctype_name()
FormGenerator->>DocTypeService: _initialize_doctype(..., module="User Forms")
Note right of DocTypeService: flags set to 0\npermissions added\nsection break field
DocTypeService->>DocTypeService: insert(ignore_permissions=true)
FormGenerator->>DocShareService: add_docshare(doctype, user, ignore_share_permission=True)
FormGenerator->>FormDoc: _initialize_form_document(document_type="System")
FormDoc->>FormDoc: insert(...)
FormGenerator->>DocShareService: add_docshare(form, user, ignore_share_permission=True)
FormGenerator-->>Client: return created form
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forms_pro/utils/form_generator.py (1)
159-159: Inconsistent module filter in DocType name generation.The
_generate_doctype_namemethod counts existing DocTypes from the "Forms Pro" module, but new DocTypes are created with module "User Forms" (Line 65). This inconsistency could lead to name collisions or incorrect serial numbers.Consider updating the filter to count DocTypes from "User Forms" module instead:
🔎 Proposed fix
- count = frappe.db.count("DocType", filters={"module": "Forms Pro", "name": ["like", "formspro_%"]}) + count = frappe.db.count("DocType", filters={"module": "User Forms", "name": ["like", "formspro_%"]})Alternatively, if you want to maintain continuity with existing DocTypes, count from both modules:
count = frappe.db.count("DocType", filters={"module": ["in", ["Forms Pro", "User Forms"]], "name": ["like", "formspro_%"]})
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
forms_pro/modules.txtforms_pro/user_forms/__init__.pyforms_pro/utils/form_generator.pyforms_pro/utils/test_form_generator.py
🧰 Additional context used
🧬 Code graph analysis (1)
forms_pro/utils/test_form_generator.py (1)
forms_pro/utils/form_generator.py (2)
FormGenerator(43-170)generate(54-57)
🪛 Ruff (0.14.10)
forms_pro/utils/form_generator.py
32-32: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
forms_pro/modules.txt (1)
1-2: LGTM!The addition of the "User Forms" module entry is clean and aligns with the module name used in the DocType initialization.
forms_pro/utils/test_form_generator.py (2)
77-77: LGTM!The module assertion updates correctly reflect the module name change from "Forms Pro" to "User Forms" in the implementation.
Also applies to: 136-136
161-213: Excellent test coverage for DocShare functionality!The new tests thoroughly verify that DocShare entries are created with correct permissions for both the DocType and Form documents. The tests appropriately set user context and validate all expected permission flags.
forms_pro/utils/form_generator.py (5)
44-48: LGTM!The addition of type hints improves code clarity and makes the API more explicit.
57-57: LGTM!Adding cache clearing ensures that newly created DocTypes and Forms are immediately visible throughout the system.
65-96: Good DocType initialization with explicit configuration.The changes improve the DocType initialization by:
- Explicitly setting all relevant flags to 0 for clarity
- Adding a permissions entry for System Manager
- Including a Section Break field
- Updating the module name to "User Forms"
100-110: LGTM!The DocShare implementation correctly grants the creating user read, write, and share permissions on the DocType, while appropriately excluding submit permissions. The use of
ignore_share_permissionflag is necessary for creation.
137-137: LGTM!The Form initialization correctly sets
document_typeto "System" and implements DocShare with appropriate permissions matching the DocType pattern.Also applies to: 140-150
| 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()) | ||
|
|
There was a problem hiding this comment.
Fix error handling issues.
The error handling has several issues:
- Catching bare
Exceptionis too broad and can mask unexpected errors. Consider catching specific exceptions. - Line 35 is unreachable because
frappe.throw()on Line 34 raises an exception. - 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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forms_pro/utils/form_generator.py (1)
148-164: Fix module filter mismatch in DocType counting.The
_generate_doctype_namemethod counts DocTypes from the "Forms Pro" module (Line 154), but_initialize_doctypecreates DocTypes in the "User Forms" module (Line 78). This mismatch means:
- The counter will never include newly created DocTypes
- Multiple DocTypes may receive the same sequence number
- This could lead to naming collisions
🔎 Proposed fix
def _generate_doctype_name(self) -> str: """ Generate a unique DocType name with format: formspro_XXXXXX_YYYYYY where XXXXXX is a random 6-character string and YYYYYY is a serialized number """ - # Count existing formspro_* DocTypes from Forms Pro module - count = frappe.db.count("DocType", filters={"module": "Forms Pro", "name": ["like", "formspro_%"]}) + # Count existing formspro_* DocTypes from User Forms module + count = frappe.db.count("DocType", filters={"module": "User Forms", "name": ["like", "formspro_%"]}) # Next number is count + 1 next_number = count + 1Alternatively, if you want to count both old and new modules during migration:
- # Count existing formspro_* DocTypes from Forms Pro module - count = frappe.db.count("DocType", filters={"module": "Forms Pro", "name": ["like", "formspro_%"]}) + # Count existing formspro_* DocTypes from both Forms Pro and User Forms modules + count = frappe.db.count("DocType", filters={"name": ["like", "formspro_%"]})
🧹 Nitpick comments (1)
forms_pro/utils/form_generator.py (1)
70-70: Consider targeted cache clearing for better performance.
frappe.clear_cache()clears all caches globally, which can be expensive in production. Since you're only creating a DocType and Form, consider clearing specific caches instead.🔎 Alternative approach
- frappe.clear_cache() + frappe.clear_cache(doctype="DocType") + frappe.clear_cache(doctype="Form")Or use
frappe.cache().delete_value()for even more targeted clearing if you know the specific cache keys.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
forms_pro/utils/form_generator.py
🧰 Additional context used
🪛 Ruff (0.14.10)
forms_pro/utils/form_generator.py
20-20: Do not catch blind exception: Exception
(BLE001)
44-44: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
forms_pro/utils/form_generator.py (7)
2-3: LGTM!The new imports support translation and DocShare functionality introduced in this PR.
78-88: LGTM!Module renamed to "User Forms" per PR objectives, and flags are explicitly initialized to ensure predictable defaults.
108-109: LGTM!Adding a placeholder Section Break field ensures the DocType has at least one field, which is a valid approach for template DocTypes.
135-145: Verify DocShare is created for the correct user.Same as the DocType sharing: confirm that
add_docshareshares the Form with the user who created it by default.
17-24: Catch specific exceptions instead of bareException.Catching bare
Exceptionis too broad and can mask unexpected errors likeKeyboardInterrupt,SystemExit, or programming bugs. Review theFormGenerator.generate()method to determine what exceptions it can actually raise, then catch only those specific exception types rather than the catch-allException.
90-106: Clarify the dual-access permission model.The code grants "System Manager" role-based permissions while also using DocShare to provide the creating user direct access. Verify this design is intentional: users without "System Manager" role should still have full access through DocShare, but role-based access controls may be confusing or unnecessary in this context.
41-48: Catch specific exceptions instead of bareException.Same issue as in
create_form_with_doctype: catching bareExceptionis too broad and can mask unexpected errors. Adjust the caught exception types based on whatFormGeneratorandgenerate()can raise.
| add_docshare( | ||
| "DocType", | ||
| placeholder_doctype.name, | ||
| read=1, | ||
| write=1, | ||
| share=1, | ||
| submit=0, | ||
| flags={ | ||
| "ignore_share_permission": True, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for add_docshare definition in the codebase
rg "def add_docshare" -B 2 -A 10Repository: 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 -50Repository: 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 -30Repository: 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:
- Frappe forum example showing usage with user and write flag. [1]
- 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.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
99-99: Verify the CI environment variable type change.The CI variable was changed from a boolean (
Yes) to a string ("Yes"). While this likely won't cause issues, please confirm that frappe-bench handles both formats correctly, as this change wasn't mentioned in the PR description.
.github/workflows/ci.yml
Outdated
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: '3.10' | ||
| python-version: "3.12" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
frappe-bench Python 3.12 compatibility support
💡 Result:
Short answer
- Frappe/bench itself can work with Python 3.12 in recent releases, but official stable documentation and supported-stack recommendations still list Python 3.10+ as the baseline for production. Use Python 3.10 (or 3.11 where explicitly documented) for the most reliable results unless you are using a develop/bleeding-edge branch that explicitly adds 3.12 support. [1][2][3][4]
Details (concise)
- Official install docs (current stable lines) list Python 3.10+ as the required Python version. This is the safe production target. [1]
- bench releases have added the ability to create venvs with a chosen Python interpreter; that allows using Python 3.12 if your bench release and the Frappe branch support it. Check your bench version and release notes. [3]
- Frappe/ERPNext app compatibility varies by major version. Community/issue threads show Python 3.12 support being added/worked on in the develop branches (and some users report running ERPNext on 3.12), but several stable app versions (e.g., v14/v15 historically) required 3.10 minimum and needed explicit fixes for 3.12. Verify the specific Frappe/ERPNext branch you plan to run. [2][4]
Recommendation
- For production: use the Python version listed in the official docs for the Frappe/ERPNext release you plan to run (usually 3.10; confirm per-release). [1]
- If you want to test Python 3.12: use a non-production environment, upgrade bench to the latest release, use the develop/branch that lists py3.12 support, run tests/migrations, and watch for package issues (RestrictedPython, other deps). [3][4]
If you tell me which Frappe/ERPNext version and your OS (Ubuntu release), I’ll check the exact branch docs and confirm whether that version has explicit Python 3.12 support and any required workarounds.
Sources
- Frappe install docs (system requirements). [1]
- Frappe/ERPNext forum discussions re: Python 3.12 compatibility. [2][4]
- frappe/bench release notes (venv/python handling). [3]
Verify Python 3.12 compatibility with your Frappe/ERPNext version.
Python 3.12 is not officially stable in most frappe-bench releases; official documentation recommends Python 3.10+. While recent bench releases and develop branches may support 3.12, confirm that your specific Frappe/ERPNext version explicitly supports Python 3.12 and has no known compatibility issues before merging this change.
🤖 Prompt for AI Agents
.github/workflows/ci.yml around line 49: the workflow sets python-version to
"3.12" but many Frappe/ERPNext releases officially support up to Python 3.10;
verify your exact Frappe/ERPNext and bench versions explicitly support 3.12
before merging. Either change the workflow to a known-supported version (e.g.,
3.10), or convert the job to a matrix that runs 3.10 (required) and 3.12
(optional) so CI validates compatibility; update the PR with links to the tested
Frappe/ERPNext/bench commit ranges and add a short note in the workflow header
describing why 3.12 is included (or remove it if unsupported).
User FormsSummary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.