Added ability to copy/paste annotation guide with assets #7989
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes address a bug related to copying/pasting annotation guides with assets, refine the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add 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: 5
Outside diff range and nitpick comments (3)
cvat-ui/src/components/md-guide/guide-page.tsx (1)
Line range hint
77-128: Refactor file handling to improve error handling and readability.The
handleInsertFilesfunction is quite complex and handles multiple responsibilities. Consider breaking it down into smaller functions, each handling a specific part of the file processing workflow. This will improve readability and maintainability.+ async function processFile(file: File, guideId: number): Promise<[File, string]> { + const { uuid } = await core.assets.create(file, guideId); + return [file, uuid]; + } + + function updateTextAreaValue(addedAssets: [File, string][], textAreaValue: string, selectionStart: number, selectionEnd: number): string { + // Implementation goes here + } + - // Original complex function + // Simplified function using the above helperscvat/apps/engine/serializers.py (2)
Line range hint
14-14: Consider using a more specific import forreandshutilto improve code clarity and maintainability.
[!TIP]
Codebase VerificationEnsure the deletion of the temporary file created by
NamedTemporaryFile.
- The temporary file created with
NamedTemporaryFileis not deleted after its use, which could lead to potential security vulnerabilities.- Consider adding code to delete the temporary file using
os.remove(temporary_file)after it is no longer needed.Analysis chain
Line range hint
14-14: Ensure thatNamedTemporaryFileis used securely, especially in a web application context, to avoid potential security vulnerabilities.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the secure usage of `NamedTemporaryFile`. # Test: Search for `NamedTemporaryFile` usage in the file. Expect: Secure usage patterns. rg "NamedTemporaryFile" cvat/apps/engine/serializers.pyLength of output: 273
Script:
#!/bin/bash # Description: Extract surrounding lines of `NamedTemporaryFile` usage to verify secure handling. # Extract 5 lines before and after each occurrence of `NamedTemporaryFile`. rg -C 5 "NamedTemporaryFile" cvat/apps/engine/serializers.pyLength of output: 1224
Script:
#!/bin/bash # Description: Verify the deletion of the temporary file created by `NamedTemporaryFile`. # Search for the deletion of the temporary file. rg "os.remove\|shutil.rmtree" cvat/apps/engine/serializers.pyLength of output: 63
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- changelog.d/20240605_154324_boris_fix_anno_guide.md (1 hunks)
- cvat-ui/src/components/md-guide/guide-page.tsx (6 hunks)
- cvat-ui/src/components/md-guide/styles.scss (1 hunks)
- cvat/apps/engine/permissions.py (2 hunks)
- cvat/apps/engine/serializers.py (1 hunks)
- cvat/apps/engine/views.py (3 hunks)
Files skipped from review due to trivial changes (1)
- cvat-ui/src/components/md-guide/styles.scss
Additional comments not posted (12)
cvat/apps/engine/serializers.py (10)
Line range hint
14-14: Check ifstringis used in the file, as it is imported but not visible in the provided code.Verification successful
The
stringmodule is used in the filecvat/apps/engine/serializers.pyin the expressionstring.ascii_lowercase + string.digits + "-".
- Line:
string.ascii_lowercase + string.digits + "-"Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `string` module in the file. # Test: Search for `string` usage in the file. Expect: Usage instances or none. rg "string\." cvat/apps/engine/serializers.pyLength of output: 104
Line range hint
14-14: Ensure thatUnionis used in the file, as it is imported but not visible in the provided code.Verification successful
The
Uniontype is indeed used multiple times in the filecvat/apps/engine/serializers.py. Therefore, the import ofUnionis necessary and correctly utilized.
- Lines where
Unionis used:
parent_instance: Union[models.Project, models.Task]def _get_parent_info(cls, parent_instance: Union[models.Project, models.Task])instance: Union[models.Project, models.Task]Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Union` in the file. # Test: Search for `Union` usage in the file. Expect: Usage instances or none. rg "Union" cvat/apps/engine/serializers.pyLength of output: 428
Line range hint
14-14: Check iftextwrapis necessary since it is imported but not visible in the provided code.Verification successful
The
textwrapmodule is used multiple times in thecvat/apps/engine/serializers.pyfile, specifically for thededentfunction. Therefore, the import is necessary.
- Instances of
textwrap.dedentusage are found throughout the file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `textwrap` module in the file. # Test: Search for `textwrap` usage in the file. Expect: Usage instances or none. rg "textwrap\." cvat/apps/engine/serializers.pyLength of output: 781
Line range hint
14-14: Confirm the usage ofOrderedDictas it is imported but not visible in the provided code.Verification successful
The
OrderedDictis indeed used in the filecvat/apps/engine/serializers.pyat multiple locations:
- Line 14:
from typing import Any, Dict, Iterable, Optional, OrderedDict, Union- Line 2159:
errors = OrderedDict()- Line 2161:
return OrderedDict([(key, obj[key]) for key in keys])- Line 2163:
OrderedDict([(key, attr[key]) for key in attr_keys]) for attr in attr_setScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `OrderedDict` in the file. # Test: Search for `OrderedDict` usage in the file. Expect: Usage instances or none. rg "OrderedDict" cvat/apps/engine/serializers.pyLength of output: 288
Line range hint
14-14: Check ifOptionalis necessary since it is imported but not visible in the provided code.Verification successful
The
Optionalimport is necessary as it is used in multiple type annotations within the file.
- Lines where
Optionalis used:
parent_label: Optional[models.Label] = None) -> Optional[models.Label]:def _configure_related_storages(validated_data: Dict[str, Any]) -> Dict[str, Optional[models.Storage]]:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Optional` in the file. # Test: Search for `Optional` usage in the file. Expect: Usage instances or none. rg "Optional" cvat/apps/engine/serializers.pyLength of output: 407
Line range hint
14-14: Verify the necessity of importingosas it is not explicitly used in the visible code.
Line range hint
14-14: Ensure thatIterableis used in the file, as it is imported but not visible in the provided code.Verification successful
The
Iterabletype is indeed used in the filecvat/apps/engine/serializers.pyat multiple locations.
- Line 14:
from typing import Any, Dict, Iterable, Optional, OrderedDict, Union- Line 2159:
sublabels: Iterable[Dict[str, Any]],- Line 2160:
labels: Iterable[Dict[str, Any]],- Line 2164:
labels: Iterable[Dict[str, Any]],Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Iterable` in the file. # Test: Search for `Iterable` usage in the file. Expect: Usage instances or none. rg "Iterable" cvat/apps/engine/serializers.pyLength of output: 243
Line range hint
14-14: Verify the necessity of importingAnyas it is not explicitly used in the visible code.Verification successful
The import of
Anyfrom thetypingmodule is necessary as it is used in type annotations throughout the file.
validated_data: Dict[str, Any]sublabels: Iterable[Dict[str, Any]]labels: Iterable[Dict[str, Any]]- Function
_configure_related_storages(validated_data: Dict[str, Any]) -> Dict[str, Optional[models.Storage]]Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Any` module in the file. # Test: Search for `Any` usage in the file. Expect: Usage instances or none. rg "Any" cvat/apps/engine/serializers.pyLength of output: 418
Line range hint
14-14: Check ifDictis used in the file, as it is imported but not visible in the provided code.Verification successful
The
Dicttype from thetypingmodule is indeed used in the filecvat/apps/engine/serializers.py. Here are the specific instances:
- Line 14:
from typing import Any, Dict, Iterable, Optional, OrderedDict, Union- Line 2159:
validated_data: Dict[str, Any],- Line 2160:
sublabels: Iterable[Dict[str, Any]],- Line 2161:
labels: Iterable[Dict[str, Any]],- Line 2162:
labels: Iterable[Dict[str, Any]],- Line 2163:
errors = OrderedDict()- Line 2164:
return OrderedDict([(key, obj[key]) for key in keys])- Line 2165:
OrderedDict([(key, attr[key]) for key in attr_keys]) for attr in attr_set- Line 2166:
validated_data: Dict[str, Any],- Line 2167:
def _configure_related_storages(validated_data: Dict[str, Any]) -> Dict[str, Optional[models.Storage]]:The
Dicttype is used in type annotations for various functions and variables, confirming its necessity in the file.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Dict` module in the file. # Test: Search for `Dict` usage in the file. Expect: Usage instances or none. rg "Dict" cvat/apps/engine/serializers.pyLength of output: 590
Line range hint
1-1: Ensure the copyright notice is up-to-date.cvat/apps/engine/views.py (2)
2795-2847: Ensure proper handling of asset copying in_update_related_assets.Verification successful
The
_update_related_assetsmethod is called in bothperform_createandperform_updatemethods. These methods are responsible for creating and updating instances, respectively. The context provided shows that_update_related_assetsis used to handle assets related to anAnnotationGuideduring these operations.
perform_createandperform_updateboth call_update_related_assetsafter the main operation (create/update) is performed.- The
perform_destroymethod does not call_update_related_assets, which is expected since it deals with deletion.Given this context, the asset copying logic in
_update_related_assetsappears to be properly integrated into the create and update workflows.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify asset copying logic in `_update_related_assets`. # Test: Search for the method usage. Expect: Proper handling of asset copying. rg --type python $'_update_related_assets'Length of output: 72
Script:
#!/bin/bash # Description: Verify asset copying logic in `_update_related_assets`. # Test: Search for the method usage. Expect: Proper handling of asset copying. rg --type py $'_update_related_assets'Length of output: 328
Script:
#!/bin/bash # Extract lines around the calls to `_update_related_assets` to understand the context. rg --type py -A 5 -B 5 $'_update_related_assets' cvat/apps/engine/views.pyLength of output: 1101
2856-2857: Verify thattarget.touch()is called appropriately after updating related assets.Also applies to: 2861-2862
Verification successful
The search results show that
target.touch()is called twice incvat/apps/engine/views.py. However, to ensure it is called appropriately after updating related assets, we need to verify the context around these calls.
The context around the
target.touch()calls confirms that they are appropriately called after updating related assets in bothperform_createandperform_updatemethods.
perform_createmethod:def perform_create(self, serializer): super().perform_create(serializer) self._update_related_assets(self.request, serializer.instance) serializer.instance.target.touch()
perform_updatemethod:def perform_update(self, serializer): super().perform_update(serializer) self._update_related_assets(self.request, serializer.instance) serializer.instance.target.touch()This ensures that
target.touch()is called after_update_related_assetsin both cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `target.touch()` is called after updating related assets. # Test: Search for the method usage. Expect: Proper invocation of `target.touch()`. rg --type python $'target.touch()'Length of output: 66
Script:
#!/bin/bash # Description: Verify that `target.touch()` is called after updating related assets. # Test: Search for the method usage. Expect: Proper invocation of `target.touch()`. rg --type py 'target.touch()'Length of output: 169
Script:
#!/bin/bash # Description: Verify the context around `target.touch()` calls to ensure they follow `_update_related_assets`. # Extract 5 lines before and after each occurrence of `target.touch()` rg --type py 'target.touch()' -A 5 -B 5Length of output: 1017
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7989 +/- ##
===========================================
- Coverage 83.36% 83.33% -0.04%
===========================================
Files 388 388
Lines 41315 41339 +24
Branches 3855 3855
===========================================
+ Hits 34443 34450 +7
- Misses 6872 6889 +17
|
|
| @@ -2788,6 +2793,67 @@ class AnnotationGuidesViewSet( | |||
| ordering = "-id" | |||
| iam_organization_field = None | |||
|
|
|||
| def _update_related_assets(self, request, guide: AnnotationGuide): | |||
There was a problem hiding this comment.
There are many db objects being modified here, so I suggest wrapping this function as an atomic transaction
There was a problem hiding this comment.
I added transaction.atomic where we copy existing assets
|
Motivation and context
Related #6536
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Bug Fixes
Refactor
GuidePagecomponent toAnnotationGuidePageand refactored logic for fetching and saving guides.Style
New Features