Skip to content

Conversation

@piyushdev04
Copy link
Contributor

Modified JsonSchemaWidget media property to load advanced assets only when advanced_mode=True. CommandSchemaWidget now skips loading unnecessary JS/CSS. Verified config forms load assets, commands do not.

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #525.

Please open a new issue if there isn't an existing issue yet.

Description of Changes

  • Updated JsonSchemaWidget.media property to conditionally load advanced editor assets (advanced-mode.js, tomorrow_night_bright.js, advanced-mode.css) only when advanced_mode=True.
  • CommandSchemaWidget overrides advanced_mode=False, so these assets are not loaded for command forms, improving page performance.
  • Verified that configuration forms (templates, devices) still load the advanced editor as expected.
  • Verified that command forms (Connection → Commands) no longer load advanced editor assets.

@coderabbitai
Copy link

coderabbitai bot commented Jan 17, 2026

📝 Walkthrough

Walkthrough

The JsonSchemaWidget.media property in openwisp_controller/config/widgets.py was changed to exclude advanced editor assets by default. The base media now includes JS: jsonschema-ui.js, admin/jquery.init.js, widget.js, utils.js; and CSS: config/css/lib/jsonschema-ui.css. If advanced_mode is truthy, advanced-mode.js and tomorrow_night_bright.js are prepended to the JS list and config/css/lib/advanced-mode.css is appended to the CSS list. DeviceGroupJsonSchemaWidget still extends the base media and adds devicegroup.css. No public signatures changed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: avoiding loading the advanced editor for commands, which aligns with the primary objective of the PR.
Description check ✅ Passed The description covers the key changes, references issue #525, includes manual testing confirmation, but lacks new test cases and documentation updates as noted in unchecked boxes.
Linked Issues check ✅ Passed The PR successfully addresses issue #525 by conditionally loading advanced editor assets only when needed, preventing unnecessary asset loading for command forms while preserving functionality for configuration forms.
Out of Scope Changes check ✅ Passed All changes in JsonSchemaWidget and CommandSchemaWidget are directly scoped to the objective of avoiding advanced editor asset loading for commands as specified in issue #525.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb31b27 and 328f8b2.

📒 Files selected for processing (1)
  • openwisp_controller/config/widgets.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Applied to files:

  • openwisp_controller/config/widgets.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

  • openwisp_controller/config/widgets.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). (11)
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (2)
openwisp_controller/config/widgets.py (2)

28-32: LGTM! Clean implementation of conditional asset loading.

The insert order is correct—advanced-mode.js and tomorrow_night_bright.js are prepended before widget.js which depends on them, while advanced-mode.css is appended only when needed. This achieves the PR objective of reducing unnecessary asset loading for forms that don't require the advanced editor.


63-65: Correctly inherits conditional asset loading from parent.

With advanced_mode = False on line 60, the super().media call will return only the base assets without advanced editor JS/CSS, and then devicegroup.css is appended. The inheritance chain works as expected.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 17, 2026
Modified JsonSchemaWidget media property to load advanced assets
only when advanced_mode=True. CommandSchemaWidget now skips loading
unnecessary JS/CSS. Verified config forms load assets, commands do not.

Closes openwisp#525
@piyushdev04 piyushdev04 force-pushed the issues/525-disable-advanced-editor-for-commands branch from 2fa65d6 to 772f78a Compare January 17, 2026 18:44
@piyushdev04 piyushdev04 changed the title [change] Avoid loading advanced editor for commands #525 [admin] Avoid loading advanced editor for commands #525 Jan 17, 2026
Modified JsonSchemaWidget media property to load advanced assets
only when advanced_mode=True. CommandSchemaWidget now skips loading
unnecessary JS/CSS. Verified config forms load assets, commands do not.

Closes openwisp#525
@piyushdev04 piyushdev04 force-pushed the issues/525-disable-advanced-editor-for-commands branch from bb31b27 to 328f8b2 Compare January 17, 2026 19:15
@coveralls
Copy link

Coverage Status

coverage: 98.642% (+0.001%) from 98.641%
when pulling 328f8b2 on piyushdev04:issues/525-disable-advanced-editor-for-commands
into 6697a3a on openwisp:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

You're changing the class without changing anything else?

@piyushdev04
Copy link
Contributor Author

yes i changed the base class so asset loading depends on advanced_mode. since subclasses already set this flag, they automatically get the correct behavior without any other changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[change] Avoid loading advanced editor for commands

3 participants