[FIX] cetmix_tower_server: Server creation #183
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
329-331: Consider adding validation for option_id consistency.While the option handling is correct, consider validating that the option_id belongs to the variable to prevent potential mismatches.
variable_option_id = configuration_variable_options.get( variable_reference ) +if variable_option_id: + option = self.env['cx.tower.variable.option'].browse(variable_option_id) + if option.variable_id != variable.id: + raise ValidationError(_( + "Invalid option selected for variable %(var)s", + var=variable_reference + )) variable_vals_list.append( ( 0, 0, { "variable_id": variable.id, "value_char": variable_value, "option_id": variable_option_id, }, ) )Also applies to: 337-340
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/models/cx_tower_server_template.py(2 hunks)cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
cetmix_tower_server/models/cx_tower_server_template.py (2)
Learnt from: tendil
PR: cetmix/cetmix-tower#167
File: cetmix_tower_server/models/cx_tower_server_template.py:0-0
Timestamp: 2024-12-31T11:16:47.023Z
Learning: The `_validate_required_variables` method in `cx_tower_server_template.py` now preserves the original order of required variables in its error messages by using a list instead of sorting, ensuring that tests and user-facing messages align with expectations.
Learnt from: tendil
PR: cetmix/cetmix-tower#154
File: cetmix_tower_server/models/cx_tower_server_template.py:204-233
Timestamp: 2024-12-02T10:09:44.153Z
Learning: In `cetmix_tower_server/models/cx_tower_server_template.py`, avoid adding validation logic in data normalization methods like `_create_new_server`; validation for required variables should be performed at the model level or in business logic methods to prevent duplication and maintain architectural cleanliness.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (3)
cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py (1)
113-117: LGTM! Verify option mapping in integration tests.The addition of configuration_variable_options correctly maps variable references to their option IDs, fixing the server variable creation issue.
Run this script to verify the variable option mapping implementation:
✅ Verification successful
Implementation is well-tested ✓
The variable option mapping is verified through comprehensive test coverage:
- Variable-option integration in
test_variable_option.py- Server template variable handling in
test_server_template.py- Variable value operations in
test_variable.py🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for test coverage of the new configuration_variable_options mapping rg -l "configuration_variable_options.*test" | while read -r file; do echo "=== $file ===" rg -A 5 "configuration_variable_options" "$file" doneLength of output: 61
Script:
#!/bin/bash # Find all test files and their content related to server templates and variables echo "=== Finding test files ===" fd "test.*\.py$" echo -e "\n=== Searching for server template tests ===" rg -l "class.*Server.*Test|class.*Template.*Test" -t py echo -e "\n=== Checking test content for variable and option testing ===" rg "def test.*(?i)(variable|option|server|template)" -t py -A 5Length of output: 33173
cetmix_tower_server/models/cx_tower_server_template.py (2)
304-306: LGTM! Clean extraction of configuration options.The extraction of configuration_variable_options is properly placed before variable processing.
344-344: Verify the impact on existing variable values.The direct assignment to variable_value_ids replaces all existing values. Ensure this is the intended behavior.
✅ Verification successful
Direct assignment of variable values is the intended behavior
The codebase shows that variable values are designed to be fully manageable, with tests explicitly demonstrating removal and addition of values. The UI supports direct editing, and the One2many field type is consistently used across models for this purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any places in the codebase that expect variable values to be preserved rg -A 3 "variable_value_ids.*append"Length of output: 38
Script:
#!/bin/bash # Search for variable_value_ids modifications and context rg -B 2 -A 2 "variable_value_ids.*=" # Look for documentation about variable values fd README -e md -x cat {} # Check for write operations using AST ast-grep --pattern 'variable_value_ids = $_'Length of output: 10777
GabbasovDinar
force-pushed
the
14.0-t4253-cetmix_tower_server-fix-copy-from-template
branch
2 times, most recently
from
b880fd7 to
0bb04e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
356-377: Consider extracting variable update logic into a separate method.While the implementation is correct, extracting the variable update logic into a separate method would improve maintainability and readability.
def _prepare_server_values(self, with_template_variables=True, **kwargs): # ... existing code ... if configuration_variables: variable_vals_list = self._prepare_variable_values( configuration_variables, configuration_variable_options ) if with_template_variables: values["variable_value_ids"] = self._update_existing_variables( values.get("variable_value_ids", []), variable_vals_list ) else: values["variable_value_ids"] = variable_vals_list +def _prepare_variable_values(self, configuration_variables, configuration_variable_options): + variable_vals_list = [] + variable_obj = self.env["cx.tower.variable"] + + for variable_reference, variable_value in configuration_variables.items(): + variable = variable_obj.search([("reference", "=", variable_reference)]) + if not variable: + variable = variable_obj.create({"name": variable_reference}) + + variable_option_id = configuration_variable_options.get(variable_reference) + variable_vals_list.append( + (0, 0, { + "variable_id": variable.id, + "value_char": variable_value, + "option_id": variable_option_id, + }) + ) + return variable_vals_list + +def _update_existing_variables(self, existing_variable_values, new_variable_vals): + variable_id_to_index = { + cmd[2]["variable_id"]: idx + for idx, cmd in enumerate(existing_variable_values) + if cmd[0] == 0 and "variable_id" in cmd[2] + } + + for new_command in new_variable_vals: + variable_id = new_command[2]["variable_id"] + if variable_id in variable_id_to_index: + idx = variable_id_to_index[variable_id] + existing_variable_values[idx] = new_command + else: + existing_variable_values.append(new_command) + + return existing_variable_values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/models/cx_tower_server_template.py(6 hunks)cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (5)
cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py (2)
113-117: LGTM! Clean implementation of configuration variable options.The addition of
configuration_variable_optionsproperly maps variable references to their corresponding option IDs, aligning with the template class changes.
132-134: LGTM! Proper handling of template variables.Setting
with_template_variables=Falseensures that only wizard-provided variables are used, preventing potential variable duplication or conflicts.cetmix_tower_server/models/cx_tower_server_template.py (3)
174-177: LGTM! Well-documented parameter.The documentation for
with_template_variablesclearly explains its purpose, behavior, and default value.
262-262: LGTM! Consistent parameter implementation.The
with_template_variablesparameter is properly added to the method signature with consistent documentation.Also applies to: 271-274
316-318: LGTM! Well-structured variable options handling.The implementation properly extracts, looks up, and applies variable options during server creation.
Also applies to: 341-343, 349-351
GabbasovDinar
force-pushed
the
14.0-t4253-cetmix_tower_server-fix-copy-from-template
branch
from
0bb04e1 to
d2b957d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
314-346: Consider reordering validation checks for better UX.Currently, required variables are validated before checking if options exist. Consider reordering these checks to validate options first, providing a better user experience by showing the most relevant error message first.
if configuration_variables: - # Validate required variables - self._validate_required_variables(configuration_variables) - # Search for existing variable options. option_references = list(configuration_variable_options.values()) existing_options = option_references and self.env[ "cx.tower.variable.option" ].search([("reference", "in", option_references)]) missing_options = list( set(option_references) - {option.reference for option in existing_options} ) if missing_options: raise UserError( _( "Option with references '%(references)s' " "is not found for variables.", references=", ".join(missing_options), ) ) + # Validate required variables after options are validated + self._validate_required_variables(configuration_variables) + # Map variable options to their IDs. configuration_variable_options_dict = { option.variable_id.id: option.id for option in existing_options }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/models/cx_tower_server_template.py(6 hunks)cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (4)
cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py (2)
113-117: LGTM! Configuration variable options mapping added.The addition of
configuration_variable_optionscorrectly maps variable references to their option references, aligning with the template class changes.
132-134: LGTM! Server creation now correctly handles template variables.Setting
with_template_variables=Falseensures that only the variables from the wizard are used, preventing duplication with template variables.cetmix_tower_server/models/cx_tower_server_template.py (2)
174-177: LGTM! Well-documented parameter addition.The documentation for the
with_template_variablesparameter is clear, consistent, and explains the behavior well across all methods.Also applies to: 206-209, 271-274
388-409: LGTM! Robust handling of template variables.The conditional handling of variable values based on
with_template_variablesis well-implemented:
- When True: Updates or adds variable values from the template
- When False: Uses only the new variable values
GabbasovDinar
force-pushed
the
14.0-t4253-cetmix_tower_server-fix-copy-from-template
branch
4 times, most recently
from
d454653 to
3a0d0a3
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
316-341: Enhance error handling for variable options.The validation of variable options is thorough, but the error message could be more specific.
Consider this improvement:
- "Option with references '%(references)s' " - "is not found for variables.", + "Variable options with references '%(references)s' " + "were not found in the system. Please ensure all referenced options exist.",
🛑 Comments failed to post (1)
cetmix_tower_server/tests/test_server_template.py (1)
809-809:
⚠️ Potential issueFix code formatting issue.
There's a code formatting issue detected by ruff-format.
Add a newline at line 809 to fix the formatting issue.
🧰 Tools
🪛 GitHub Actions: pre-commit
[error] 809-809: Code formatting issue detected by ruff-format
GabbasovDinar
force-pushed
the
14.0-t4253-cetmix_tower_server-fix-copy-from-template
branch
from
3a0d0a3 to
d9ed258
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/models/cx_tower_server_template.py (1)
391-427: Consider adding error handling for duplicate variables.The variable values handling is well-structured, but there's no explicit handling for potential duplicate variables when merging template and configuration variables.
Consider adding validation to ensure no duplicate variable IDs exist when merging:
if with_template_variables: + # Check for duplicates in existing variables + existing_variable_ids = set() + for cmd in existing_variable_values: + if cmd[0] == 0 and "variable_id" in cmd[2]: + if cmd[2]["variable_id"] in existing_variable_ids: + raise ValidationError(_("Duplicate variable found in template")) + existing_variable_ids.add(cmd[2]["variable_id"]) + # update or add variable values existing_variable_values = values.get("variable_value_ids", [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cetmix_tower_server/models/cx_tower_server_template.py(6 hunks)cetmix_tower_server/readme/USAGE.md(3 hunks)cetmix_tower_server/tests/test_server_template.py(4 hunks)cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/readme/USAGE.md
- cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py
🔇 Additional comments (5)
cetmix_tower_server/models/cx_tower_server_template.py (4)
174-177: LGTM! Well-documented parameter.The docstring for
with_template_variablesis clear and comprehensive.
271-274: LGTM! Consistent documentation.The docstring for
with_template_variablesmatches the parent function's documentation.
314-318: LGTM! Clean configuration handling.The configuration variable options are properly extracted from kwargs.
324-346: LGTM! Robust validation.The validation of variable options is thorough with:
- Proper error handling for missing options
- Clear error messages
- Efficient mapping of options to their IDs
cetmix_tower_server/tests/test_server_template.py (1)
813-878: LGTM! Comprehensive test coverage.The test thoroughly validates the partial variable removal scenario:
- Creates variable option with test data
- Sets up template with multiple variables
- Simulates user interaction through wizard
- Verifies both count and content of variables
- Fix incorrect creation of server variables when generating a server from a template. - Enable the creation of servers from a template with either a full copy of variables or only the specified ones. - Allow the creation of servers with specific variable options. - Optimize search of existing variables and creation of new ones Task: 4253
GabbasovDinar
force-pushed
the
14.0-t4253-cetmix_tower_server-fix-copy-from-template
branch
from
d9ed258 to
5da7233
Compare
- Improve validation error message Task: 4253
GabbasovDinar
force-pushed
the
14.0-t4253-cetmix_tower_server-fix-copy-from-template
branch
from
5da7233 to
016312e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/tests/test_server_template.py (1)
864-866: Consider enhancing the test robustness.
- The magic number
0inremove(0)makes the test fragile. Consider finding the line by variable reference instead.- Add assertions to verify that the removed variable is actually gone.
- Consider adding negative test cases (e.g., removing required variables).
- wizard_form.line_ids.remove(0) + # Find and remove the line with variable_path + for index, line in enumerate(wizard_form.line_ids): + if line.variable_id == self.variable_path: + wizard_form.line_ids.remove(index) + break wizard_form.save() + # Verify that the removed variable is not present + self.assertFalse( + any(line.variable_id == self.variable_path for line in wizard.line_ids), + "Removed variable should not be present in the wizard", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cetmix_tower_server/models/cx_tower_server_template.py(5 hunks)cetmix_tower_server/readme/USAGE.md(3 hunks)cetmix_tower_server/tests/test_server_template.py(4 hunks)cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cetmix_tower_server/readme/USAGE.md
- cetmix_tower_server/models/cx_tower_server_template.py
- cetmix_tower_server/wizards/cx_tower_server_template_create_wizard.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (2)
cetmix_tower_server/tests/test_server_template.py (2)
Line range hint
156-178: LGTM! Good enhancement to the test.The addition of variable option testing improves the test coverage and makes the test more maintainable by using dynamic values instead of hardcoded strings.
813-878: LGTM! Well-structured test case.The test effectively verifies the partial variable removal functionality with clear setup, actions, and assertions. Good use of Form to simulate UI interactions.
| "Variable reference '%(var_ref)s' has an invalid " | ||
| "option reference '%(opt_ref)s'." | ||
| ) | ||
| % {"var_ref": var_ref, "opt_ref": opt_ref} |
There was a problem hiding this comment.
Please use correct method for string translations.
| @@ -251,7 +259,7 @@ def _get_fields_tower_server(self): | |||
| "server_log_ids", | |||
| ] | |||
|
|
|||
| def _prepare_server_values(self, **kwargs): | |||
| def _prepare_server_values(self, pick_all_template_variables=True, **kwargs): | |||
There was a problem hiding this comment.
with these changes, prepare_server_values is not a bit long, should we refactor to make it shorter and easier to understand?
| configuration_variables = kwargs.pop("configuration_variables", None) | ||
| line_ids_variables = kwargs.pop("line_ids_variables", None) | ||
| configuration_variable_options = kwargs.pop( | ||
| "configuration_variable_options", {} |
There was a problem hiding this comment.
I don't see this one added to the docstring similar to how configuration_variables is, is it missed?
Task: 4253
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation