Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved asset bed relations for camera preset #2387

Merged
merged 24 commits into from
Oct 18, 2024

Conversation

rithviknishad
Copy link
Member

@rithviknishad rithviknishad commented Aug 22, 2024

Proposed Changes

  • Brief of changes made.

Associated Issue

Architecture changes

  • Remove this section if not used

Merge Checklist

  • Tests added/fixed
  • Update docs in /docs
  • Linting Complete
  • Any other necessary step

Only PR's with test cases included and passing lint and test pipelines will be reviewed

@coronasafe/care-backend-maintainers @coronasafe/care-backend-admins

@rithviknishad rithviknishad added the In progress Work in progress label Aug 22, 2024
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 85.15625% with 19 lines in your changes missing coverage. Please review.

Project coverage is 65.98%. Comparing base (0fc5260) to head (1370725).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
care/utils/queryset/asset_bed.py 66.66% 6 Missing and 6 partials ⚠️
care/facility/api/serializers/camera_preset.py 92.85% 2 Missing ⚠️
care/facility/api/viewsets/camera_preset.py 95.00% 1 Missing and 1 partial ⚠️
care/facility/models/bed.py 60.00% 2 Missing ⚠️
care/facility/api/viewsets/bed.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2387      +/-   ##
===========================================
+ Coverage    65.62%   65.98%   +0.36%     
===========================================
  Files          223      227       +4     
  Lines        13398    13482      +84     
  Branches      1856     1856              
===========================================
+ Hits          8792     8896     +104     
+ Misses        4222     4195      -27     
- Partials       384      391       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sainak sainak mentioned this pull request Sep 22, 2024
6 tasks
@gigincg
Copy link
Member

gigincg commented Sep 23, 2024

@rithviknishad Is this good to be tested/reviewed? Can you update the issue?

@rithviknishad rithviknishad marked this pull request as ready for review September 29, 2024 08:05
@rithviknishad rithviknishad requested a review from a team as a code owner September 29, 2024 08:05
@rithviknishad rithviknishad self-assigned this Sep 29, 2024
@rithviknishad rithviknishad removed the In progress Work in progress label Sep 29, 2024
care/facility/api/serializers/camera_preset.py Outdated Show resolved Hide resolved
care/facility/api/serializers/camera_preset.py Outdated Show resolved Hide resolved
care/facility/api/viewsets/camera_preset.py Outdated Show resolved Hide resolved
care/facility/api/viewsets/camera_preset.py Show resolved Hide resolved
rithviknishad and others added 2 commits October 8, 2024 15:13
---------

Co-authored-by: Aakash Singh <mail@singhaakash.dev>
@nihal467
Copy link
Member

LGTM

@vigneshhari
Copy link
Member

This is a very very specific implementation, we should try to avoid creating features like this as much as possible. Care needs more abstract thinking.

AssetBed.objects.filter(id__in=assetbeds_to_delete).update(
deleted=True, meta={}
)
AssetBed.objects.filter(deleted=False, asset__asset_class="ONVIF").update(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrations usually delete data later on, deleting data right away is usually not the best approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove it from this PR and create separate PR to remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The duplicate asset-bed records would have to be marked as deleted for the unique constraint to be applied, and for the front-end to know which asset-bed record to link to when the preset is being created.

We can however skip deleting the meta data from these old records so that data is still preserved, just that duplicate asset-bed link records would no longer be accessible by the viewset.

is_migrated=True,
)
)
if asset_bed.row_number != 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is 1 so special ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to preserve the first asset-bed link since the camera preset would be associated to the asset-bed link. So far each old preset had it's own asset-bed record. However, asset-bed records would be now unique together for asset and bed and the newly created preset record would be linked to this asset bed, hence we need to preserve the first asset-bed.

care/facility/migrations/0466_camera_presets.py Outdated Show resolved Hide resolved
CameraPreset = apps.get_model("facility", "CameraPreset")

paginator = Paginator(
AssetBed.objects.annotate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have that many assetbed realtions in prod right now ?

@vigneshhari
Copy link
Member

Also, i don't see any tests for this feature.

@vigneshhari vigneshhari merged commit 14d3ef9 into develop Oct 18, 2024
7 checks passed
@vigneshhari vigneshhari deleted the rithviknishad/feat/camera-presets branch October 18, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants