Show job assignee from quality reports in UI #8123
Conversation
zhiltsov-max
requested review from
azhavoro,
bsekachev,
SpecLad,
Marishka17 and
nmanovic
as code owners
zhiltsov-max
changed the title
|
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 changes introduce a new feature to track the last update time of assignees for jobs, projects, and tasks in CVAT. Key updates include adding an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: Update assignee of a task
Frontend->>Backend: Send update request with new assignee info
Backend->>Database: Update task assignee and assignee_updated_date
Database-->>Backend: Confirm update
Backend-->>Frontend: Send confirmation of update
Frontend-->>User: Display updated assignee information
User->>Frontend: Request quality report
Frontend->>Backend: Fetch quality report
Backend->>Database: Retrieve quality report and assignee info
Database-->>Backend: Provide quality report
Backend-->>Frontend: Send quality report data
Frontend-->>User: Display quality report with assignee info
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 Configration 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 (5)
tests/python/rest_api/test_quality_control.py (2)
431-433: Add a docstring to the test method.Consider adding a docstring to the test method to describe its purpose.
""" Test that report data includes job assignees when available. """
500-502: Add a docstring to the test method.Consider adding a docstring to the test method to describe its purpose.
""" Test that reports can be created with job assignees. """tests/python/rest_api/test_jobs.py (3)
314-314: Add a docstring to the test method.Consider adding a docstring to describe the purpose of this test case.
""" Test creating a job with and without an assignee. """
Line range hint
241-241:
Add a docstring to the test method.Consider adding a docstring to describe the purpose of this test case.
""" Test updating the assignee and the corresponding updated date. """
[!TIP]
Codebase VerificationEnsure proper test case coverage.
The test case
test_can_update_assignee_updated_date_on_assignee_updatesdoes not handle edge cases such as invalid assignee IDs or missingusersentries. Consider adding tests to cover these scenarios.
- File:
tests/python/rest_api/test_jobs.py- Function:
test_can_update_assignee_updated_date_on_assignee_updatesAnalysis chain
Line range hint
242-270:
Ensure proper test case coverage.The test case covers updating the assignee and verifying the updated date. Verify that it handles edge cases such as invalid assignee IDs or missing
usersentries.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the edge cases for updating the assignee and the corresponding updated date. # Test: Ensure the test covers invalid assignee IDs and missing users entries. rg --type python $'test_can_update_assignee_updated_date_on_assignee_updates'Length of output: 107
Script:
#!/bin/bash # Description: Verify the edge cases for updating the assignee and the corresponding updated date. # Test: Search for the test function definition and its contents to ensure it covers invalid assignee IDs and missing users entries. rg 'def test_can_update_assignee_updated_date_on_assignee_updates' -A 20Length of output: 5194
Script:
#!/bin/bash # Description: Verify the edge cases for updating the assignee and the corresponding updated date. # Test: Search for additional lines of code in the test function definition and its contents to ensure it covers invalid assignee IDs and missing users entries. rg 'def test_can_update_assignee_updated_date_on_assignee_updates' -A 50Length of output: 10342
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (22)
- changelog.d/20240704_201201_mzhiltso_track_last_assignee_update.md (1 hunks)
- cvat-core/src/quality-report.ts (4 hunks)
- cvat-core/src/server-response-types.ts (1 hunks)
- cvat-ui/src/components/analytics-page/task-quality/job-list.tsx (3 hunks)
- cvat/apps/engine/migrations/0079_job_assignee_updated_date_and_more.py (1 hunks)
- cvat/apps/engine/models.py (5 hunks)
- cvat/apps/engine/serializers.py (13 hunks)
- cvat/apps/engine/tests/test_rest_api.py (1 hunks)
- cvat/apps/quality_control/migrations/0002_qualityreport_assignee.py (1 hunks)
- cvat/apps/quality_control/models.py (3 hunks)
- cvat/apps/quality_control/quality_reports.py (9 hunks)
- cvat/apps/quality_control/serializers.py (3 hunks)
- cvat/apps/quality_control/views.py (2 hunks)
- cvat/schema.yml (4 hunks)
- tests/python/rest_api/test_jobs.py (3 hunks)
- tests/python/rest_api/test_projects.py (3 hunks)
- tests/python/rest_api/test_quality_control.py (3 hunks)
- tests/python/rest_api/test_tasks.py (3 hunks)
- tests/python/shared/assets/jobs.json (25 hunks)
- tests/python/shared/assets/projects.json (12 hunks)
- tests/python/shared/assets/quality_reports.json (12 hunks)
- tests/python/shared/assets/tasks.json (18 hunks)
Files skipped from review due to trivial changes (6)
- changelog.d/20240704_201201_mzhiltso_track_last_assignee_update.md
- cvat/apps/engine/migrations/0079_job_assignee_updated_date_and_more.py
- cvat/apps/engine/tests/test_rest_api.py
- cvat/apps/quality_control/migrations/0002_qualityreport_assignee.py
- tests/python/shared/assets/projects.json
- tests/python/shared/assets/tasks.json
Additional comments not posted (43)
cvat-core/src/quality-report.ts (4)
6-6: Import statement approved.The import of the
Userclass is correct and necessary for the newassigneeproperty.
40-40: Property addition approved.The
assigneeproperty is correctly defined asUser | null.
52-57: Constructor logic approved.The constructor correctly initializes the
assigneeproperty based on theinitialData.
88-90: Getter method approved.The getter method for the
assigneeproperty is correctly implemented.cvat/apps/quality_control/serializers.py (3)
9-9: Import statement approved.The import of
BasicUserSerializerfromengine_serializersis correct and necessary for the newassigneefield in theQualityReportSerializer.
47-47: Field addition approved.The
assigneefield is correctly defined usingBasicUserSerializer.
62-62: Field integration approved.The
assigneefield is correctly integrated into thefieldsarray in theQualityReportSerializer.cvat/apps/quality_control/models.py (2)
15-15: Import statement approved.The import of the
Usermodel is correct and necessary for the newassigneefield in theQualityReportmodel.
89-91: Field addition approved.The
assigneefield is correctly defined as aForeignKeyto theUsermodel withon_delete=models.SET_NULL.cvat-ui/src/components/analytics-page/task-quality/job-list.tsx (3)
70-75: Function modification approved.The
collectUsersfunction correctly maps thejobsReportsand returns the expected values.
130-131: Function modification approved.The
renderfunction correctly references thereportfor theassigneefield.
235-235: Object modification approved.The object correctly references the
reportfor thestage,quality,conflicts, andframe_intersectionfields.tests/python/shared/assets/quality_reports.json (6)
7-7: Addition of "assignee" field looks good.The "assignee" field is added with a value of null, which is consistent with the rest of the data structure.
39-39: Addition of "assignee" field looks good.The "assignee" field is added with a value of null, which is consistent with the rest of the data structure.
71-71: Addition of "assignee" field looks good.The "assignee" field is added with a value of null, which is consistent with the rest of the data structure.
103-103: Addition of "assignee" field looks good.The "assignee" field is added with a value of null, which is consistent with the rest of the data structure.
135-135: Addition of "assignee" field looks good.The "assignee" field is added with a value of null, which is consistent with the rest of the data structure.
167-167: Addition of "assignee" field looks good.The "assignee" field is added with a value of null, which is consistent with the rest of the data structure.
cvat-core/src/server-response-types.ts (1)
299-299: Addition of "assignee" field looks good.The "assignee" field is added to the
SerializedQualityReportDatainterface, which is consistent with the rest of the data structure and follows TypeScript best practices.cvat/apps/quality_control/views.py (1)
314-316: Serialization of "assignee" field looks good.The added lines correctly serialize the
assigneefield in thecreatemethod of theQualityReportViewSetclass, following Django best practices.tests/python/shared/assets/jobs.json (1)
8-8: Addition of "assignee_updated_date" field looks good.The "assignee_updated_date" field is added with a value of null, which is consistent with the rest of the data structure.
Also applies to: 41-41, 74-74, 107-107, 140-140, 173-173, 206-206, 247-247, 288-288, 329-329, 370-370, 411-411, 444-444, 485-485, 526-526, 565-565, 606-606, 639-639, 672-672, 711-711, 750-750, 783-783, 816-816, 855-855, 894-894
cvat/apps/engine/models.py (4)
334-335: Approved: Addedassignee_updated_datefield toProjectclass.The addition of the
assignee_updated_datefield is correctly implemented as aDateTimeFieldwithnull=True,blank=True, anddefault=None.
407-407: Approved: Addedassignee_updated_datefield toTaskclass.The addition of the
assignee_updated_datefield is correctly implemented as aDateTimeFieldwithnull=True,blank=True, anddefault=None.
670-670: Approved: Addedassignee_updated_datefield toJobclass.The addition of the
assignee_updated_datefield is correctly implemented as aDateTimeFieldwithnull=True,blank=True, anddefault=None.
714-714: Approved: Updatedget_task_idmethod inJobclass.The
get_task_idmethod now correctly returnsself.segment.task_id.tests/python/rest_api/test_projects.py (2)
498-515: Approved: Added test for project creation with assignee.The test correctly verifies the
assigneeandassignee_updated_datefields during project creation.
1170-1198: Approved: Added test for updatingassignee_updated_dateon assignee updates.The test correctly verifies the behavior of the
assignee_updated_datefield when the assignee is updated.cvat/apps/quality_control/quality_reports.py (1)
Line range hint
2490-2508:
LGTM!The changes correctly integrate the serialized assignee data into the report.
cvat/apps/engine/serializers.py (9)
595-595: LGTM! The new fieldassignee_updated_dateis correctly added.The addition of the
assignee_updated_datefield is appropriate and aligns with the rest of the serializer.
752-762: LGTM! Theassignee_updated_datefield is correctly handled during job creation.The field is correctly set to the job's updated date when an assignee is assigned.
781-785: LGTM! Theassignee_updated_datefield is correctly handled during job updates.The field is correctly updated to the current time when the assignee changes.
1114-1114: LGTM! The new fieldassignee_updated_dateis correctly added.The addition of the
assignee_updated_datefield is appropriate and aligns with the rest of the serializer.
1198-1200: LGTM! Theassignee_updated_datefield is correctly handled during task creation.The field is correctly set to the task's updated date when an assignee is assigned.
1213-1219: LGTM! Theassignee_updated_datefield is correctly handled during task updates.The field is correctly updated to the current time when the assignee changes.
1345-1345: LGTM! The new fieldassignee_updated_dateis correctly added.The addition of the
assignee_updated_datefield is appropriate and aligns with the rest of the serializer.
1413-1416: LGTM! Theassignee_updated_datefield is correctly handled during project creation.The field is correctly set to the project's updated date when an assignee is assigned.
1426-1432: LGTM! Theassignee_updated_datefield is correctly handled during project updates.The field is correctly updated to the current time when the assignee changes.
tests/python/rest_api/test_tasks.py (2)
415-432: LGTM!The test function
test_can_create_with_assigneeis well-structured and covers the necessary cases for testing task creation with and without an assignee.
2677-2706: LGTM!The test function
test_can_update_assignee_updated_date_on_assignee_updatesis comprehensive and covers various scenarios for updating the assignee, ensuring theassignee_updated_dateis correctly managed.cvat/schema.yml (4)
8095-8098: Addition ofassignee_updated_datefield inJobReadschema.The new field
assignee_updated_datehas been added to theJobReadschema. This field is of typestring, with formatdate-time, and is nullable. It is correctly defined to track the last update time of the assignee.
9577-9580: Addition ofassignee_updated_datefield inProjectReadschema.The new field
assignee_updated_datehas been added to theProjectReadschema. This field is of typestring, with formatdate-time, and is nullable. It is correctly defined to track the last update time of the assignee.
10443-10446: Addition ofassignee_updated_datefield inTaskReadschema.The new field
assignee_updated_datehas been added to theTaskReadschema. This field is of typestring, with formatdate-time, and is nullable. It is correctly defined to track the last update time of the assignee.
9678-9682: Addition ofassigneefield inQualityReportschema.The new field
assigneehas been added to theQualityReportschema. This field referencesBasicUserand is nullable. It is correctly defined to track the assignee of the quality report.
| def _serialize_assignee(assignee: Optional[User]) -> Optional[dict]: | ||
| if not db_report.assignee: | ||
| return None | ||
|
|
||
| reported_keys = ["id", "username", "first_name", "last_name"] | ||
| assert set(reported_keys).issubset(engine_serializers.BasicUserSerializer.Meta.fields) | ||
| # check that only safe fields are reported | ||
|
|
||
| return {k: getattr(assignee, k) for k in reported_keys} |
There was a problem hiding this comment.
Consider using a serializer for consistency.
While the current implementation is correct, using engine_serializers.BasicUserSerializer directly would ensure consistency with other parts of the codebase.
- def _serialize_assignee(assignee: Optional[User]) -> Optional[dict]:
- if not db_report.assignee:
- return None
- reported_keys = ["id", "username", "first_name", "last_name"]
- assert set(reported_keys).issubset(engine_serializers.BasicUserSerializer.Meta.fields)
- return {k: getattr(assignee, k) for k in reported_keys}
+ def _serialize_assignee(assignee: Optional[User]) -> Optional[dict]:
+ if not assignee:
+ return None
+ return engine_serializers.BasicUserSerializer(assignee).dataCommittable 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 _serialize_assignee(assignee: Optional[User]) -> Optional[dict]: | |
| if not db_report.assignee: | |
| return None | |
| reported_keys = ["id", "username", "first_name", "last_name"] | |
| assert set(reported_keys).issubset(engine_serializers.BasicUserSerializer.Meta.fields) | |
| # check that only safe fields are reported | |
| return {k: getattr(assignee, k) for k in reported_keys} | |
| def _serialize_assignee(assignee: Optional[User]) -> Optional[dict]: | |
| if not assignee: | |
| return None | |
| return engine_serializers.BasicUserSerializer(assignee).data |
| if bool(normal_job["assignee"]) != has_assignee: | ||
| with make_api_client(admin_user) as api_client: | ||
| api_client.jobs_api.partial_update( | ||
| normal_job["id"], | ||
| patched_job_write_request={ | ||
| "assignee": new_assignee["id"] if new_assignee else None | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Use a context manager for updating the job assignee.
Using a context manager ensures that the API client is properly closed after the update.
with make_api_client(admin_user) as api_client:
api_client.jobs_api.partial_update(
normal_job["id"],
patched_job_write_request={
"assignee": new_assignee["id"] if new_assignee else None
},
)| report_data = json.loads(self._test_get_report_data_200(admin_user, job_report["id"]).data) | ||
| assert ( | ||
| DeepDiff( | ||
| ( | ||
| { | ||
| k: v | ||
| for k, v in new_assignee.items() | ||
| if k in ["id", "username", "first_name", "last_name"] | ||
| } | ||
| if new_assignee | ||
| else None | ||
| ), | ||
| report_data["assignee"], | ||
| ) | ||
| == {} | ||
| ) | ||
|
|
There was a problem hiding this comment.
Simplify the assertion for assignee comparison.
The assertion can be simplified by directly comparing the expected and actual assignee data.
expected_assignee = (
{k: v for k, v in new_assignee.items() if k in ["id", "username", "first_name", "last_name"]}
if new_assignee
else None
)
assert DeepDiff(expected_assignee, report_data["assignee"]) == {}| with make_api_client(admin_user) as api_client: | ||
| api_client.jobs_api.partial_update( | ||
| normal_job["id"], | ||
| patched_job_write_request={ | ||
| "assignee": users_by_name[admin_user]["id"] if has_assignee else None | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Use a context manager for updating the job assignee.
Using a context manager ensures that the API client is properly closed after the update.
with make_api_client(admin_user) as api_client:
api_client.jobs_api.partial_update(
normal_job["id"],
patched_job_write_request={
"assignee": users_by_name[admin_user]["id"] if has_assignee else None
},
)
tests/python/rest_api/test_jobs.py
Outdated
| def test_can_create_with_assignee(self, admin_user, tasks, jobs, users_by_name, assignee): | ||
| task = next( | ||
| t | ||
| for t in tasks | ||
| if t["size"] > 0 | ||
| if all(j["type"] != "ground_truth" for j in jobs if j["task_id"] == t["id"]) | ||
| ) | ||
|
|
||
| spec = { | ||
| "task_id": task["id"], | ||
| "type": "ground_truth", | ||
| "frame_selection_method": "random_uniform", | ||
| "frame_count": 1, | ||
| "assignee": users_by_name[assignee]["id"] if assignee else None, | ||
| } | ||
|
|
||
| with make_api_client(admin_user) as api_client: | ||
| (job, _) = api_client.jobs_api.create(job_write_request=spec) | ||
|
|
||
| if assignee: | ||
| assert job.assignee.username == assignee | ||
| assert job.assignee_updated_date | ||
| else: | ||
| assert job.assignee is None | ||
| assert job.assignee_updated_date is None | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper test case coverage.
The test_can_create_with_assignee function tests job creation with and without an assignee. However, it does not cover edge cases such as invalid assignee IDs or missing users_by_name entries. Please add test cases to handle these scenarios.
- Verify behavior when
users_by_namedoes not contain the assignee. - Verify behavior when the assignee ID is invalid.
Analysis chain
Ensure proper test case coverage.
The test case covers job creation with and without an assignee. Verify that it handles edge cases such as invalid assignee IDs or missing users_by_name entries.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the edge cases for job creation with an assignee.
# Test: Ensure the test covers invalid assignee IDs and missing users_by_name entries.
rg --type python $'test_can_create_with_assignee'
Length of output: 79
Script:
#!/bin/bash
# Description: Verify the edge cases for job creation with an assignee.
# Correcting the `rg` command to search for the test case `test_can_create_with_assignee`.
rg 'def test_can_create_with_assignee' --glob '*.py'
Length of output: 423
Script:
#!/bin/bash
# Extract the content of the `test_can_create_with_assignee` function from `tests/python/rest_api/test_jobs.py`.
ast-grep --lang python --pattern $'def test_can_create_with_assignee(self, admin_user, tasks, jobs, users_by_name, assignee):\n $$$' tests/python/rest_api/test_jobs.py
Length of output: 2074
cvat-ui/src/components/analytics-page/task-quality/job-list.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Kirill Lakhov <[email protected]>
zhiltsov-max
changed the title
|
/check |
|
❌ Some checks failed |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8123 +/- ##
===========================================
- Coverage 83.40% 83.40% -0.01%
===========================================
Files 388 388
Lines 41299 41305 +6
Branches 3852 3854 +2
===========================================
+ Hits 34446 34450 +4
- Misses 6853 6855 +2
|
Depends on #8119
Motivation and context
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
New Features
assigneefield to Quality Reports for better user assignment management.Enhancements
assignee_updated_datefield, improving assignment tracking.Bug Fixes
Tests
Documentation
assignee_updated_datefield.