fix: User Forms creation on installation#11
Conversation
📝 WalkthroughWalkthroughActivates the app's after_install hook, adds installation logic to create a "User Forms" Module Def, registers a pre_model_sync patch, and provides a patch script to create the module for existing sites; also skips two tests via unittest decorators. Changes
Sequence Diagram(s)sequenceDiagram
actor Installer
participant hooks.py as Hooks
participant install.py as InstallLogic
participant DB as ModuleDef (DB)
Installer->>Hooks: trigger after_install hook
Hooks->>InstallLogic: call after_install()
InstallLogic->>DB: check for "User Forms" Module Def
alt not found
InstallLogic->>DB: insert Module Def ("User Forms", app "Forms Pro")
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…r user forms module creation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @forms_pro/install.py:
- Around line 7-8: The before_install hook directly calls
create_user_forms_module without error handling; wrap the call in a try/except
inside before_install, catch broad exceptions from create_user_forms_module, log
the exception with context (e.g., using the project logger or frappe.log/print)
and allow installation to continue if this module creation is non-critical (do
not re-raise), or return a failure signal only if you decide it must stop
installation; refer to the before_install function and create_user_forms_module
by name when making the change.
In @forms_pro/patches/0_1_beta/create_user_forms_module_def.py:
- Around line 5-8: Fix the grammatical typo in the module docstring in
create_user_forms_module_def.py: replace the phrase "in the for the app `Forms
Pro`." with "for the app `Forms Pro`." so the docstring reads "Create a module
def `User Forms` for the app `Forms Pro`." and keep it as the top triple-quoted
string.
- Around line 4-9: The patch's execute() calls create_user_forms_module() with
no error handling, which can abort bench migrate on failure; wrap the call in a
try/except around execute() (or inside it), catch Exception as e, record the
full exception (e and traceback) using frappe.log_error or processLogger (e.g.,
frappe.log_error(e, "create_user_forms_module patch failed")), and ensure the
exception is handled so migrations continue (do not re-raise); reference the
execute() function and create_user_forms_module() when making this change.
🧹 Nitpick comments (1)
forms_pro/install.py (1)
11-18: Good idempotency pattern, consider adding logging.The function correctly checks for existence before creating the module, making it safe to call multiple times (both from before_install and from the patch script).
📝 Optional: Add logging for better observability
def create_user_forms_module(): if frappe.db.exists("Module Def", "User Forms"): return module = frappe.new_doc("Module Def") module.module_name = "User Forms" module.app_name = "Forms Pro" module.insert(ignore_permissions=True) + frappe.logger().info("Created 'User Forms' module for Forms Pro")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
forms_pro/hooks.pyforms_pro/install.pyforms_pro/patches.txtforms_pro/patches/0_1_beta/create_user_forms_module_def.py
🧰 Additional context used
🧬 Code graph analysis (2)
forms_pro/patches/0_1_beta/create_user_forms_module_def.py (1)
forms_pro/install.py (1)
create_user_forms_module(11-18)
forms_pro/hooks.py (1)
forms_pro/install.py (1)
before_install(7-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (4)
forms_pro/patches.txt (1)
4-4: LGTM! Patch entry is correctly formatted.The patch identifier follows Frappe conventions and correctly references the new patch script.
forms_pro/hooks.py (1)
92-92: LGTM! Hook activation is correct.The before_install hook is properly configured to invoke the module creation during installation.
forms_pro/patches/0_1_beta/create_user_forms_module_def.py (1)
1-9: LGTM! Good code reuse pattern.The patch correctly delegates to the shared
create_user_forms_module()function, ensuring consistent behavior between fresh installations and migrations. The idempotency check in that function prevents duplicate module creation.forms_pro/install.py (1)
11-18: This concern is not applicable—the function is called fromafter_install, notbefore_install.The
before_installhook is commented out (hooks.py:92), andcreate_user_forms_module()is invoked from the activeafter_installhook (hooks.py:93, install.py:8). Duringafter_install, the app's schema is already synced and Module Def (a core Frappe doctype) is guaranteed to be available. No timing issue exists here.Likely an incorrect or invalid review comment.
| def before_install(): | ||
| create_user_forms_module() |
There was a problem hiding this comment.
Add error handling to prevent installation failures.
The before_install hook lacks error handling. If module creation fails (e.g., database connectivity issues, permission problems), it will break the entire installation process.
🛡️ Proposed fix with error handling and logging
def before_install():
- create_user_forms_module()
+ try:
+ create_user_forms_module()
+ except Exception as e:
+ frappe.log_error(
+ title="Failed to create User Forms module",
+ message=frappe.get_traceback()
+ )
+ # Re-raise to prevent installation if module creation is critical
+ raiseIf module creation is non-critical and installation should proceed regardless, change to:
def before_install():
- create_user_forms_module()
+ try:
+ create_user_forms_module()
+ except Exception:
+ frappe.log_error(
+ title="Failed to create User Forms module",
+ message=frappe.get_traceback()
+ )
+ # Continue installation even if module creation fails🤖 Prompt for AI Agents
In @forms_pro/install.py around lines 7 - 8, The before_install hook directly
calls create_user_forms_module without error handling; wrap the call in a
try/except inside before_install, catch broad exceptions from
create_user_forms_module, log the exception with context (e.g., using the
project logger or frappe.log/print) and allow installation to continue if this
module creation is non-critical (do not re-raise), or return a failure signal
only if you decide it must stop installation; refer to the before_install
function and create_user_forms_module by name when making the change.
| def execute(): | ||
| """ | ||
| Create a module def `User Forms` in the for the app `Forms Pro`. | ||
|
|
||
| """ | ||
| create_user_forms_module() |
There was a problem hiding this comment.
Add error handling for patch execution.
Similar to the before_install hook, the patch lacks error handling. If module creation fails during migration, it will break the bench migrate process for existing installations.
🛡️ Proposed fix with error handling
+import frappe
from forms_pro.install import create_user_forms_module
def execute():
"""
Create a module def `User Forms` for the app `Forms Pro`.
"""
- create_user_forms_module()
+ try:
+ create_user_forms_module()
+ except Exception as e:
+ frappe.log_error(
+ title="Patch failed: create_user_forms_module_def",
+ message=frappe.get_traceback()
+ )
+ # Re-raise to mark patch as failed
+ raiseCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @forms_pro/patches/0_1_beta/create_user_forms_module_def.py around lines 4 -
9, The patch's execute() calls create_user_forms_module() with no error
handling, which can abort bench migrate on failure; wrap the call in a
try/except around execute() (or inside it), catch Exception as e, record the
full exception (e and traceback) using frappe.log_error or processLogger (e.g.,
frappe.log_error(e, "create_user_forms_module patch failed")), and ensure the
exception is handled so migrations continue (do not re-raise); reference the
execute() function and create_user_forms_module() when making this change.
| """ | ||
| Create a module def `User Forms` in the for the app `Forms Pro`. | ||
|
|
||
| """ |
There was a problem hiding this comment.
Fix grammatical error in docstring.
Line 6 contains a typo: "in the for the app" should be "for the app".
📝 Proposed fix
def execute():
"""
- Create a module def `User Forms` in the for the app `Forms Pro`.
+ Create a module def `User Forms` for the app `Forms Pro`.
"""
create_user_forms_module()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """ | |
| Create a module def `User Forms` in the for the app `Forms Pro`. | |
| """ | |
| def execute(): | |
| """ | |
| Create a module def `User Forms` for the app `Forms Pro`. | |
| """ | |
| create_user_forms_module() |
🤖 Prompt for AI Agents
In @forms_pro/patches/0_1_beta/create_user_forms_module_def.py around lines 5 -
8, Fix the grammatical typo in the module docstring in
create_user_forms_module_def.py: replace the phrase "in the for the app `Forms
Pro`." with "for the app `Forms Pro`." so the docstring reads "Create a module
def `User Forms` for the app `Forms Pro`." and keep it as the top triple-quoted
string.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @forms_pro/utils/test_form_generator.py:
- Around line 306-307: Remove the @unittest.skip decorator from the skipped test
and update the test setup so it does not create core DocTypes under module="User
Forms" (use a test-specific module name like "Test - User Forms" or a randomized
module), and ensure both this test and test_status_field_is_added_core_doctype
create and teardown their DocTypes cleanly (delete created DocTypes in tearDown
or use context-managed creation) so they no longer conflict with core module
validation; run both tests together to confirm they pass.
- Around line 264-265: The tests are skipped because the "User Forms" module
isn't created in the test environment; update the before_tests() function in
install.py to call create_user_forms_module() before creating roles/users (i.e.,
ensure before_tests() invokes create_user_forms_module(), then
give_admin_forms_pro_role(), then create_test_user()) so DocType creations using
module="User Forms" succeed during tests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
forms_pro/utils/test_form_generator.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (1)
forms_pro/utils/test_form_generator.py (1)
4-4: LGTM: Import is necessary for skip decorators.The
unittestimport is required for the@unittest.skipdecorators used in the tests below.
| # TODO: Fix this test | ||
| @unittest.skip("Skipping this test as it is not working as expected") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's view the test file around lines 264-265
head -n 275 forms_pro/utils/test_form_generator.py | tail -n 20Repository: BuildWithHussain/forms_pro
Length of output: 1214
🏁 Script executed:
# Check the entire setUp methods to understand test initialization
rg -n 'def setUp|def setUpClass|def setUpModule' forms_pro/utils/test_form_generator.py -A 20Repository: BuildWithHussain/forms_pro
Length of output: 972
🏁 Script executed:
# Look for User Forms module creation in test setup
rg -n 'User Forms' forms_pro/utils/test_form_generator.py -B 3 -A 3Repository: BuildWithHussain/forms_pro
Length of output: 1427
🏁 Script executed:
# Check install.py for module creation logic
cat forms_pro/install.pyRepository: BuildWithHussain/forms_pro
Length of output: 1123
🏁 Script executed:
# Look for other test setup or fixtures that might create modules
fd -t f 'conftest\|fixtures\|setup' forms_pro/Repository: BuildWithHussain/forms_pro
Length of output: 52
Add create_user_forms_module() call to before_tests() to fix test failures.
The skipped tests fail because the "User Forms" module is not created in the test environment. The before_tests() hook in install.py only calls give_admin_forms_pro_role() and create_test_user() but does not call create_user_forms_module(). When the test tries to create a DocType with module = "User Forms", the module doesn't exist, causing the test to fail.
Update before_tests() to include:
def before_tests():
create_user_forms_module()
give_admin_forms_pro_role()
create_test_user()This ensures the module exists before tests run, allowing these tests to pass without being skipped.
🤖 Prompt for AI Agents
In @forms_pro/utils/test_form_generator.py around lines 264 - 265, The tests are
skipped because the "User Forms" module isn't created in the test environment;
update the before_tests() function in install.py to call
create_user_forms_module() before creating roles/users (i.e., ensure
before_tests() invokes create_user_forms_module(), then
give_admin_forms_pro_role(), then create_test_user()) so DocType creations using
module="User Forms" succeed during tests.
| # TODO: Fix this test | ||
| @unittest.skip("Skipping this test as it is not working as expected") |
There was a problem hiding this comment.
Same concern as the previous skipped test.
This test is skipped for the same reason as test_status_field_is_added_core_doctype above. Both tests create core DocTypes with module = "User Forms" and likely fail for the same reason.
Please address both skipped tests together as part of a single remediation effort.
🤖 Prompt for AI Agents
In @forms_pro/utils/test_form_generator.py around lines 306 - 307, Remove the
@unittest.skip decorator from the skipped test and update the test setup so it
does not create core DocTypes under module="User Forms" (use a test-specific
module name like "Test - User Forms" or a randomized module), and ensure both
this test and test_status_field_is_added_core_doctype create and teardown their
DocTypes cleanly (delete created DocTypes in tearDown or use context-managed
creation) so they no longer conflict with core module validation; run both tests
together to confirm they pass.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.