feat: add cyclic check on parent_folder #1388
Conversation
Mohamed-Hacene
requested review from
ab-smith,
nas-tabchiche and
eric-intuitem
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe pull request introduces a new validation method Changes
Sequence DiagramsequenceDiagram
participant User
participant Serializer
participant Validation
User->>Serializer: Attempt to set parent folder
Serializer->>Validation: Validate parent folder
alt Cycle detected
Validation-->>User: Raise ValidationError
else No cycle
Validation-->>Serializer: Allow parent folder
end
Poem
✨ 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: 0
🧹 Nitpick comments (1)
enterprise/backend/enterprise_core/serializers.py (1)
23-32: Improve the validation method implementation and documentation.The validation logic is correct but can be enhanced:
- Simplify the nested if statements
- Add more details to the docstring
Apply this diff to improve the implementation:
def validate_parent_folder(self, parent_folder): """ Check that the folders graph will not contain cycles + + Args: + parent_folder: The folder to be set as parent + + Returns: + The validated parent_folder + + Raises: + ValidationError: If setting this parent_folder would create a cycle in the folder graph """ if not self.instance: return parent_folder - if parent_folder: - if parent_folder == self.instance or parent_folder in self.instance.get_sub_folders(): - raise serializers.ValidationError("errorFolderGraphMustNotContainCycles") + if parent_folder and (parent_folder == self.instance or parent_folder in self.instance.get_sub_folders()): + raise serializers.ValidationError("errorFolderGraphMustNotContainCycles") return parent_folder🧰 Tools
🪛 Ruff (0.8.2)
29-30: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
enterprise/backend/enterprise_core/serializers.py(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
enterprise/backend/enterprise_core/serializers.py
29-30: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (2)
frontend/messages/en.json (1)
999-999: LGTM! Error message is clear and consistent.The error message follows the same pattern as the existing asset graph error message and clearly communicates the validation failure to users.
frontend/messages/fr.json (1)
999-999: LGTM! French translation is accurate and consistent.The French translation accurately conveys the same meaning as the English message and maintains consistency with the existing asset graph error message pattern.
nas-tabchiche
deleted the
CA-758-prevent-folder-graph-from-being-cyclic
branch
Summary by CodeRabbit