[ui] Highlight command status with colors #812 #1168
Conversation
Made command status easier to read by adding colors - green for success, red for failed, and gray for in-progress. Also made the text bold so it stands out more.
Srinath0916
force-pushed
the
issues/812-ui-command-highlight
branch
from
001fcc5 to
da537d5
Compare
|
Hi @pandafy, could you please review this, when u get time?? |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Sorry for my late response, we're being flooded with PRs.
It looks good, can you please add a basic test for this?
@coderabbitai review
|
|
||
| /* Command status highlighting */ | ||
| .command-status-success { | ||
| color: #417927; |
There was a problem hiding this comment.
I wonder if we have variables for this? We recently started using CSS variables more. It may be a good idea to define these colors in openwisp-utils.
There was a problem hiding this comment.
@nemesifier I'll keep the current approach for this PR. I haven't worked with openwisp-utils before, but I'd be happy to look into setting it up and creating CSS variables for these status colors as a follow-up if that would help with consistency across the project.
📝 WalkthroughWalkthroughIntroduces a Changes
Sequence Diagram(s)(omitted — change is a UI/admin presentation addition without multi-component sequential flow) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
openwisp_controller/connection/admin.py (1)
106-107: Consider adding ClassVar annotations for mutable class attributes.The field definitions are correct and properly updated to use
status_display. However, as flagged by static analysis, mutable class attributes in Django admin classes should be annotated withtyping.ClassVarfor better type safety and clarity.🔎 Proposed refactor with ClassVar annotations
Add the import at the top of the file:
+from typing import ClassVar from datetime import timedeltaThen apply these annotations:
class CommandInline(admin.StackedInline): model = Command verbose_name = _("Recent Commands") verbose_name_plural = verbose_name - fields = ["status_display", "type", "input_data", "output_data", "created", "modified"] - readonly_fields = ["status_display", "type", "input_data", "output_data", "created", "modified"] + fields: ClassVar = ["status_display", "type", "input_data", "output_data", "created", "modified"] + readonly_fields: ClassVar = ["status_display", "type", "input_data", "output_data", "created", "modified"] formset = LimitedCommandResultsopenwisp_controller/connection/static/connection/css/command-inline.css (1)
235-249: Consider using CSS variables for consistency if they exist in openwisp-utils.The implementation correctly applies the color coding specified in the PR objectives. While the project does use CSS variables from openwisp-utils elsewhere (e.g.,
--ow-color-fg-ghostin admin.css), the specific status colors (#417927, #ba2121, #666) do not appear to have CSS variable equivalents in the current setup. These colors are also used as hardcoded values elsewhere in the codebase (e.g., jsonschema-ui.css). If you plan to establish a consistent color palette for status indicators across the project, consider creating and using CSS variables for these colors; otherwise, the current implementation is acceptable and follows existing patterns.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/connection/admin.pyopenwisp_controller/connection/static/connection/css/command-inline.css
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_controller/connection/admin.py (2)
openwisp_controller/config/base/device.py (1)
status(398-403)openwisp_controller/connection/base/models.py (1)
input_data(638-642)
🪛 Ruff (0.14.10)
openwisp_controller/connection/admin.py
106-106: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
107-107: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (1)
openwisp_controller/connection/admin.py (1)
136-147: No action required.The implementation is correct. The Command model's status field choices ("in-progress", "success", "failed") match the CSS class names in command-inline.css exactly. The status field has a default value and is never null, so no None handling is needed. The use of
format_htmlproperly guards against XSS vulnerabilities.
- Add ClassVar annotations for fields and readonly_fields as suggested by reviewer - Fix line length issues by properly formatting field lists - Fix Black formatting for format_html call - Address all CI formatting failures
Srinath0916
force-pushed
the
issues/812-ui-command-highlight
branch
from
c5f5ee2 to
47ec5ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openwisp_controller/connection/admin.py (1)
151-160: Consider adding tests for the new status_display method.Per the PR checklist, tests have not been written. Consider adding unit tests to verify:
- Correct CSS class generation for each status value (success, failed, in-progress)
- Proper HTML output with format_html
- Correct display text via get_status_display()
Example test structure
# In tests for connection admin def test_command_inline_status_display_success(self): command = Command(status='success') inline = CommandInline(Command, admin_site) result = inline.status_display(command) assert 'command-status-success' in result assert 'Success' in result # or whatever get_status_display() returns def test_command_inline_status_display_failed(self): command = Command(status='failed') inline = CommandInline(Command, admin_site) result = inline.status_display(command) assert 'command-status-failed' in result def test_command_inline_status_display_in_progress(self): command = Command(status='in-progress') inline = CommandInline(Command, admin_site) result = inline.status_display(command) assert 'command-status-in-progress' in result
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/connection/admin.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_controller/connection/admin.py (2)
openwisp_controller/config/base/device.py (1)
status(398-403)openwisp_controller/connection/base/models.py (1)
input_data(638-642)
⏰ 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.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | 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.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (4)
openwisp_controller/connection/admin.py (4)
2-2: LGTM: Good type annotation practice.Adding
ClassVarimport to properly type-hint class-level attributes is a good practice that improves code clarity and IDE support.
107-122: LGTM: Field configuration is correct.The conversion to
ClassVartyping and replacement ofstatuswithstatus_displayaligns with the custom status rendering implementation. All fields are appropriately marked as readonly for this read-only inline.
160-160: LGTM: Proper label configuration.The short description is correctly set with translation support, consistent with other field labels in the inline.
151-156: No changes needed. The implementation correctly usesformat_htmlfor XSS safety andget_status_display()for human-readable text. Verification confirms:
- The CSS file (
command-inline.css) exists in the correct Django static directory and is automatically served by Django's static file system- All status values defined in
AbstractCommand.STATUS_CHOICES("in-progress", "success", "failed") have corresponding CSS classes (.command-status-in-progress,.command-status-success,.command-status-failed)The code is production-ready.
|
Hey, @nemesifier All feedback addressed and ready for merge! |
3 participants
Made command status easier to read by adding colors - green for success, red for failed, and gray for in-progress. Also made the text bold so it stands out more.
Checklist
Reference to Existing Issue
Closes #812.
Description of Changes
Added color highlighting to command status in the device admin page to improve readability. The status field now displays with color coding and bold text:
This makes it easier to quickly scan command results when u read.
Screenshot
(
)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.