[IMP] cetmix_tower_server: implement UI updates #185
Conversation
WalkthroughThe pull request introduces modifications to several files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
95-111: Enhance code quality with constants and improved documentation.Consider the following improvements:
- Define constants for action types and default messages
- Enhance docstring with parameter and return type documentation
+ # Constants + ACTION_FILE_TEMPLATE = "file_using_template" + ACTION_PLAN = "plan" + DEFAULT_TEMPLATE_MSG = "No template code available" + DEFAULT_PLAN_MSG = "No related lines available" + DEFAULT_PREVIEW_MSG = "No preview available" @api.depends("command_id", "action") def _compute_command_code(self): """ Compute the preview of the command based on its action. + + Computes a preview text based on the command's action type: + - For file_using_template: Shows the template code + - For plan: Shows a list of flight plan line names + - For other actions: Shows a default message + + Dependencies: + - command_id: The related command record + - action: The type of action to be performed """ for line in self: - if line.action == "file_using_template": + if line.action == ACTION_FILE_TEMPLATE: line.command_code = ( - line.command_id.code or "No template code available" + line.command_id.code or DEFAULT_TEMPLATE_MSG ) - elif line.action == "plan": + elif line.action == ACTION_PLAN: flight_plan_lines = line.command_id.flight_plan_id.line_ids preview_lines = [line.name for line in flight_plan_lines] - line.command_code = "\n".join(preview_lines) if preview_lines else "No related lines available" + line.command_code = "\n".join(preview_lines) if preview_lines else DEFAULT_PLAN_MSG else: - line.command_code = "No preview available" + line.command_code = DEFAULT_PREVIEW_MSG🧰 Tools
🪛 Ruff (0.8.2)
108-108: Line too long (111 > 88)
(E501)
🪛 GitHub Actions: pre-commit
[error] 108-108: Line too long (111 > 88 characters)
cetmix_tower_server/views/cx_tower_server_template_view.xml (1)
59-63: Consider UX enhancements for the server count display.The implementation follows Odoo patterns and provides clear visibility of the count. Consider these UX improvements:
<div attrs="{'invisible': [('server_count', '=', False)]}"> <strong>Servers:</strong> - <span class="badge badge-primary"> + <span class="badge badge-primary" + t-att-title="'Number of servers using this template'"> <t t-esc="record.server_count.value" /> </span> </div>🧰 Tools
🪛 GitHub Actions: pre-commit
[warning] File was modified by prettier formatting hook
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cetmix_tower_server/models/cx_tower_command_log.py(1 hunks)cetmix_tower_server/models/cx_tower_plan_line.py(2 hunks)cetmix_tower_server/views/cx_tower_command_log_view.xml(2 hunks)cetmix_tower_server/views/cx_tower_command_view.xml(1 hunks)cetmix_tower_server/views/cx_tower_plan_log_view.xml(1 hunks)cetmix_tower_server/views/cx_tower_server_template_view.xml(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: pre-commit
cetmix_tower_server/views/cx_tower_server_template_view.xml
[warning] File was modified by prettier formatting hook
cetmix_tower_server/models/cx_tower_plan_line.py
[error] 108-108: Line too long (111 > 88 characters)
🪛 Ruff (0.8.2)
cetmix_tower_server/models/cx_tower_plan_line.py
108-108: Line too long (111 > 88)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (7)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
47-51: LGTM! Field declaration follows Odoo conventions.The change from a related field to a computed field is well-structured and aligns with the requirement to show dynamic content based on the action type.
cetmix_tower_server/models/cx_tower_command_log.py (1)
63-83: LGTM! Well-structured related fields implementation.The new related fields for plan_log_id tracking are well-implemented with:
- Proper field types matching the related model
- Appropriate store=True for query optimization
- Clear string labels for UI display
cetmix_tower_server/views/cx_tower_plan_log_view.xml (1)
84-84: LGTM! Sorting implementation matches requirements.The default ordering by start_date desc, id desc aligns with the PR objective to sort Flight Plan Logs by "Started" date in descending order.
cetmix_tower_server/views/cx_tower_command_log_view.xml (2)
77-83: LGTM! Well-structured form view group for plan log tracking.The new group of fields provides clear visibility of related Flight Plan Log details, matching the PR objective.
109-109: LGTM! Consistent sorting implementation.The default ordering matches the Flight Plan Log view, maintaining consistency across the application.
cetmix_tower_server/views/cx_tower_command_view.xml (1)
253-257: LGTM! Search filter enhances command filtering capabilities.The new filter for Run Flight Plan commands improves the search functionality by allowing specific filtering of flight plan related commands.
cetmix_tower_server/views/cx_tower_server_template_view.xml (1)
14-14: LGTM! Verify the computed field implementation.The field declaration follows Odoo conventions and is properly positioned with related fields.
Run this script to verify the computed field implementation:
✅ Verification successful
✓ Computed field implementation verified and correct
The server_count field is properly implemented with a clean One2many relationship and follows Odoo best practices. The computation is lightweight and suitable for real-time display in views.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify server_count computed field implementation # Expected: Find the compute method definition and dependencies # Search for server_count compute method ast-grep --pattern 'server_count = fields.$_($$$, compute="$_")' # Search for potential dependencies in compute method rg -A 10 "def _compute_server_count"Length of output: 1293
Script:
#!/bin/bash # Verify server_ids field definition and related performance settings ast-grep --pattern 'server_ids = fields.$_($$$)' # Check for potential performance impact by finding server count usage rg "server_count" -A 2Length of output: 5385
🧰 Tools
🪛 GitHub Actions: pre-commit
[warning] File was modified by prettier formatting hook
This commit implements the following UI updates:
1. Sort Flight Plan Log and Command Log by "Started" (newest first).
- Added `default_order="start_date desc, id desc"` in tree views for both logs.
2. Show related Flight Plan Log line in the Command Log form.
- Added related fields in the Command Log model and updated the form view.
3. Update the "Code" field in the Flight Plan Line form.
- Implemented a computed field `command_code` to display:
- File template code for the action "Upload File from Template".
- Related flight plan lines for the action "Execute Flight Plan".
4. Display the number of servers in the Server Template Kanban view.
- Used the `server_count` computed field to show the count as a numeric value in Kanban cards.
5. Add a pre-defined "Flight Plan" filter to the Command search view.
- Added a filter for the `action='plan'` in the search view of the Command model.
Task ID: 4263
tendil
force-pushed
the
14.0-t4263-cetmix_tower_server-ui-updates
branch
from
280b587 to
54fcb67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
102-103: Consider optimizing memory usage for large flight plans.For large flight plans, building the preview list could be memory intensive. Consider limiting the number of lines shown.
- flight_plan_lines = line.command_id.flight_plan_id.line_ids - preview_lines = [line.name for line in flight_plan_lines] + # Limit preview to first 10 lines for better performance + flight_plan_lines = line.command_id.flight_plan_id.line_ids[:10] + preview_lines = [line.name for line in flight_plan_lines] + if len(line.command_id.flight_plan_id.line_ids) > 10: + preview_lines.append("... and more lines")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cetmix_tower_server/models/cx_tower_command_log.py(1 hunks)cetmix_tower_server/models/cx_tower_plan_line.py(2 hunks)cetmix_tower_server/views/cx_tower_command_log_view.xml(2 hunks)cetmix_tower_server/views/cx_tower_command_view.xml(1 hunks)cetmix_tower_server/views/cx_tower_plan_log_view.xml(1 hunks)cetmix_tower_server/views/cx_tower_server_template_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cetmix_tower_server/views/cx_tower_command_view.xml
- cetmix_tower_server/views/cx_tower_plan_log_view.xml
- cetmix_tower_server/views/cx_tower_command_log_view.xml
- cetmix_tower_server/models/cx_tower_command_log.py
- cetmix_tower_server/views/cx_tower_server_template_view.xml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (1)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
47-49: LGTM! Field declaration follows best practices.The change from a related field to a computed field is well-structured and follows Odoo field declaration conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_plan_line.py(2 hunks)cetmix_tower_server/views/cx_tower_command_log_view.xml(2 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server/views/cx_tower_command_log_view.xml
🔇 Additional comments (3)
cetmix_tower_server/views/cx_tower_plan_line_view.xml (1)
42-50: LGTM! Well-structured visibility rules.The visibility rules for
command_codeandplan_line_idsfields are correctly implemented based on theactionfield value, following the feedback from the previous review.cetmix_tower_server/models/cx_tower_plan_line.py (2)
110-120: LGTM! Well-implemented plan lines computation.The
_compute_plan_line_idsmethod correctly:
- Handles the 'plan' action case
- Includes proper null check for flight_plan
- Sets False for non-plan actions
47-55: LGTM! Field definitions are clear and well-structured.The computed fields are properly defined with correct parameters and string labels.
tendil
force-pushed
the
14.0-t4263-cetmix_tower_server-ui-updates
branch
from
5111416 to
8fec17f
Compare
Task ID: 4263
tendil
force-pushed
the
14.0-t4263-cetmix_tower_server-ui-updates
branch
from
8fec17f to
76d5c23
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
99-111: Simplify the command code computation logic.The condition
elif line.action != "plan"is redundant since we already handle the 'plan' action in the previous condition.@api.depends("command_id", "action") def _compute_command_code(self): """ Compute the preview of the command based on its action. """ for line in self: if line.action == "file_using_template": line.command_code = line.command_id.code or "No template code available" elif line.action == "plan": line.command_code = False - elif line.action != "plan": + else: line.command_code = "No preview available"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_plan_line.py(2 hunks)cetmix_tower_server/views/cx_tower_command_log_view.xml(2 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/views/cx_tower_plan_line_view.xml
- cetmix_tower_server/views/cx_tower_command_log_view.xml
🔇 Additional comments (2)
cetmix_tower_server/models/cx_tower_plan_line.py (2)
47-55: LGTM! Field declarations are well-structured.The computed fields
command_codeandplan_line_idsare properly defined with appropriate field types and compute methods.
112-123: 🛠️ Refactor suggestionAdd validation for command_id in plan line computation.
The method could raise an AttributeError if
command_idis False. Add validation to handle this case gracefully.@api.depends("command_id", "action") def _compute_plan_line_ids(self): """ Compute the related plan lines if the action is "plan". """ for line in self: - if line.action == "plan": + if not line.command_id: + line.plan_line_ids = False + elif line.action == "plan": flight_plan = line.command_id.flight_plan_id line.plan_line_ids = flight_plan.line_ids if flight_plan else [] else: line.plan_line_ids = FalseLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cetmix_tower_server/views/cx_tower_plan_line_view.xml (2)
41-41: Consider adding options and domain attributes to the action field.The
actionfield controls the visibility of other fields but lacks attributes to restrict its possible values. Consider adding:
widget="selection"for better UXoptions="{'no_create': True, 'no_create_edit': True}"to prevent invalid values- A domain to restrict values to the expected set:
['ssh_command', 'python_code', 'plan']- <field name="action" /> + <field + name="action" + widget="selection" + options="{'no_create': True, 'no_create_edit': True}" + />
42-49: Improve field visibility conditions and grouping.The visibility conditions for
command_codeandplan_line_idsuse different operators ('not in' vs '!=') for what appears to be mutually exclusive conditions. Consider:
- Using consistent operators for better maintainability
- Grouping related fields within a sub-group for better organization
- <field - name="command_code" - attrs="{'invisible': [('action', 'not in', ['ssh_command', 'python_code'])]}" - /> - <field - name="plan_line_ids" - attrs="{'invisible': [('action', '!=', 'plan')]}" - /> + <group> + <field + name="command_code" + attrs="{'invisible': [('action', 'not in', ['ssh_command', 'python_code'])], + 'required': [('action', 'in', ['ssh_command', 'python_code'])]}" + /> + <field + name="plan_line_ids" + attrs="{'invisible': [('action', 'not in', ['plan'])], + 'required': [('action', 'in', ['plan'])]}" + /> + </group>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_plan_line.py(1 hunks)cetmix_tower_server/views/cx_tower_command_log_view.xml(2 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/views/cx_tower_command_log_view.xml
- cetmix_tower_server/models/cx_tower_plan_line.py
🔇 Additional comments (1)
cetmix_tower_server/views/cx_tower_plan_line_view.xml (1)
41-49: Verify the action field values and field dependencies.The view changes suggest that
actioncan have values like 'ssh_command', 'python_code', and 'plan'. Let's verify:
- All possible values of the
actionfield- That these fields are properly defined in the model
- The relationships between these fields
✅ Verification successful
Field values and dependencies are correctly implemented
The action field values ('ssh_command', 'python_code', 'plan') and their relationships with command_code and plan_line_ids fields are properly defined and extensively tested in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify action field values and related field definitions # Check the model definition for action field and its values rg -A 10 "action = fields\." cetmix_tower_server/models/ # Check for any references to these specific action values rg -e "ssh_command|python_code|plan" cetmix_tower_server/ # Check the model definitions for command_code and plan_line_ids rg "command_code = fields\." cetmix_tower_server/models/ rg "plan_line_ids = fields\." cetmix_tower_server/models/Length of output: 94595
| ) | ||
| plan_line_ids = fields.One2many( | ||
| comodel_name="cx.tower.plan.line", | ||
| compute="_compute_plan_line_ids", | ||
| inverse_name="command_id", |
There was a problem hiding this comment.
This will allow to modify those lines directly which is not what we want.
There was a problem hiding this comment.
After analyzing and checking, I found that this field is redundant within the scope of this task. If this is not the case, please let me know
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cetmix_tower_server/models/cx_tower_file_template.py (1)
26-26: Consider grouping fields together for better code organization.The
activefield should be grouped with other fields for better readability and maintenance.Move the field definition to be grouped with other fields:
- active = fields.Boolean(default=True) file_name = fields.Char( help="Default full file name with file type for example: test.txt", ) + active = fields.Boolean(default=True)cetmix_tower_server/views/cx_tower_file_template_view.xml (1)
132-136: Consider enhancing the archive filter functionality.While the implementation is correct, consider adding a default filter for active records and grouping them together.
Add default active filter:
+ <filter + string="Active" + name="active" + domain="[('active', '=', True)]" + /> <filter string="Archived" name="archived" domain="[('active', '=', False)]" /> + <separator/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cetmix_tower_server/models/cx_tower_file_template.py(1 hunks)cetmix_tower_server/models/cx_tower_plan_line.py(1 hunks)cetmix_tower_server/views/cx_tower_file_template_view.xml(3 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/views/cx_tower_plan_line_view.xml
- cetmix_tower_server/models/cx_tower_plan_line.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (3)
cetmix_tower_server/models/cx_tower_file_template.py (1)
26-26: Verify inheritance and data migration needs.Please verify:
- That none of the inherited mixins (
cx.tower.reference.mixin,cx.tower.key.mixin,cx.tower.template.mixin) already provide anactivefield to avoid conflicts.- Whether existing records need a data migration script to set the
activefield.✅ Verification successful
No conflicts or migration needed for the
activefield
- None of the inherited mixins (
cx.tower.reference.mixin,cx.tower.key.mixin,cx.tower.template.mixin) contain anactivefield, so there are no inheritance conflicts.- The
default=Truevalue ensures both new and existing records will work as expected without requiring data migration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if active field exists in inherited mixins echo "Checking for active field in inherited mixins..." rg -A 5 "class.*Mixin.*:" | rg -A 5 "active.*=.*fields\." # Check for existing records that might need migration echo "Checking if there are existing records..." ast-grep --pattern 'self.env["cx.tower.file.template"].search'Length of output: 324
Script:
#!/bin/bash # Search for specific mixin classes and their fields echo "Searching for specific mixin classes..." rg -A 10 "class.*(ReferenceMixin|KeyMixin|TemplateMixin).*:" # Look for model usage patterns echo -e "\nChecking for model usage patterns..." rg -l "cx.tower.file.template" # Check for data files and initialization echo -e "\nChecking for data files..." fd -g "data/*.xml"Length of output: 4273
cetmix_tower_server/views/cx_tower_file_template_view.xml (2)
9-23: LGTM! Archive/Unarchive buttons implementation follows Odoo standards.The implementation correctly uses:
toggle_activemethod- Proper visibility attributes
- Standard button styling
46-46: LGTM! Invisible active field placement is correct.The
activefield is correctly placed in the title div and set as invisible, following Odoo standards.
| @@ -44,7 +44,9 @@ class CxTowerPlanLine(models.Model): | |||
| help="Actions trigger based on command result." | |||
| " If empty next command will be executed", | |||
| ) | |||
| command_code = fields.Text(related="command_id.code", readonly=True) | |||
| command_code = fields.Text( | |||
| comodel_name="cx.tower.command", string="Code", readonly=True | |||
There was a problem hiding this comment.
What is the reason behind removing the "related" key?
and ref naming Task: 4055
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cetmix_tower_server/models/cx_tower_plan_line.py (3)
60-65: Remove redundant comodel_name attribute.The
comodel_nameattribute is unnecessary for related fields as they inherit the model from their related path.command_code = fields.Text( related="command_id.code", - comodel_name="cx.tower.command", string="Code", readonly=True, )
88-108: Consider refactoring compute methods to reduce duplication.Both compute methods share similar logic patterns. Consider extracting the common logic into a helper method.
+ def _get_related_flight_plan(self): + """Helper method to get related flight plan if action is 'plan'.""" + return ( + self.command_id.flight_plan_id + if self.action == "plan" and self.command_id.flight_plan_id + else False + ) + @api.depends("command_id", "command_id.flight_plan_id", "action") def _compute_related_plan_line_ids(self): """Compute the related plan lines if the action is "plan".""" for line in self: - if line.action == "plan" and line.command_id.flight_plan_id: - line.related_plan_line_ids = line.command_id.flight_plan_id.line_ids - else: - line.related_plan_line_ids = False + flight_plan = line._get_related_flight_plan() + line.related_plan_line_ids = flight_plan and flight_plan.line_ids or False @api.depends("command_id", "action") def _compute_related_flight_plan_id(self): """Compute related Flight Plan ID if the command action is "plan".""" for line in self: - if line.action == "plan" and line.command_id.flight_plan_id: - line.related_flight_plan_id = line.command_id.flight_plan_id - else: - line.related_flight_plan_id = False + line.related_flight_plan_id = line._get_related_flight_plan()
90-92: Fix docstring line length to comply with PEP 8.The docstrings exceed the recommended line length. Let's make them more concise.
- """ - Compute the related plan lines if the action is "plan". - """ + """Compute the related plan lines if the action is "plan".""" - """ - Compute related Flight Plan ID if the command action is "plan". - """ + """Compute related Flight Plan ID if the command action is "plan"."""Also applies to: 101-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/models/cx_tower_plan_line.py(3 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server/views/cx_tower_plan_line_view.xml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (1)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
28-40: LGTM! Well-structured field declarations.The field definitions follow best practices with appropriate storage strategies and access controls.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
95-105: Consider adding error handling in compute methods.The compute methods could benefit from additional error handling to gracefully handle edge cases.
@api.depends("command_id", "command_id.flight_plan_id", "action") def _compute_related_plan_line_ids(self): """ Compute the related plan lines if the action is "plan". """ for line in self: + try: if line.action == "plan" and line.command_id.flight_plan_id: line.related_plan_line_ids = line.command_id.flight_plan_id.line_ids else: line.related_plan_line_ids = False + except Exception as e: + _logger.error("Error computing related plan lines: %s", str(e)) + line.related_plan_line_ids = FalseAlso applies to: 106-116, 117-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/models/cx_tower_plan_line.py(3 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (5)
cetmix_tower_server/views/cx_tower_plan_line_view.xml (3)
26-29: LGTM! Visibility conditions foruse_sudoandpathfields.The visibility conditions ensure these fields are only shown when
actionisssh_command, which aligns with their purpose.Also applies to: 30-33
45-49: LGTM! Field visibility logic.The visibility condition for
command_codeis correctly set to show only whenactionis eitherssh_commandorpython_code.
51-58: LGTM! Related fields visibility.The visibility conditions for related fields are correctly implemented:
related_file_template_idis shown only whenactionisfile_using_template.related_flight_plan_idandrelated_plan_line_idsare shown only whenactionisplan.Also applies to: 59-67
cetmix_tower_server/models/cx_tower_plan_line.py (2)
28-33: LGTM! Field definitions.The new fields are correctly defined with appropriate attributes:
- All fields are marked as
readonly=Trueto prevent direct modifications.- Compute methods are specified for each field.
- Appropriate
comodel_namevalues are set.Also applies to: 34-40, 87-93
60-65: LGTM! Updated command_code field.The field definition is enhanced with
comodel_nameand a clearer string label.
| flight_plan_id = fields.Many2one( | ||
| comodel_name="cx.tower.plan", | ||
| related="command_id.flight_plan_id", | ||
| store=True, |
There was a problem hiding this comment.
Why do we need to store it? Are we searching by this field?
There was a problem hiding this comment.
I'd leave it in this field we leave it as it can be used in filters/search
There was a problem hiding this comment.
This field is show in the pop-up form only, so it cannot be searched.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cetmix_tower_server/views/cx_tower_plan_line_view.xml (1)
51-55: Remove redundant string attribute.The visibility condition is correct, but the
stringattribute on thefile_template_idfield is redundant as it will be automatically generated from the field name.- <field name="file_template_id" string="File Template" /> + <field name="file_template_id" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_plan_line.py(3 hunks)cetmix_tower_server/views/cx_tower_file_template_view.xml(2 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server/views/cx_tower_file_template_view.xml
🔇 Additional comments (6)
cetmix_tower_server/models/cx_tower_plan_line.py (3)
28-33: LGTM! Well-structured field definition.The
plan_line_idsfield is correctly defined as a readonly One2many field, maintaining data integrity while providing access to related plan lines.
34-39: LGTM! Field definition aligns with requirements.The
flight_plan_idfield is correctly defined withstore=Trueto support filtering and search functionality, as previously discussed.
85-89: LGTM! Well-structured field definition.The
file_template_idfield is correctly defined as a readonly Many2one field, maintaining data integrity while providing access to the related file template.cetmix_tower_server/views/cx_tower_plan_line_view.xml (3)
26-29: LGTM! Appropriate visibility rules.The visibility conditions for
use_sudoandpathfields are correctly set to show only whenaction='ssh_command', which aligns with their intended usage.Also applies to: 30-34
45-49: LGTM! Well-structured preview section.The visibility rule for
command_codeis correctly implemented to show only for code-related actions (ssh_commandandpython_code).
56-86: LGTM! Well-structured flight plan section.The flight plan section is well implemented with:
- Appropriate visibility conditions
- Well-organized tree view with optional columns
- Proper security group restrictions on sensitive fields
tendil
force-pushed
the
14.0-t4263-cetmix_tower_server-ui-updates
branch
from
5f7e94e to
4105ded
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_plan_line.py (1)
28-33: Remove explicit string parameter.The string parameter can be removed as it will be automatically generated from the field name in a human-readable format.
plan_line_ids = fields.One2many( comodel_name="cx.tower.plan.line", related="plan_id.line_ids", - string="Flight Plan Lines", readonly=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_plan_line.py(3 hunks)cetmix_tower_server/views/cx_tower_file_template_view.xml(2 hunks)cetmix_tower_server/views/cx_tower_plan_line_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/views/cx_tower_file_template_view.xml
- cetmix_tower_server/views/cx_tower_plan_line_view.xml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (3)
cetmix_tower_server/models/cx_tower_plan_line.py (3)
59-62: LGTM!The field definition is correct and follows best practices.
84-88: LGTM!The field definition is correct and follows best practices.
34-39: Remove unnecessary store parameter.Based on past discussions, storing this related field might not be necessary unless it's used in filters/search views.
Let's verify if this field is used in any search views:
✅ Verification successful
Confirmed: Remove unnecessary store parameter
The
flight_plan_idfield is only used for display purposes in form views and is not utilized in any search views or filters. Since it's a related field and there's no need for searching or filtering on this field, thestore=Trueparameter can be safely removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if flight_plan_id is used in search views or filters rg -l "flight_plan_id" --type xmlLength of output: 238
Script:
#!/bin/bash # Check for search views and filters using flight_plan_id rg "flight_plan_id" --type xml -C 5 cetmix_tower_server/views/cx_tower_plan_line_view.xml rg "<search.*?>.*?flight_plan_id.*?</search>" --type xml -A 10 -B 10 rg "<filter.*?flight_plan_id.*?>" --type xml -C 5Length of output: 932
| flight_plan_id = fields.Many2one( | ||
| comodel_name="cx.tower.plan", | ||
| related="command_id.flight_plan_id", | ||
| store=True, |
There was a problem hiding this comment.
This field is show in the pop-up form only, so it cannot be searched.
| @@ -56,7 +57,10 @@ | |||
| <div class="oe_kanban_global_click"> | |||
| <div class="col-12 pt8 o_kanban_primary_right"> | |||
| <div> | |||
| <field name="reference" /> | |||
| <strong>Servers:</strong> | |||
| <span class="badge badge-primary"> | |||
There was a problem hiding this comment.
No need to use badge here, because it's less readable and the entire design of this section is different.
| @@ -25,6 +25,18 @@ class CxTowerPlanLine(models.Model): | |||
| auto_join=True, | |||
| ondelete="cascade", | |||
| ) | |||
| plan_line_ids = fields.One2many( | |||
| comodel_name="cx.tower.plan.line", | |||
| related="plan_id.line_ids", | |||
There was a problem hiding this comment.
Why??? Why are you showing the lines of the flight plan that this line belongs to??
| attrs="{'invisible': [('action', 'not in', ['ssh_command', 'python_code'])]}" | ||
| /> | ||
| </group> | ||
| <group |
There was a problem hiding this comment.
Please use 'invisible' attribute for fields, not for groups. Use the same group for all the fields.
Move the code, plan lines and template preview out of the group, so it consumes all the form width.
P.S. template preview is still missing.
There was a problem hiding this comment.
P.S. template preview is still missing.
I'm not sure what you're asking. Do I need to add another `file_template_preview' field?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/views/cx_tower_plan_line_view.xml (1)
65-66: Clarify Template Preview Requirement:
Previous review feedback noted that a template preview might be missing. Could you please confirm if a dedicatedfile_template_previewfield is required to display a preview of the file template? If so, consider adding it to the view to enhance user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cetmix_tower_server/views/cx_tower_plan_line_view.xml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (8)
cetmix_tower_server/views/cx_tower_plan_line_view.xml (8)
26-29: "use_sudo" Field Visibility:
The conditional attributeattrs="{'invisible': [('action', '!=', 'ssh_command')]}"ensures that the "use_sudo" field is only visible when theactionis'ssh_command'. This fulfills the intended UI behavior.
29-33: "path" Field Visibility and Placeholder:
The "path" field not only provides a clear placeholder to guide the user but also uses an analogous conditional attribute to hide the field when theactionis not'ssh_command'. Ensure the placeholder text is sufficiently clear for end users.
45-45: Introduction of "action" Field:
The newly added "action" field drives the conditional display logic for several subsequent fields (e.g., "command_code", "file_template_id", "flight_plan_id", and "plan_line_ids"). Please verify that the underlying model has the corresponding field definition (and computed logic or domain if applicable) to support this UI behavior.
47-50: "command_code" Field Conditional Display:
The conditionattrs="{'invisible': [('action', 'not in', ['ssh_command', 'python_code'])]}"effectively restricts the visibility of "command_code" to cases whereactionis either'ssh_command'or'python_code'. Confirm that these allowed values match the business and UX requirements.
51-55: Addition of "file_template_id" Field:
The new "file_template_id" field will only be visible whenactionequals'file_using_template', aligning with the UI update objectives. Make sure that the related model field is defined properly and integrates seamlessly with this view logic.
56-60: Conditional Display for "flight_plan_id":
Setting the "flight_plan_id" field to be visible only whenactionequals'plan'is consistent with the intended functionality. It is recommended to double-check that the backend logic and relationship in the model support this condition correctly.
61-65: "plan_line_ids" Field with Nested Tree View:
The configuration of "plan_line_ids", complete with its nested tree view, is well-structured and uses the intended conditional visibility (i.e., visible only whenactionequals'plan'). Please verify that the one2many relationship in the model corresponds with this UI representation and that any ordering or domain constraints (if needed) are applied at the model level.
66-90: Nested Tree View for "plan_line_ids":
The tree view defined within the "plan_line_ids" field provides a clear layout by listing details such as sequence, name, action, and conditional fields. The use of widgets (e.g.,widget="handle"andwidget="many2many_tags") and group restrictions (viagroups="cetmix_tower_server.group_manager") appears well thought out. Please ensure that access control and visibility on these fields are tested as per the update requirements.
This commit implements the following UI updates:
Sort Flight Plan Log and Command Log by "Started" (newest first).
default_order="start_date desc, id desc"in tree views for both logs.Show related Flight Plan Log line in the Command Log form.
Update the "Code" field in the Flight Plan Line form.
command_codeto display:Display the number of servers in the Server Template Kanban view.
server_countcomputed field to show the count as a numeric value in Kanban cards.Add a pre-defined "Flight Plan" filter to the Command search view.
action='plan'in the search view of the Command model.Task ID: 4263
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI Changes