[IMP][REF][ADD] cetmix_tower_server: Refactor Access Rules and Add Te… #187
Conversation
WalkthroughThis pull request introduces updates to the Cetmix Tower Server Management module, focusing on access control, variable management, and documentation refinement. Key changes include the addition of access level controls for variables and variable values, updates to security rules, modifications to views, and alignment of documentation with the new version (14.0). These modifications enhance the granularity of access management and improve the user interface for variable configuration. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
✨ Finishing Touches
🪧 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 (23)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (3)
9-12: Consider using integer comparison for access_level.The domain condition uses string comparison
('access_level', '=', '1')for access levels. Since access levels are hierarchical, using integer comparison would be more appropriate and safer.- ('access_level', '=', '1'), + ('access_level', '=', 1),
22-27: Use integer comparison for manager access_level.Similar to the user rule, the manager access rule uses string comparison. Convert to integer for consistency and proper hierarchical comparison.
- ('access_level', '<=', '2'), + ('access_level', '<=', 2),
Line range hint
39-45: Improve domain readability with proper formatting.The domain conditions in action rules have complex nested relationships. Consider improving readability by properly formatting the domain lists.
Example format for better readability:
<field name="domain_force"> [ ('plan_line_action_id.access_level', '=', 1), '|', ('plan_line_action_id.line_id.plan_id.server_ids', '=', False), ( 'plan_line_action_id.line_id.plan_id.server_ids.message_partner_ids', 'in', [user.partner_id.id] ) ] </field>Also applies to: 61-67, 83-89
cetmix_tower_server/static/description/index.html (11)
485-487: Improve formatting of the documentation.The line breaks in this section make the text harder to read in the source. Consider keeping related content on the same line.
-They can create new <a class="reference external" href="#configure-a-server">Servers</a> too however they -cannot delete them. Users of this group have access to the entities -with <tt class="docutils literal">Access Level</tt> set to <tt class="docutils literal">Manager</tt> or <tt class="docutils literal">User</tt>.</li> +They can create new <a class="reference external" href="#configure-a-server">Servers</a> too however they cannot delete them. Users of this group have access to the entities with <tt class="docutils literal">Access Level</tt> set to <tt class="docutils literal">Manager</tt> or <tt class="docutils literal">User</tt>.</li>
551-552: Improve formatting of the documentation.Similar to the previous comment, the line breaks here make the text harder to read in the source.
-<li><strong>Files</strong>: Shows all <a class="reference external" href="#configure-a-file">Files</a> that belong to this -server</li> +<li><strong>Files</strong>: Shows all <a class="reference external" href="#configure-a-file">Files</a> that belong to this server</li>
563-564: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-<li><strong>Flight Plan</strong>: Select a flight plan to be executed after a server is -created</li> +<li><strong>Flight Plan</strong>: Select a flight plan to be executed after a server is created</li>
602-604: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-<li><strong>Reference</strong>: Unique identifier used to address the tag in conditions -and expressions. Leave this field blank to generate it automatically -based on the name</li> +<li><strong>Reference</strong>: Unique identifier used to address the tag in conditions and expressions. Leave this field blank to generate it automatically based on the name</li>
694-695: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-<li>Local values. Those are values that are defined at a record level. For -example for a server or an action.</li> +<li>Local values. Those are values that are defined at a record level. For example for a server or an action.</li>
1074-1075: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-<li>Commands run with <tt class="docutils literal">sudo</tt> with password are be split and executed one -by one anyway.</li> +<li>Commands run with <tt class="docutils literal">sudo</tt> with password are be split and executed one by one anyway.</li>
1195-1200: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-<li><strong>Show shared</strong>: By default only commands available for the selected -server(s) are selectable. Activate this checkbox to select any -command</li> -<li><strong>Path</strong>: Directory where command will be executed. Important: this -field does not support variables! Ensure that user has access to -this location even if you run command using sudo.</li> +<li><strong>Show shared</strong>: By default only commands available for the selected server(s) are selectable. Activate this checkbox to select any command</li> +<li><strong>Path</strong>: Directory where command will be executed. Important: this field does not support variables! Ensure that user has access to this location even if you run command using sudo.</li>
1204-1205: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-rendered. However during the command execution command code will be -rendered for each server separately.</li> +rendered. However during the command execution command code will be rendered for each server separately.</li>
1214-1216: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-command log in a new wizard window. <strong>IMPORTANT:</strong> Button will be show -only if single server is selected. If you try to run a command for -several servers from code, you will get a ValidationError.</li> +command log in a new wizard window. <strong>IMPORTANT:</strong> Button will be show only if single server is selected. If you try to run a command for several servers from code, you will get a ValidationError.</li>
1236-1237: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-<li><strong>Show shared</strong>: By default only flight plans available for the -selected server(s) are selectable. Activate this checkbox to select -any flight plan</li> +<li><strong>Show shared</strong>: By default only flight plans available for the selected server(s) are selectable. Activate this checkbox to select any flight plan</li>
1260-1263: Improve formatting of the documentation.Consider keeping related content on the same line for better readability in the source.
-<li>Click the <tt class="docutils literal">Refresh</tt> button to update the log. You can also click the -<tt class="docutils literal">Refresh All</tt> button <strong>(3)</strong> located above the log list in order to -refresh all logs at once. Log output will be displayed in the HTML -field below.</li> +<li>Click the <tt class="docutils literal">Refresh</tt> button to update the log. You can also click the <tt class="docutils literal">Refresh All</tt> button <strong>(3)</strong> located above the log list in order to refresh all logs at once. Log output will be displayed in the HTML field below.</li>cetmix_tower_server/tests/test_variable.py (3)
729-790: Use constants or enums for access levels to improve readability and maintainability.Currently, access levels are hardcoded as strings like
"1","2", and"3". Defining constants or an enumeration for access levels would enhance code clarity and reduce the likelihood of typos.For example, you could define access level constants:
ACCESS_LEVEL_USER = "1" ACCESS_LEVEL_MANAGER = "2" ACCESS_LEVEL_ROOT = "3"Then use these constants throughout your tests:
-variable_private = self.Variable.create( - {"name": "Private Variable", "access_level": "1"} -) +variable_private = self.Variable.create( + {"name": "Private Variable", "access_level": ACCESS_LEVEL_USER} +)
948-979: Ensure variable rendering is tested thoroughly with assertions.In the
test_variable_rendering_in_executionmethod, while variables are emulated for execution, explicit assertions verifying the rendered output would strengthen the test's effectiveness. This ensures that all variables, regardless of access level, are correctly rendered during execution.Consider adding assertions like:
self.assertEqual( used_variables["User Variable"], "User Value", "User variable must be used during execution", )
1089-1100: Utilize a templating engine for variable rendering instead of manual replacements.Manually replacing variables using
str.replace()can be error-prone and does not scale well. Leveraging the existing templating engine (e.g., Jinja2) would make the code cleaner and more maintainable.Refactor the rendering logic:
-rendered_file_name = file_as_user.name.replace( - "{{ version2 }}", "Manager Value" -) +rendered_file_name = self.env['ir.qweb']._render_raw( + file_as_user.name, {'version2': 'Manager Value'} +)cetmix_tower_server/models/cx_tower_variable_value.py (2)
14-15: Consider optimizing the access level computation.The implementation correctly inherits access levels from variables, but could be optimized for bulk operations.
Consider this optimization:
@api.depends("variable_id") def _compute_access_level(self): - for rec in self: - if rec.variable_id: - rec.access_level = rec.variable_id.access_level + for rec in self.filtered('variable_id'): + rec.access_level = rec.variable_id.access_levelAlso applies to: 148-156
193-221: Simplify access level consistency check.The constraint logic is correct, but the implementation could be simplified for better maintainability.
Consider this simplified version:
@api.constrains("access_level", "variable_id") def _check_access_level_consistency(self): for rec in self: - if rec.variable_id and rec.access_level < rec.variable_id.access_level: - raise ValidationError( - _( - "The access level for Variable Value '%(value)s' cannot be" - "lower than the access level of its Variable '%(variable)s'.\n" - "Variable Access Level: %(var_level)s\n" - "Variable Value Access Level: %(val_level)s", - value=rec.value_char or "Undefined", - variable=rec.variable_id.name, - var_level=dict( - rec.fields_get(["access_level"])["access_level"][ - "selection" - ] - )[rec.variable_id.access_level], - val_level=dict( - rec.fields_get(["access_level"])["access_level"][ - "selection" - ] - )[rec.access_level], - ) - ) + if not rec.variable_id: + continue + if rec.access_level < rec.variable_id.access_level: + selection_dict = dict(rec.fields_get(["access_level"])["access_level"]["selection"]) + raise ValidationError( + _( + "The access level for Variable Value '%(value)s' cannot be " + "lower than the access level of its Variable '%(variable)s'.\n" + "Variable Access Level: %(var_level)s\n" + "Variable Value Access Level: %(val_level)s", + value=rec.value_char or "Undefined", + variable=rec.variable_id.name, + var_level=selection_dict[rec.variable_id.access_level], + val_level=selection_dict[rec.access_level], + ) + )cetmix_tower_server/security/cx_tower_variable_security.xml (1)
21-26: Simplify root access rule domain.The root access rule can be simplified by omitting the domain_force field entirely since the default behavior grants full access.
Consider this simplification:
<record id="cx_tower_variable_rule_group_root_access" model="ir.rule"> <field name="name">Tower Variable: Root Access Rule</field> <field name="model_id" ref="model_cx_tower_variable" /> - <field name="domain_force">[(1, '=', 1)]</field> <field name="groups" eval="[(4, ref('cetmix_tower_server.group_root'))]" /> </record>cetmix_tower_server/views/cx_tower_variable_value_view.xml (1)
Line range hint
4-27: Add access_level field to tree view.The access_level field is available in the search view but not in the tree view. Consider adding it for better visibility and management.
Add the field to the tree view:
<record id="cx_tower_variable_value_view_tree" model="ir.ui.view"> <field name="name">cx.tower.variable.value.view.tree</field> <field name="model">cx.tower.variable.value</field> <field name="arch" type="xml"> <tree editable="top" decoration-bf="is_global==True"> <field name="reference" optional="hidden" /> <field name="variable_id" /> + <field name="access_level" optional="show" /> <field name="is_global" widget="boolean_toggle" />cetmix_tower_server/views/cx_tower_variable_view.xml (1)
Line range hint
77-85: Add access_level field to tree view.For consistency with the form and search views, consider adding the access_level field to the tree view.
Add the field to the tree view:
<record id="cx_tower_variable_view_tree" model="ir.ui.view"> <field name="name">cx.tower.variable.view.tree</field> <field name="model">cx.tower.variable</field> <field name="arch" type="xml"> <tree> <field name="name" /> <field name="reference" optional="show" /> + <field name="access_level" optional="show" /> <field name="value_ids_count" /> </tree> </field> </record>cetmix_tower_server/views/cx_tower_server_view.xml (1)
286-286: Enhance UX for access_level field.The access_level field has been added to both tree and form views, but could benefit from additional attributes for better user experience.
Consider adding these improvements:
- <field name="access_level" /> + <field name="access_level" + widget="selection" + options="{'no_create': True}" + placeholder="Select Access Level" />This change would:
- Use a selection widget for better visual representation
- Prevent accidental creation of new values
- Add a helpful placeholder text
Also applies to: 303-305
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(1 hunks)cetmix_tower_server/models/cx_tower_variable.py(1 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(3 hunks)cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(6 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_variable.py(4 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)cetmix_tower_server/views/cx_tower_variable_value_view.xml(4 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cetmix_tower_server/README.rst
🧰 Additional context used
📓 Learnings (4)
cetmix_tower_server/models/cx_tower_variable.py (2)
Learnt from: tendil
PR: cetmix/cetmix-tower#160
File: cetmix_tower_server/models/cx_tower_command.py:125-132
Timestamp: 2024-12-30T10:57:47.019Z
Learning: When inheriting `cx.tower.template.mixin`, there is no need to separately inherit `cx.tower.variable.mixin` because the former already handles the variable logic.
Learnt from: ivs-cetmix
PR: cetmix/cetmix-tower#129
File: cetmix_tower_server/models/cx_tower_plan_line.py:13-15
Timestamp: 2024-11-12T14:48:17.141Z
Learning: Adding `cx.tower.reference.mixin` to the class `CxTowerPlanLine` in `cetmix_tower_server/models/cx_tower_plan_line.py` does not require updating the `_compute_variable_ids` method.
cetmix_tower_server/views/cx_tower_variable_view.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:18-34
Timestamp: 2024-11-25T17:05:10.576Z
Learning: In the `cetmix_tower_server` module, the `access_level` field is a custom field defined in the `CxTowerAccessMixin` class in `cetmix_tower_access_mixin.py`. It is used for access control in security rules and is not related to the standard permission fields like `perm_read`, `perm_write`, etc.
cetmix_tower_server/security/cx_tower_variable_security.xml (2)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_plan_line_security.xml:38-50
Timestamp: 2024-11-20T22:04:20.044Z
Learning: In `cetmix_tower_server/security/*.xml` files, when defining access control rules for Odoo models, if the intention is to grant full permissions (read, create, write, unlink) within the domain specified, it is acceptable to omit the `perm_*` fields and rely on default permission values.
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (11)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (2)
Line range hint
98-134: Well-structured server template rules.The implementation correctly follows Odoo's security model where group rules unify. The separation of access and deletion rules with proper permission flags is well done.
135-141: Root access rule properly implemented.The root access implementation follows the standard pattern with unrestricted domain access.
cetmix_tower_server/static/description/index.html (3)
372-372: LGTM! Updated GitHub repository links.The links to the GitHub repository have been updated from the development branch (
14.0-dev) to the stable branch (14.0), which is appropriate for a production release.
413-413: LGTM! Updated external link.The link to the Python requests library documentation has been properly formatted and updated.
472-472: LGTM! Updated image source.The image source for the user profile screenshot has been updated to point to the stable branch (
14.0).cetmix_tower_server/tests/test_variable.py (1)
728-1116: Comprehensive test coverage for variable access and rendering.The added test methods effectively verify access control logic across different user groups and ensure that variable rendering functions correctly, enhancing the reliability of the system.
cetmix_tower_server/models/cx_tower_variable.py (2)
9-9: Confirm that the new mixin does not introduce conflicts or redundant functionality.By adding
cx.tower.access.mixinto_inherit, ensure that it does not conflict with existing inherited methods or introduce redundant features already provided by other mixins, as indicated in previous learnings.Reference the retrieved learning:
When inheriting
cx.tower.template.mixin, there is no need to separately inheritcx.tower.variable.mixinbecause the former already handles the variable logic.While this case involves
cx.tower.access.mixin, double-checking for overlapping functionality would be prudent.
9-9: LGTM!The inclusion of
cx.tower.access.mixinappropriately extends access control capabilities to theTowerVariablemodel.cetmix_tower_server/__manifest__.py (2)
26-26: Ensure the security file is loaded in the correct sequence.Adding
"security/cx_tower_variable_security.xml"is correct, but verify that it's placed appropriately in the list to respect dependencies and loading order of security rules.If the new security rules depend on groups or models defined earlier, the placement is fine. Otherwise, consider reordering.
26-26: LGTM!The addition of the new security file to the manifest aligns with the changes made and integrates seamlessly.
cetmix_tower_server/views/cx_tower_server_view.xml (1)
269-269: Verify security implications of granting user group access to configuration page.Adding
group_userto the configuration page's access groups significantly broadens access to server configuration settings. Please confirm if this is intentional and aligns with security requirements.Run this script to analyze the access rules and ensure proper restrictions are in place:
✅ Verification successful
Configuration page access for users is properly restricted
The change to grant
group_useraccess to the configuration page is safe because:
- Users already have read-only access to server records
- Record-level rules restrict users to only see authorized servers
- Write operations remain restricted to managers
- Similar access patterns exist in other views
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify access rules for configuration settings # Check security rules defined for cx.tower.server model rg -A 5 "cx.tower.server" security/ # Check other views that might be affected by this change rg -A 5 "cetmix_tower_server.group_user" views/Length of output: 180
Script:
#!/bin/bash # Find security and view files echo "=== Finding security files ===" fd -e xml "security" --full-path fd "ir.model.access.csv" --full-path echo -e "\n=== Finding view files ===" fd -e xml "view" --full-path echo -e "\n=== Searching for group definitions ===" rg -l "group_(manager|user)" --type xmlLength of output: 4035
Script:
#!/bin/bash echo "=== Group Definitions ===" cat cetmix_tower_server/security/cetmix_tower_server_groups.xml echo -e "\n=== Server Model Security Rules ===" cat cetmix_tower_server/security/cx_tower_server_security.xml echo -e "\n=== Access Rights ===" rg "cx.tower.server" cetmix_tower_server/security/ir.model.access.csv echo -e "\n=== Similar Group Usage in Views ===" rg -A 2 "groups.*cetmix_tower_server.group_(manager|user)" cetmix_tower_server/views/Length of output: 11378
| @@ -266,7 +266,7 @@ | |||
| <page | |||
| name="configuration" | |||
| string="Configuration" | |||
| groups="cetmix_tower_server.group_manager" | |||
| groups="cetmix_tower_server.group_manager, cetmix_tower_server.group_user" | |||
There was a problem hiding this comment.
Just remove the "groups" key.
|
Supersedes #166 |
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature-v2
branch
from
de1104d to
dec2672
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: 13
🧹 Nitpick comments (23)
cetmix_tower_git/views/cx_tower_server_view.xml (1)
18-25: Consider enhancing form view organization.While the form view is functional, consider organizing related fields into logical groups for better UX. For example:
<form> <group> - <field name="server_id" invisible="1" /> - <field name="git_project_id" /> - <field name="file_id" /> - <field name="project_format" /> + <group string="Project"> + <field name="server_id" invisible="1" /> + <field name="git_project_id" /> + </group> + <group string="Configuration"> + <field name="file_id" /> + <field name="project_format" /> + </group> </group> </form>cetmix_tower_server/tests/test_cetmix_tower.py (1)
Line range hint
1-124: Consider adding docstrings and type hints for better maintainability.While the test implementation is progressing well, consider these additional improvements:
- Add detailed docstrings to test methods describing test scenarios and parameters
- Add type hints to improve code maintainability
- Consider using pytest parametrize for testing different combinations of attempts and wait times
Example docstring format:
def test_server_check_ssh_connection(self) -> None: """Test SSH connection scenarios. Test cases: 1. Successful connection with default parameters 2. Connection timeout after maximum attempts 3. Invalid server reference Args: None Returns: None """cetmix_tower_server/tests/common.py (1)
221-223: Enhance SSH client mock with timeout validation.The change from returning
TruetoMagicMock()improves the test setup by providing a more realistic SSH client mock. However, consider validating the timeout parameter since it's now part of the method signature.def _get_ssh_client_patch(self, raise_on_error=True, timeout=5000): """Mock method for connection""" + if timeout <= 0: + raise ValueError("Timeout must be positive") return MagicMock()cetmix_tower_git/views/cx_tower_git_project_views.xml (3)
22-128: Add chatter integration for change tracking.Consider adding
<div class="oe_chatter">section after the</sheet>tag to enable message threading and tracking changes.</sheet> + <div class="oe_chatter"> + <field name="message_follower_ids" widget="mail_followers"/> + <field name="message_ids" widget="mail_thread"/> + </div> </form>
35-39: Improve reference field's placeholder text.The placeholder text is too long and should be translatable. Consider:
- Breaking it into shorter, clearer text
- Adding translation support
- placeholder="Reference. Can contain English letters, digits and '_'. Leave blank to autogenerate" + placeholder="Enter reference or leave blank to autogenerate"
86-91: Fix XML indentation for better readability.The indentation in the Files page is inconsistent. Some elements are over-indented while others are under-indented.
<page name="files" string="Files"> - <group> - <field - name="git_aggregator_root_dir" - placeholder="Git aggregator root directory where sources will be cloned. Leave blank to use '.'" - /> - </group> - <field name="git_project_rel_ids"> + <group> + <field + name="git_aggregator_root_dir" + placeholder="Git aggregator root directory where sources will be cloned. Leave blank to use '.'" + /> + </group> + <field name="git_project_rel_ids">Also applies to: 92-111
cetmix_tower_yaml/tests/test_tower_yaml_mixin.py (1)
15-32: Enhance test coverage with additional test cases.The test method could be more comprehensive. Consider adding test cases for:
- Empty dictionary
- Nested dictionaries
- Dictionary with special characters
- Dictionary with multi-line strings
Example implementation:
def test_convert_dict_to_yaml(self): # Test regular flow self.assertEqual( self.YamlMixin._convert_dict_to_yaml({"a": 1, "b": 2}), "a: 1\nb: 2\n", "Dictionary was not converted to YAML correctly", ) # Test empty dictionary self.assertEqual( self.YamlMixin._convert_dict_to_yaml({}), "{}\n", "Empty dictionary was not converted to YAML correctly", ) # Test nested dictionary self.assertEqual( self.YamlMixin._convert_dict_to_yaml({"a": {"b": 1}}), "a:\n b: 1\n", "Nested dictionary was not converted to YAML correctly", ) # Test multi-line string self.assertEqual( self.YamlMixin._convert_dict_to_yaml({"a": "line1\nline2"}), "a: |\n line1\n line2\n", "Multi-line string was not converted to YAML correctly", ) # Test exception due to wrong values with self.assertRaises(ValidationError) as e: self.YamlMixin._convert_dict_to_yaml("not_a_dict") self.assertEqual( str(e.exception), _("Values must be a dictionary"), "Exception message doesn't match", )cetmix_tower_server/views/cx_tower_key_view.xml (1)
13-13: LGTM! Consider adding copy options for consistency.The addition of the
CopyClipboardCharwidget improves UX by making it easier to copy the reference value. However, for consistency with thereference_codefield below it, consider adding the same copy options.- <field name="reference" widget="CopyClipboardChar" /> + <field name="reference" widget="CopyClipboardChar" options="{'string': 'Copy'}" />cetmix_tower_git/models/cx_tower_git_project.py (1)
114-122: Update regex pattern to capture all valid environment variable names.In the
_extract_variables_from_textmethod, the current regexr"\$([A-Z0-9_]+)"only matches variables containing uppercase letters and underscores. Environment variable names can include lowercase letters. Consider updating the regex to include lowercase letters.Apply this diff to improve variable extraction:
- variables = re.findall(r"\$([A-Z0-9_]+)", text) + variables = re.findall(r"\$([A-Za-z0-9_]+)", text)cetmix_tower_git/models/cx_tower_git_remote.py (1)
155-209: Consider using a specialized library for URL parsingCurrently,
_get_repo_protocol_and_provider_from_urluses manual string manipulation to parse the repository URL. This approach may not handle all edge cases and could lead to errors with unconventional URLs. It's recommended to use a dedicated library likegiturlparseor similar to reliably parse Git URLs, enhancing maintainability and robustness.cetmix_tower_server/models/cx_tower_server.py (1)
493-494: Simplify condition in_is_being_deletedmethodThe condition
self.status and self.status == "deleting"can be simplified toself.status == "deleting". Since comparingNoneto a string returnsFalse, the extra check is unnecessary.Apply this diff to simplify the condition:
def _is_being_deleted(self): self.ensure_one() - return self.status and self.status == "deleting" + return self.status == "deleting"cetmix_tower_git/models/cx_tower_file.py (2)
23-33: Consider optimizing the compute method.The current implementation could be more concise using field access shorthand.
@api.depends("git_project_ids") def _compute_git_project_id(self): - """ - Link to project using the proxy model. - """ for record in self: - # File is related to project via proxy model. - # So there can be only one record in o2m field. - record.git_project_id = ( - record.git_project_ids and record.git_project_ids[0].id - ) + record.git_project_id = record.git_project_ids[:1].id🧰 Tools
🪛 GitHub Actions: tests
[warning] method cx.tower.file._check_git_project_id: @constrains parameter 'git_project_id' is not writeable
35-50: Enhance error message formatting and docstring.The constraint method could benefit from improved documentation and error message formatting.
@api.constrains("git_project_ids") def _check_git_project_id(self): """ - Check if file is related to a single project only. + Ensure file is related to a single project only. + + :raises: ValidationError if file is related to multiple projects """ for record in self: if len(record.git_project_ids) > 1: raise ValidationError( _( - "File '%(file)s' is related to multiple projects:" - " %(projects)s \n" - "Please select only one project.", + "File '%(file)s' is related to multiple projects:\n" + "%(projects)s\n\n" + "Please select only one project.", file=record.name, projects=", ".join(record.git_project_ids.mapped("name")), ) )🧰 Tools
🪛 GitHub Actions: tests
[warning] method cx.tower.file._check_git_project_id: @constrains parameter 'git_project_id' is not writeable
cetmix_tower_git/tests/common.py (2)
11-14: Consider using shorter aliases for frequently used models.For better readability in test methods, consider using shorter aliases for frequently accessed models.
- self.GitProject = self.env["cx.tower.git.project"] - self.GitProjectRel = self.env["cx.tower.git.project.rel"] - self.GitSource = self.env["cx.tower.git.source"] - self.GitRemote = self.env["cx.tower.git.remote"] + self.Project = self.env["cx.tower.git.project"] + self.ProjectRel = self.env["cx.tower.git.project.rel"] + self.Source = self.env["cx.tower.git.source"] + self.Remote = self.env["cx.tower.git.remote"]
29-71: Consider extracting remote creation into helper methods.The remote creation logic is repetitive and could be extracted into helper methods for better maintainability.
def _create_remote(self, url, source, head_type="branch", head="main", sequence=0): return self.Remote.create({ "url": url, "source_id": source.id, "head_type": head_type, "head": head, "sequence": sequence, })Then use it like:
self.remote_github_https = self._create_remote( "https://github.com/cetmix/cetmix-tower.git", self.git_source_1, head_type="pr", head="https://github.com/cetmix/cetmix-tower/pull/123", sequence=1 )cetmix_tower_git/tests/test_file_rel.py (2)
18-67: Consider breaking down the test into smaller, focused test methods.The
test_file_rel_createmethod tests multiple scenarios. For better clarity and maintenance, consider splitting it into separate test methods.def test_file_content_updates_on_creation(self): """Test if file content is updated after creation""" yaml_code = self.file_1_rel._generate_code_git_aggregator(self.file_1_rel) self.assertEqual(self.server_1_file_1.code, yaml_code) self.assertIn(self.remote_other_ssh.url, self.server_1_file_1.code) def test_file_content_updates_on_remote_modification(self): """Test if file content is updated after remote modification""" initial_code = self.server_1_file_1.code self.remote_other_ssh.url = "https://github.com/cetmix/cetmix-memes.git" self.assertNotEqual(self.server_1_file_1.code, initial_code) self.assertIn("https://github.com/cetmix/cetmix-memes.git", self.server_1_file_1.code) def test_file_content_updates_on_source_deactivation(self): """Test if file content is updated after source deactivation""" self.git_source_2.active = False self.assertNotIn("https://github.com/cetmix/cetmix-memes.git", self.server_1_file_1.code)
74-111: Consider using a fixture for expected YAML content.The expected YAML content could be moved to a fixture file for better maintainability and readability.
Create a new file
tests/fixtures/expected_yaml.py:EXPECTED_GIT_AGGREGATOR_YAML = """# This file is generated with Cetmix Tower https://cetmix.com/tower # It's designed to be used with git-aggregator tool developed by Acsone. # Documentation for git-aggregator: https://github.com/acsone/git-aggregator ./git_project_1_git_source_1: remotes: remote_1: https://github.com/cetmix/cetmix-tower.git ... """Then import and use it in the test:
from .fixtures.expected_yaml import EXPECTED_GIT_AGGREGATOR_YAML def test_format_git_aggregator(self): """Test if format git aggregator works correctly""" yaml_code = self.file_1_rel._generate_code_git_aggregator(self.file_1_rel) self.assertEqual(yaml_code, EXPECTED_GIT_AGGREGATOR_YAML)cetmix_tower_git/tests/test_project.py (1)
29-100: Consider breaking down the test into smaller, focused test methods.The
test_project_accessmethod tests multiple scenarios. For better clarity and maintenance, consider splitting it into separate test methods with more descriptive names.def test_user_without_group_cannot_access_project(self): """Test that user without any group cannot access project resources""" with self.assertRaises(AccessError): self.project_as_bob.read([]) with self.assertRaises(AccessError): self.source_as_bob.read([]) with self.assertRaises(AccessError): self.remote_as_bob.read([]) def test_manager_can_access_project(self): """Test that manager can access project resources""" self.add_to_group(self.user_bob, "cetmix_tower_server.group_manager") self._assert_bob_can_read_all() def test_server_link_restricts_access(self): """Test that linking server restricts access for non-subscribers""" self.add_to_group(self.user_bob, "cetmix_tower_server.group_manager") self._create_project_server_link() with self.assertRaises(AccessError): self.project_as_bob.read([]) def test_server_subscription_grants_access(self): """Test that server subscription grants access to linked projects""" self.add_to_group(self.user_bob, "cetmix_tower_server.group_manager") self._create_project_server_link() self.server_test_1.message_subscribe([self.user_bob.partner_id.id]) self._assert_bob_can_read_all() def _assert_bob_can_read_all(self): """Helper method to assert Bob can read all resources""" res = self.project_as_bob.read([]) self.assertEqual(res[0]["name"], self.git_project_1.name) res = self.source_as_bob.read([]) self.assertEqual(res[0]["name"], self.git_source_1.name) res = self.remote_as_bob.read([]) self.assertEqual(res[0]["name"], self.remote_other_ssh.name) def _create_project_server_link(self): """Helper method to create project-server link""" return self.GitProjectRel.create({ "server_id": self.server_test_1.id, "file_id": self.server_1_file_1.id, "git_project_id": self.git_project_1.id, "project_format": "git_aggregator", })cetmix_tower_git/security/cx_tower_git_remote_security.xml (1)
4-12: Consider indexing the relational path for better performance.The domain traverses multiple relations (
source_id.git_project_id.server_ids). Consider adding indexes to improve query performance.cetmix_tower_git/views/cx_tower_git_remote_views.xml (1)
48-50: Consider using generic examples in the URL placeholder.Instead of specific repository names, use generic examples to avoid confusion if the repositories change or move:
- placeholder="Git remote URL. Eg 'https://github.com/cetmix/cetmix-tower.git' or '[email protected]:cetmix/cetmix-tower.git'" + placeholder="Git remote URL. Eg 'https://github.com/organization/repository.git' or '[email protected]:organization/repository.git'"cetmix_tower_git/views/cx_tower_git_source_views.xml (1)
48-58: Enhance the warning message about remote ordering.The current message could be clearer about the implications of remote ordering:
- The top one remote will be used as a merge target. - You can re-arrange remotes by dragging them or changing their sequence value. + Important: The first remote (lowest sequence) will be used as the merge target. + You can re-arrange remotes by: + - Dragging them up/down + - Changing their sequence valuecetmix_tower_git/demo/demo_data.xml (1)
93-94: Consider using a placeholder for the commit hash.The hard-coded commit hash
1234567890might become invalid. Consider using a more obvious placeholder value or documenting that this is a demo value.cetmix_tower_git/readme/CONFIGURE.md (1)
1-1: Format the URL as a Markdown link.Convert the bare URL to a proper Markdown link:
-For information on how to configure Git Projects, please refer to the official documentation: https://cetmix.com/tower +For information on how to configure Git Projects, please refer to the [official documentation](https://cetmix.com/tower).🧰 Tools
🪛 Markdownlint (0.37.0)
1-1: null
Bare URL used(MD034, no-bare-urls)
🛑 Comments failed to post (13)
cetmix_tower_git/readme/USAGE.md (1)
1-1:
⚠️ Potential issueDocumentation should be managed in a separate repository.
According to previous feedback from ivs-cetmix, documentation for Cetmix Tower modules is managed in a separate repository. Documentation changes should not be made in the main repository.
Please remove this file and submit documentation updates to the dedicated documentation repository instead.
🧰 Tools
🪛 Markdownlint (0.37.0)
1-1: null
Bare URL used(MD034, no-bare-urls)
cetmix_tower_server/static/description/index.html (2)
371-372:
⚠️ Potential issueSecurity: Update HTTP URLs to HTTPS.
The following URLs should be updated to use HTTPS for improved security:
http://www.gnu.org/licenses/agpl-3.0-standalone.htmlhttp://cetmix.com/towerWould you like me to help create an issue to track updating these URLs in the source files?
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 371-371: "This link points to a plaintext HTTP URL. Prefer an encrypted HTTPS URL if possible."
Context:
Note: [CWE-319] Authentication Bypass by Primary Weakness [REFERENCES]
- https://cwe.mitre.org/data/definitions/319.html(plaintext-http-link-html)
[warning] 372-372: "This link points to a plaintext HTTP URL. Prefer an encrypted HTTPS URL if possible."
Context: Cetmix Tower
Note: [CWE-319] Authentication Bypass by Primary Weakness [REFERENCES]
- https://cwe.mitre.org/data/definitions/319.html(plaintext-http-link-html)
1127-1128:
⚠️ Potential issueFix broken documentation links.
The links to
CONFIGURE.mdare broken as they point to a non-existent file. The links should point to the corresponding sections within the same document.Apply this diff to fix the broken links:
-<li>Go to the <tt class="docutils literal">Cetmix Tower/Servers/Templates</tt> menu and select a <a class="reference external" href="CONFIGURE.md/#configure-a-server-template">Server -Template</a></li> +<li>Go to the <tt class="docutils literal">Cetmix Tower/Servers/Templates</tt> menu and select a <a class="reference external" href="#configure-a-server-template">Server Template</a></li> -<li>If a <a class="reference external" href="CONFIGURE.md/#configure-a-flight-plan">Flight Plan</a> is -defined in the server template it will be automatically executed after -a new server is created</li> +<li>If a <a class="reference external" href="#configure-a-flight-plan">Flight Plan</a> is defined in the server template it will be automatically executed after a new server is created</li>Also applies to: 1135-1136
cetmix_tower_server/tests/test_cetmix_tower.py (2)
108-124: 🛠️ Refactor suggestion
Enhance test coverage and improve mock implementation.
The current implementation has limited test coverage and the mock function could be better structured. Consider these improvements:
- Move the mock function to a class-level or module-level fixture:
@classmethod def setUpClass(cls): super().setUpClass() cls.mock_ssh_failure = lambda *args, **kwargs: {"status": -1} cls.mock_ssh_success = lambda *args, **kwargs: {"status": 0}
- Add test cases for different scenarios:
@patch.object("cx.tower.server", "test_ssh_connection") def test_server_check_ssh_connection(self, mock_ssh): # Test successful connection mock_ssh.side_effect = self.mock_ssh_success result = self.env["cetmix.tower"].server_check_ssh_connection( self.server_test_1.reference ) self.assertEqual( result["exit_code"], 0, "SSH connection should succeed with status 0" ) # Test connection timeout mock_ssh.side_effect = self.mock_ssh_failure result = self.env["cetmix.tower"].server_check_ssh_connection( self.server_test_1.reference, attempts=2, wait_time=1 ) self.assertEqual( result["exit_code"], SSH_CONNECTION_ERROR, "SSH connection should fail after 2 attempts with 1s wait time" ) # Test invalid server reference result = self.env["cetmix.tower"].server_check_ssh_connection( "invalid_reference" ) self.assertEqual( result["exit_code"], SSH_CONNECTION_ERROR, "SSH connection should fail with invalid server reference" )
102-106: 🛠️ Refactor suggestion
Mock the SSH connection for reliable testing.
The test makes an actual SSH connection attempt, which could lead to flaky tests and slower test execution. Based on established practices in cetmix-tower, SSH connection tests should use mocks to simulate different scenarios.
Consider this implementation:
+ @patch.object( + "cx.tower.server", + "test_ssh_connection", + return_value={"status": 0, "message": "Connection successful"} + ) def test_server_check_ssh_connection(self): # Test successful connection result = self.env["cetmix.tower"].server_check_ssh_connection( self.server_test_1.reference, ) self.assertEqual(result["exit_code"], 0, "SSH connection should be successful.")Committable suggestion skipped: line range outside the PR's diff.
cetmix_tower_git/views/cx_tower_git_project_views.xml (1)
114-117: 🛠️ Refactor suggestion
Security: Add inverse group check.
The current group check only handles non-members. Add an inverse check for group members to ensure complete access control.
- <div groups="!cetmix_tower_yaml.group_export"> + <div groups="!cetmix_tower_yaml.group_export" class="alert alert-warning" role="alert"> <h3>You must be a member of the "YAML/Export" group to export data as YAML.</h3> </div> + <div groups="cetmix_tower_yaml.group_export"> + <p class="text-muted">You have permission to export YAML data.</p> + </div>📝 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.<div groups="!cetmix_tower_yaml.group_export" class="alert alert-warning" role="alert"> <h3>You must be a member of the "YAML/Export" group to export data as YAML.</h3> </div> <div groups="cetmix_tower_yaml.group_export"> <p class="text-muted">You have permission to export YAML data.</p> </div>cetmix_tower_yaml/tests/test_tower_yaml_mixin.py (1)
26-32:
⚠️ Potential issueFix the exception message assertion placement.
The assertion for the exception message is incorrectly placed inside the
with self.assertRaises(ValidationError)block. This will never execute because the exception will be raised before reaching the assertion.Apply this diff to fix the assertion placement:
with self.assertRaises(ValidationError) as e: self.YamlMixin._convert_dict_to_yaml("not_a_dict") - self.assertEqual( - str(e), - _("Values must be a dictionary"), - "Exception message doesn't match", - ) +self.assertEqual( + str(e.exception), + _("Values must be a dictionary"), + "Exception message doesn't match", +)📝 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.with self.assertRaises(ValidationError) as e: self.YamlMixin._convert_dict_to_yaml("not_a_dict") self.assertEqual( str(e.exception), _("Values must be a dictionary"), "Exception message doesn't match", )cetmix_tower_git/models/cx_tower_git_source.py (1)
43-46:
⚠️ Potential issueAvoid duplicate labels for
remote_idsandremote_count.The fields
remote_ids(lines 36-42) andremote_count(lines 43-46) both have the label "Remotes". This can cause confusion in the UI and has resulted in the pipeline warning:[warning] Two fields (remote_count, remote_ids) of
cx.tower.git.source()have the same label: RemotesConsider changing the label of
remote_countto differentiate it fromremote_ids.Apply this diff to resolve the issue:
remote_count = fields.Integer( compute="_compute_remote_count", - string="Remotes", + string="Number of Remotes", )📝 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.remote_count = fields.Integer( compute="_compute_remote_count", string="Number of Remotes", )🧰 Tools
🪛 GitHub Actions: tests
[warning] Two fields (remote_count, remote_ids) of cx.tower.git.source() have the same label: Remotes
cetmix_tower_git/models/cx_tower_git_remote.py (3)
104-115: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid modifying other records in
_compute_namemethodIn the
_compute_namemethod, you're modifying thenamefield ofsource_remoterecords which may not be inself. This practice can lead to unexpected side effects and is discouraged in Odoo compute methods. Compute methods should only assign values to the computed field on records inself. Please refactor the method to compute thenameonly for the records inself, without modifying other records.
329-333:
⚠️ Potential issueUse correct authentication token placeholder in Bitbucket URL
In
_git_aggregator_prepare_url_bitbucket, the URL usesx-oauth-basic, but according to Bitbucket's documentation, the correct placeholder should bex-token-auth. The URL should be formatted as:-url = f"https://x-oauth-basic:$BITBUCKET_TOKEN@{url_without_protocol}" +url = f"https://x-token-auth:$BITBUCKET_TOKEN@{url_without_protocol}"This change ensures proper authentication with Bitbucket's OAuth mechanism.
📝 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.# https://x-token-auth:{access_token}@bitbucket.org/user/repo.git # From https://support.atlassian.com/bitbucket-cloud/docs/use-oauth-on-bitbucket-cloud/ url_without_protocol = self.url.replace("https://", "") url = f"https://x-token-auth:$BITBUCKET_TOKEN@{url_without_protocol}" return url
375-400:
⚠️ Potential issueCorrect the GitLab merge request reference
In
_git_aggregator_prepare_head_gitlab, the generated reference for merge requests should include therefs/prefix as expected by GitLab. Update the return statement to:-return f"merge-requests/{head_number}/head" +return f"refs/merge-requests/{head_number}/head"This ensures compatibility with GitLab's reference format for merge requests.
📝 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.def _git_aggregator_prepare_head_gitlab(self): """Prepare head for git aggregator for GitLab. Returns: Char: Prepared head for git aggregator """ # Extract branch name, PR/MR or commit number from head head_number = self.head.split("/")[-1] if not head_number: raise ValidationError( _("Git Aggregator: " "Head number is empty in %(head)s", head=self.head) ) # PR/MR if self.head_type == "pr": return f"refs/merge-requests/{head_number}/head" # Commit # https://gitlab.com/cetmix/test/-/tree/17.0-test-branch?ref_type=heads if self.head_type in ["commit", "branch"]: head_parts = head_number.split("?") return f"{head_parts[0]}" # Fallback to original head return self.headcetmix_tower_server/models/cx_tower_server.py (1)
450-486: 🛠️ Refactor suggestion
Ensure robust exception handling in
unlinkmethodWhen executing the delete plan with
server.plan_delete_id.execute(server, plan_log={"label": plan_label}), exceptions may occur and could interrupt the deletion process. To enhance robustness, wrap the execution call in a try-except block to gracefully handle any exceptions and update the server status accordingly.Apply this diff to implement exception handling:
try: server.plan_delete_id.execute(server, plan_log={"label": plan_label}) +except Exception as e: + server.status = "delete_error" + _logger.error(f"Deletion plan execution failed for server {server.id}: {e}") + continueCommittable suggestion skipped: line range outside the PR's diff.
cetmix_tower_git/tests/test_source.py (1)
30-42:
⚠️ Potential issueComplete the second test case.
The second test case appears to be incomplete. It defines the expected result but doesn't include the assertion part.
Add the missing assertion:
"target": "remote_1", } + prepared_result = self.git_source_2._git_aggregator_prepare_record() + self.assertEqual( + prepared_result, expected_result, "Prepared result is not correct" + )📝 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.# -- 2 -- # Source 2 expected_result = { "remotes": { "remote_1": "https://bitbucket.org/cetmix/cetmix-tower.git", "remote_2": "[email protected]:cetmix/cetmix-tower.git", }, "merges": [ {"remote": "remote_1", "ref": "dev"}, {"remote": "remote_2", "ref": "old"}, ], "target": "remote_1", } prepared_result = self.git_source_2._git_aggregator_prepare_record() self.assertEqual( prepared_result, expected_result, "Prepared result is not correct" )
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cetmix_tower_server/views/cx_tower_variable_value_view.xml (1)
37-37: Add a string attribute to the access_level field.The
access_levelfield in the search view would benefit from a descriptive label for better user experience.- <field name="access_level" /> + <field name="access_level" string="Access Level" />cetmix_tower_server/models/cx_tower_variable_value.py (1)
193-221: Fix the error message formatting.The error message is missing a space after "be" in the first line.
- "The access level for Variable Value '%(value)s' cannot be" - "lower than the access level of its Variable '%(variable)s'.\n" + "The access level for Variable Value '%(value)s' cannot be " + "lower than the access level of its Variable '%(variable)s'.\n"cetmix_tower_server/tests/test_variable.py (1)
980-1116: Consider refactoring the test for better maintainability.The test method is quite long and handles multiple concerns. Consider splitting it into smaller, focused test methods:
- test_variable_access_levels_in_commands
- test_variable_access_levels_in_files
This would improve maintainability and make it easier to identify the source of failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(1 hunks)cetmix_tower_server/models/cx_tower_variable.py(1 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(3 hunks)cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(6 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_variable.py(4 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)cetmix_tower_server/views/cx_tower_variable_value_view.xml(4 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cetmix_tower_server/models/cx_tower_variable.py
- cetmix_tower_server/views/cx_tower_server_view.xml
- cetmix_tower_server/security/cx_tower_variable_security.xml
- cetmix_tower_server/manifest.py
- cetmix_tower_server/README.rst
- cetmix_tower_server/views/cx_tower_variable_view.xml
🧰 Additional context used
📓 Learnings (1)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
🪛 ast-grep (0.31.1)
cetmix_tower_server/static/description/index.html
[warning] 372-372: "This link points to a plaintext HTTP URL. Prefer an encrypted HTTPS URL if possible."
Context: Cetmix Tower
Note: [CWE-319] Authentication Bypass by Primary Weakness [REFERENCES]
- https://cwe.mitre.org/data/definitions/319.html
(plaintext-http-link-html)
🔇 Additional comments (5)
cetmix_tower_server/static/description/index.html (1)
Line range hint
1-1297: Note: This is an auto-generated file.This file is generated by
oca-gen-addon-readmeand any direct changes will be overwritten. To fix the identified issues:
- Update the source documentation files (e.g., README.rst)
- Regenerate the documentation using
oca-gen-addon-readmecetmix_tower_server/security/cx_tower_variable_value_security.xml (2)
4-15: LGTM! Well-structured user access rule.The user access rule correctly restricts access to level 1 variables and enforces server subscription.
17-29: LGTM! Well-structured manager access rule.The manager access rule properly handles both global variables and server-specific variables with appropriate access levels.
cetmix_tower_server/models/cx_tower_variable_value.py (1)
148-156: LGTM! Clean implementation of access level computation.The
_compute_access_levelmethod correctly inherits the access level from the associated variable.cetmix_tower_server/tests/test_variable.py (1)
729-831: LGTM! Excellent test coverage for variable access rules.The test thoroughly covers all access scenarios for different user roles and permission levels.
|
|
|
/ocabot rebase |
|
Congratulations, PR rebased to 14.0-dev. |
CetmixGitDrone
force-pushed
the
14.0-t4055-access-levels-variables-add-feature-v2
branch
from
dec2672 to
11bd784
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cetmix_tower_server/tests/test_variable.py (1)
980-1116: Consider splitting this test into smaller, focused test cases.While the test coverage is excellent, the test method is quite long and tests multiple concerns:
- Command rendering with variables
- File name/directory/code rendering
- Access level validation
- User subscription effects
Consider refactoring into separate test methods:
test_variable_access_levels_in_commandstest_variable_access_levels_in_filestest_variable_access_levels_with_subscription-def test_variable_access_levels(self): +def test_variable_access_levels_in_commands(self): """Test that variables are rendered correctly in commands""" # Command-specific test code... +def test_variable_access_levels_in_files(self): """Test that variables are rendered correctly in files""" # File-specific test code... +def test_variable_access_levels_with_subscription(self): """Test that variables are rendered correctly with user subscription""" # Subscription-specific test code...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(1 hunks)cetmix_tower_server/models/cx_tower_variable.py(1 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(3 hunks)cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(6 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_variable.py(4 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)cetmix_tower_server/views/cx_tower_variable_value_view.xml(4 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- cetmix_tower_server/manifest.py
- cetmix_tower_server/models/cx_tower_variable.py
- cetmix_tower_server/models/cx_tower_variable_value.py
- cetmix_tower_server/views/cx_tower_variable_view.xml
- cetmix_tower_server/security/cx_tower_variable_security.xml
- cetmix_tower_server/README.rst
- cetmix_tower_server/views/cx_tower_variable_value_view.xml
- cetmix_tower_server/security/cx_tower_variable_value_security.xml
🧰 Additional context used
🪛 ast-grep (0.31.1)
cetmix_tower_server/static/description/index.html
[warning] 372-372: "This link points to a plaintext HTTP URL. Prefer an encrypted HTTPS URL if possible."
Context: Cetmix Tower
Note: [CWE-319] Authentication Bypass by Primary Weakness [REFERENCES]
- https://cwe.mitre.org/data/definitions/319.html
(plaintext-http-link-html)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (13)
cetmix_tower_server/static/description/index.html (7)
485-487: LGTM! Clear documentation about manager access level.The documentation clearly explains the manager's access level and permissions, which is essential for proper user management.
564-565: LGTM! Improved documentation about UI elements.The documentation now includes clear explanations about:
- The Flight Plan field in server templates
- The Files button functionality
Also applies to: 552-553
603-605: LGTM! Clear documentation about tag references.The documentation provides clear guidance about the tag reference field and its automatic generation feature.
1200-1205: LGTM! Important clarification about command execution path.The documentation now includes a crucial note about path variables and sudo access requirements, which helps prevent potential permission issues.
1219-1221: LGTM! Clear documentation about Run in wizard limitations.The documentation clearly explains the limitation of the Run in wizard button to single server execution, helping prevent confusion.
1241-1242: LGTM! Clear documentation about Show shared checkbox.The documentation provides a clear explanation of the Show shared checkbox functionality in the flight plan execution wizard.
1265-1268: LGTM! Clear documentation about log refresh functionality.The documentation clearly explains how to refresh individual logs and use the Refresh All functionality.
cetmix_tower_server/views/cx_tower_server_view.xml (2)
286-286: Consider removing the groups attribute as suggested in the previous review.A previous review comment suggested removing the "groups" key entirely, but this change instead modifies it to include both manager and user groups.
Could you clarify if the groups attribute should be removed entirely as previously suggested, or if the current approach of expanding access to both groups is the intended solution?
303-303: LGTM! The access_level field is consistently added to both tree and form views.The placement of the access_level field is appropriate and aligns with the PR's objective of enhancing access control.
Also applies to: 320-322
cetmix_tower_server/tests/test_variable.py (4)
Line range hint
340-403: LGTM! Comprehensive test coverage for variable access control.The test cases thoroughly validate:
- Access control for both private and global variables
- Different user group permissions
- Error cases with proper assertions
- Access changes based on user subscriptions
729-831: LGTM! Well-structured test for variable access rules.The test provides excellent coverage of:
- All access levels (private, manager, root)
- Visibility rules for different user groups
- Clear assertions with descriptive messages
832-895: LGTM! Clear and comprehensive test for variable value access rules.The test effectively validates:
- Access rules for variable values
- Different user roles and permissions
- Proper access level inheritance
896-978: LGTM! Thorough test coverage for variable rendering during execution.The test effectively ensures that:
- All variables are used during execution regardless of access level
- Variables with different access levels are properly rendered
- Access control doesn't interfere with variable usage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cetmix_tower_server/views/cx_tower_server_view.xml (2)
Line range hint
283-301: Consider enhancing the access_level field configuration in tree view.The
access_levelfield might benefit from additional attributes:
- Add a widget if it's a selection field
- Consider adding attrs to control visibility/editability based on user roles
- <field name="access_level" /> + <field name="access_level" + widget="selection" + attrs="{'readonly': [('variable_type', '=', 'o')]}" + groups="base.group_system"/>
316-318: Improve form view structure for better UX.The current implementation could be enhanced:
- Consider adding the
access_levelfield to an existing group instead of creating a new one- Add a help tooltip to explain the field's purpose
- Add appropriate widget and attrs configuration
- <group> - <field name="access_level" /> - </group> + <group string="Access Control"> + <field name="access_level" + widget="selection" + attrs="{'readonly': [('variable_type', '=', 'o')]}" + groups="base.group_system" + help="Determines the visibility level of this variable value"/> + </group>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server/README.rst
🧰 Additional context used
🪛 ast-grep (0.31.1)
cetmix_tower_server/static/description/index.html
[warning] 372-372: "This link points to a plaintext HTTP URL. Prefer an encrypted HTTPS URL if possible."
Context: Cetmix Tower
Note: [CWE-319] Authentication Bypass by Primary Weakness [REFERENCES]
- https://cwe.mitre.org/data/definitions/319.html
(plaintext-http-link-html)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (2)
cetmix_tower_server/static/description/index.html (1)
Line range hint
1-1305: This is an auto-generated file - changes should be made to the source RST files.The file is generated by oca-gen-addon-readme and should not be modified directly. Instead:
Update the source RST files to use HTTPS URLs instead of HTTP for:
http://cetmix.com/towerhttp://www.gnu.org/licenses/agpl-3.0-standalone.htmlhttp://odoo-community.orgFix broken links in the source RST files:
CONFIGURE.md/#configure-a-server-templateCONFIGURE.md/#configure-a-flight-plancetmix_tower_server/views/cx_tower_server_view.xml (1)
Line range hint
283-318: Verify security implications of access_level field.Since this is part of the access control enhancement:
- Ensure that the field inherits properly from
cx.tower.access.mixin- Verify that appropriate security rules are in place to restrict field visibility
✅ Verification successful
Security implementation for access_level field is properly structured ✅
The field implementation follows security best practices with:
- Proper inheritance from
cx.tower.access.mixin- Comprehensive record-level security rules for all user levels
- Proper access control for viewing and modifying variable values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify security rules and mixin inheritance for access_level field # Check for security rules related to access_level echo "Checking security rules for access_level..." rg -l "access_level" security/ # Check for proper mixin inheritance echo "Checking mixin inheritance..." rg -l "cx.tower.access.mixin" models/Length of output: 332
Script:
#!/bin/bash # Find relevant files and their content # Find Python files echo "=== Python files with potential model definitions ===" fd -e py # Find security-related XML files echo -e "\n=== Security-related XML files ===" fd -e xml | grep -i "security\|access" # Search for access_level field definition echo -e "\n=== Files containing access_level field definition ===" rg "access_level.*=.*fields\." -A 2 # Search for tower access mixin echo -e "\n=== Files containing tower access mixin ===" rg -i "class.*tower.*access.*mixin" -A 3Length of output: 8689
Script:
#!/bin/bash # Check security rules for variable values echo "=== Security rules for variable values ===" cat cetmix_tower_server/security/cx_tower_variable_value_security.xml # Check mixin implementation echo -e "\n=== Access mixin implementation ===" cat cetmix_tower_server/models/cx_tower_access_mixin.pyLength of output: 6998
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/tests/test_plan.py (1)
1586-1587: LGTM! Consider adding a descriptive comment.The access level setup for variables is correct and aligns with the test's purpose of validating user access rights.
Consider adding a comment to explain why these specific access levels are set:
+ # Set minimum access level for basic user access self.variable_os.write({"access_level": "1"}) self.variable_value.write({"access_level": "1"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cetmix_tower_server/tests/test_plan.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
| @@ -1583,6 +1583,8 @@ def test_plan_line_action_variable_values_user_access_rights(self): | |||
| # plan line action | |||
| # Add user to group | |||
| self.add_to_group(self.user_bob, "cetmix_tower_server.group_user") | |||
| self.variable_os.write({"access_level": "1"}) | |||
There was a problem hiding this comment.
You should use the helper write_and_invalidate method to ensure that record is updated properly. Check how it used in other places in this file, eg here.
| @@ -144,6 +150,16 @@ def _onchange_variable_id(self): | |||
| else: | |||
| rec.option_id = None | |||
|
|
|||
| @api.depends("variable_id") | |||
There was a problem hiding this comment.
What if variable access level has changed? Eg it was "2" and then became "1". This means "User" group members will not be able to access the variable values although they should.
| @@ -1583,6 +1583,8 @@ def test_plan_line_action_variable_values_user_access_rights(self): | |||
| # plan line action | |||
| # Add user to group | |||
| self.add_to_group(self.user_bob, "cetmix_tower_server.group_user") | |||
| self.variable_os.write({"access_level": "1"}) | |||
| self.variable_value.write({"access_level": "1"}) | |||
| test_plan_line_action_as_bob = self.plan_3_line_1_action.with_user( | |||
There was a problem hiding this comment.
This is a computed field that should be updated automatically when the variable access level is updated.
At the |
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature-v2
branch
from
dbceb79 to
45ac7b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
cetmix_tower_server/views/cx_tower_variable_value_view.xml (2)
Line range hint
12-14: Improve readability of readonly attributes.The readonly attributes for
server_id,server_template_id, andplan_line_action_idfields use similar conditions but are formatted differently. Consider standardizing the format for better maintainability.- <field - name="server_id" - attrs="{'readonly': ['|',('server_template_id', '!=', False), ('plan_line_action_id', '!=', False)]}" - /> - <field - name="server_template_id" - attrs="{'readonly': ['|',('server_id', '!=', False), ('plan_line_action_id', '!=', False)]}" - /> - <field - name="plan_line_action_id" - attrs="{'readonly': ['|',('server_id', '!=', False), ('server_template_id', '!=', False)]}" - /> + <field name="server_id" attrs="{ + 'readonly': ['|', + ('server_template_id', '!=', False), + ('plan_line_action_id', '!=', False) + ] + }"/> + <field name="server_template_id" attrs="{ + 'readonly': ['|', + ('server_id', '!=', False), + ('plan_line_action_id', '!=', False) + ] + }"/> + <field name="plan_line_action_id" attrs="{ + 'readonly': ['|', + ('server_id', '!=', False), + ('server_template_id', '!=', False) + ] + }"/>Also applies to: 15-18, 19-22
39-40: Consider adding a selection widget for access_level field.The access_level field would benefit from a selection widget to show predefined levels (1: User, 2: Manager, 3: Root) instead of a plain text field.
- <field name="access_level" /> + <field name="access_level" widget="selection"/>cetmix_tower_server/views/cx_tower_server_view.xml (1)
305-306: Consider grouping access control fields.The access_level field appears in both tree and form views but is not grouped with related fields. Consider creating a dedicated group for access control fields.
<tree editable="bottom"> - <field name="access_level" /> + <field name="access_level" groups="base.group_system"/> </tree> <form> <group> <field name="variable_id" /> <field name="variable_reference" widget="CopyClipboardChar" options="{'string': 'Copy reference'}" /> <field name="value_char" widget="CopyClipboardChar" options="{'string': 'Copy value'}" /> </group> - <group> - <field name="access_level" /> - </group> + <group name="access_control" string="Access Control"> + <field name="access_level" groups="base.group_system"/> + </group>Also applies to: 322-324
cetmix_tower_server/tests/test_variable.py (2)
903-985: Consider extracting common setup code.The test methods share similar setup code for creating variables and users. Consider extracting this into helper methods or setUp to reduce duplication.
def setUp(self): super().setUp() # Create variables with different access levels self.variable_user = self.Variable.create({ "name": "User Variable", "access_level": "1" }) self.variable_manager = self.Variable.create({ "name": "Manager Variable", "access_level": "2" }) self.variable_root = self.Variable.create({ "name": "Root Variable", "access_level": "3" }) def _create_variable_value(self, variable, server, value, is_global=False): return self.VariableValue.create({ "variable_id": variable.id, "server_id": None if is_global else server.id, "is_global": is_global, "value_char": value, "access_level": variable.access_level, }) def _setup_user_groups(self, user, groups_to_add=None, groups_to_remove=None): if groups_to_remove: self.remove_from_group(user, groups_to_remove) if groups_to_add: for group in groups_to_add: self.add_to_group(user, group)
987-1123: Enhance test method documentation.The test method
test_variable_access_levelscould benefit from more detailed documentation explaining the test scenarios and expected outcomes.def test_variable_access_levels(self): - """Test that variables of all access levels are rendered correctly""" + """Test that variables of all access levels are rendered correctly. + + Test cases: + 1. Create variables with different access levels (user, manager, root) + 2. Create values for these variables in both server and global scope + 3. Create a command and file that use these variables + 4. Verify as a user: + - Command path rendering with variables + - Command code rendering with variables + - File name rendering with variables + - File directory rendering with variables + - File code rendering with variables + + Expected outcomes: + - All variables should be rendered correctly regardless of access level + - User should be able to see rendered values even for higher access level variables + - Access control should only affect direct access to variables, not their rendered values + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(1 hunks)cetmix_tower_server/models/cx_tower_variable.py(2 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(4 hunks)cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(6 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_plan.py(1 hunks)cetmix_tower_server/tests/test_variable.py(4 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)cetmix_tower_server/views/cx_tower_variable_value_view.xml(4 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- cetmix_tower_server/manifest.py
- cetmix_tower_server/models/cx_tower_variable_value.py
- cetmix_tower_server/views/cx_tower_variable_view.xml
- cetmix_tower_server/tests/test_plan.py
- cetmix_tower_server/models/cx_tower_variable.py
- cetmix_tower_server/security/cx_tower_variable_security.xml
- cetmix_tower_server/README.rst
- cetmix_tower_server/security/cx_tower_variable_value_security.xml
🧰 Additional context used
🪛 GitHub Actions: tests
cetmix_tower_server/views/cx_tower_server_view.xml
[error] 167-167: Field "option_ids_domain" does not exist in model "cx.tower.variable.value"
🪛 ast-grep (0.31.1)
cetmix_tower_server/static/description/index.html
[warning] 372-372: "This link points to a plaintext HTTP URL. Prefer an encrypted HTTPS URL if possible."
Context: Cetmix Tower
Note: [CWE-319] Authentication Bypass by Primary Weakness [REFERENCES]
- https://cwe.mitre.org/data/definitions/319.html
(plaintext-http-link-html)
🔇 Additional comments (2)
cetmix_tower_server/static/description/index.html (1)
Line range hint
1-1304: This is an auto-generated file.This file is generated by oca-gen-addon-readme and should not be modified directly. Any changes should be made to the source RST files instead, as manual changes to this file will be overwritten.
cetmix_tower_server/tests/test_variable.py (1)
736-838: Comprehensive test coverage for access rules.The test method thoroughly verifies access rules for variables with different access levels. Good use of assertions and clear error messages.
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature-v2
branch
from
45ac7b3 to
dbceb79
Compare
Task: 4055
Task: 4055
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature-v2
branch
from
dbceb79 to
6158844
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cetmix_tower_server/tests/test_variable.py (2)
839-902: Consider adding edge case tests.While the test provides good coverage of basic access rules, consider adding tests for:
- Access attempts with invalid access levels
- Behavior when access level changes while users are accessing values
- Concurrent access scenarios
903-985: Consider adding complex rendering scenarios.The test effectively verifies basic variable rendering. Consider adding tests for:
- Nested variable references
- Variable rendering with special characters
- Error cases in variable rendering
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(1 hunks)cetmix_tower_server/models/cx_tower_variable.py(2 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(4 hunks)cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(6 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_plan.py(1 hunks)cetmix_tower_server/tests/test_variable.py(4 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)cetmix_tower_server/views/cx_tower_variable_value_view.xml(4 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- cetmix_tower_server/manifest.py
- cetmix_tower_server/tests/test_plan.py
- cetmix_tower_server/security/cx_tower_variable_security.xml
- cetmix_tower_server/models/cx_tower_variable_value.py
- cetmix_tower_server/models/cx_tower_variable.py
- cetmix_tower_server/views/cx_tower_server_view.xml
- cetmix_tower_server/views/cx_tower_variable_view.xml
- cetmix_tower_server/README.rst
- cetmix_tower_server/views/cx_tower_variable_value_view.xml
- cetmix_tower_server/security/cx_tower_variable_value_security.xml
🧰 Additional context used
🪛 ast-grep (0.31.1)
cetmix_tower_server/static/description/index.html
[warning] 372-372: "This link points to a plaintext HTTP URL. Prefer an encrypted HTTPS URL if possible."
Context: Cetmix Tower
Note: [CWE-319] Authentication Bypass by Primary Weakness [REFERENCES]
- https://cwe.mitre.org/data/definitions/319.html
(plaintext-http-link-html)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (4)
cetmix_tower_server/static/description/index.html (1)
Line range hint
1-1304: Documentation: Update source files to fix security concerns.This file is auto-generated by oca-gen-addon-readme and should not be modified directly. However, there are some security concerns that should be addressed in the source files:
Update plaintext HTTP URLs to HTTPS in the source files:
http://cetmix.com/tower→https://cetmix.com/towerhttp://www.gnu.org/licenses/agpl-3.0-standalone.html→https://www.gnu.org/licenses/agpl-3.0-standalone.htmlhttp://odoo-community.org→https://odoo-community.orgFix broken links to
CONFIGURE.mdin the source files by updating them to use the correct relative path:readme/CONFIGURE.md.Run the following script to verify the URLs in the source files:
#!/bin/bash # Description: Check for plaintext HTTP URLs in source files. # Search for HTTP URLs in RST files echo "Checking RST files for HTTP URLs..." rg -i "http://" --type rst # Search for broken links to CONFIGURE.md echo -e "\nChecking for broken links to CONFIGURE.md..." rg -i "CONFIGURE\.md" --type rstcetmix_tower_server/tests/test_variable.py (3)
Line range hint
340-403: LGTM! Comprehensive test coverage for variable value access control.The test thoroughly validates access control for variable values across different user roles and subscription states. It effectively tests:
- Access control for private and global values
- Impact of user group membership
- Effect of server subscription
736-838: LGTM! Well-structured test for variable access rules.The test provides excellent coverage of variable access rules:
- Tests all access levels (1-3)
- Verifies correct access for each user group
- Includes helpful error messages for debugging
987-1123: LGTM! Excellent test coverage for variable access levels.The test thoroughly validates variable rendering across different contexts:
- Tests variables in commands and files
- Verifies rendering with different access levels
- Includes comprehensive assertions
| @@ -40,6 +40,14 @@ def _compute_value_ids_count(self): | |||
| for rec in self: | |||
| rec.value_ids_count = len(rec.value_ids) | |||
|
|
|||
| @api.onchange("access_level") | |||
| def _onchange_access_level(self): | |||
There was a problem hiding this comment.
What if access level is updated from code? And you already have a "compute" method in the variable values. Why do we need another function for the same thing?
There was a problem hiding this comment.
The only other option I have left is to make - inverse="_inverse_access_level" field to correctly change the access level for variable_id as well.
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature-v2
branch
from
4209582 to
6bf84f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cetmix_tower_server/tests/test_variable.py (5)
Line range hint
340-457: LGTM! Consider adding edge cases.The test method thoroughly covers the core access control scenarios.
Consider adding test cases for:
- Concurrent access from multiple users
- Access control during variable value updates
- Edge cases with empty or invalid access levels
def test_concurrent_variable_value_access(self): """Test concurrent access to variable values from multiple users""" # Create test users with different access levels users = [ self.env['res.users'].create({ 'name': f'Test User {i}', 'login': f'test_user_{i}', }) for i in range(3) ] # Set up concurrent access scenarios # Add test implementation here
746-847: LGTM! Consider strengthening assertions.The test comprehensively covers variable access rules across different user groups.
Consider adding assertions for:
- Variable creation permissions
- Access level modification attempts
- Invalid access level values
# Add these assertions within the test method: # Test variable creation permissions with self.assertRaises(AccessError): self.Variable.with_user(user_bob).create({ 'name': 'Unauthorized Variable', 'access_level': '3' # Attempt to create root-level variable }) # Test access level modification with self.assertRaises(AccessError): variable_private.with_user(user_bob).write({ 'access_level': '3' # Attempt to escalate access level })
849-911: LGTM! Consider adding error case coverage.The test effectively validates variable value access rules.
Consider adding test cases for:
- Access denial after subscription expiry
- Access conflicts between global and local values
- Invalid access level combinations
# Add these test cases within the test method: # Test access after subscription removal server.message_unsubscribe([user_bob.partner_id.id]) with self.assertRaises(AccessError): variable_private_value.with_user(user_bob).read(['value_char']) # Test global vs local value access conflict global_value = self.VariableValue.create({ 'variable_id': variable_private.id, 'is_global': True, 'value_char': 'Global Override', 'access_level': '1' }) # Add assertions to verify access precedence
913-995: LGTM! Consider adding complex rendering scenarios.The test effectively validates variable rendering during execution.
Consider adding test cases for:
- Nested variable references
- Circular dependencies
- Missing variable handling
# Add these test cases: # Test nested variable rendering variable_nested = self.Variable.create({ 'name': 'Nested Variable', 'access_level': '1' }) self.VariableValue.create({ 'variable_id': variable_nested.id, 'server_id': server.id, 'value_char': '{{ User Variable }}/nested', 'access_level': '1' }) # Test circular dependency detection variable_circular1 = self.Variable.create({ 'name': 'Circular1', 'access_level': '1' }) variable_circular2 = self.Variable.create({ 'name': 'Circular2', 'access_level': '1' }) # Add circular reference values and verify handling
997-1133: LGTM! Consider adding validation for edge cases.The test thoroughly validates variable access levels and rendering.
Consider adding validation for:
- Variable rendering in batch operations
- Performance with large numbers of variables
- Error handling for malformed templates
# Add these validation cases: # Test batch rendering batch_commands = [ self.Command.create({ 'name': f'Batch Command {i}', 'path': '{{ directory2 }}/batch_{i}', 'code': 'echo {{ version2 }}', 'server_ids': [(4, server.id)], 'access_level': '1' }) for i in range(10) ] # Test malformed template handling malformed_command = self.Command.create({ 'name': 'Malformed Command', 'path': '{{ unclosed_variable', 'code': 'echo {{ invalid_syntax }}', 'server_ids': [(4, server.id)], 'access_level': '1' }) # Add assertions for error handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/models/cx_tower_variable.py(1 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(4 hunks)cetmix_tower_server/tests/test_variable.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cetmix_tower_server/models/cx_tower_variable.py
- cetmix_tower_server/models/cx_tower_variable_value.py
| @@ -133,6 +140,25 @@ def _onchange_variable_id(self): | |||
| if rec.variable_id.option_ids: | |||
| rec.value_char = False | |||
|
|
|||
| @api.depends("variable_id.access_level") | |||
There was a problem hiding this comment.
What if variable has changed?
| if rec.variable_id: | ||
| rec.access_level = rec.variable_id.access_level | ||
|
|
||
| def _inverse_access_level(self): |
There was a problem hiding this comment.
What is this method needed for? Is there are need to change the variable access level if variable value access level is changed?
And it there was such a request, why not to use a related field ?
…sts for Variables and Values
This is a new PR, cleaned of unnecessary commits and inconsistencies between branch versions.
So as not to lose any old comments and edits, and to keep them in the history - have a look at the old PR.
This PR improves access control logic, introduces new features, and refines existing functionality for
TowerVariableandTowerVariableValuemodels. The changes align with OCA guidelines and include comprehensive test coverage.Refactoring:
Improvements:
cx.tower.access.mixininTowerVariableandTowerVariableValue.access_levelfield in models and views for dynamic control of variable visibility.access_level.Additions:
cx_tower_variable_security.xmlto define access rules forTowerVariable.cx_tower_variable_value_security.xmlto include updated domain rules forTowerVariableValue.Tests:
test_variable_access_levels: Verifies access based onaccess_level.test_variable_access_rules: Ensures correct rules for variables.test_variable_value_access_rules: Validates refined access logic.test_variable_rendering_in_execution: Confirms all variables are rendered correctly during execution.test_variable_value_accessto ensure compatibility with new logic.Steps to Reproduce:
access_level.group_user,group_manager,group_root).File Changes:
cx_tower_variable_security.xml: New file to manage access rules forTowerVariable.cx_tower_variable_value_security.xml: Updated domain rules forTowerVariableValue.TowerVariable,TowerVariableValue):cx.tower.access.mixin.access_levelfields with required logic.access_level.test_variable_value_access.test_variable_value_server_subscription.Task: 4055
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Version Update