-
Notifications
You must be signed in to change notification settings - Fork 325
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
Privacy consultation bed #2506
base: develop
Are you sure you want to change the base?
Privacy consultation bed #2506
Changes from all commits
3652229
4746445
05b1a73
a16caeb
f24cea7
e905c09
8846ab4
8be44b5
717058c
cd88e04
7030c18
8d62bf4
34070d8
efb4763
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,6 +5,7 @@ | |||||||||||||||||||||
from drf_spectacular.utils import extend_schema, extend_schema_view | ||||||||||||||||||||||
from rest_framework import filters as drf_filters | ||||||||||||||||||||||
from rest_framework import status | ||||||||||||||||||||||
from rest_framework.decorators import action | ||||||||||||||||||||||
from rest_framework.exceptions import PermissionDenied | ||||||||||||||||||||||
from rest_framework.exceptions import ValidationError as DRFValidationError | ||||||||||||||||||||||
from rest_framework.fields import get_error_detail | ||||||||||||||||||||||
|
@@ -230,3 +231,30 @@ def get_queryset(self): | |||||||||||||||||||||
allowed_facilities = get_accessible_facilities(user) | ||||||||||||||||||||||
queryset = queryset.filter(bed__facility__id__in=allowed_facilities) | ||||||||||||||||||||||
return queryset | ||||||||||||||||||||||
|
||||||||||||||||||||||
@action(detail=True, methods=["patch"]) | ||||||||||||||||||||||
def set_privacy(self, request, external_id): | ||||||||||||||||||||||
consultation_bed: ConsultationBed = self.get_object() | ||||||||||||||||||||||
|
||||||||||||||||||||||
is_privacy_enabled = request.data.get("is_privacy_enabled") | ||||||||||||||||||||||
|
||||||||||||||||||||||
if is_privacy_enabled is None and not isinstance(is_privacy_enabled, bool): | ||||||||||||||||||||||
return Response( | ||||||||||||||||||||||
{"detail": "is_privacy_enabled is required and should be a boolean"}, | ||||||||||||||||||||||
status=status.HTTP_400_BAD_REQUEST, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
Comment on lines
+241
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve the validation condition for Interesting approach on the condition here. However, it might not catch all invalid inputs. It would be better to check whether Apply this diff to correct the condition: - if is_privacy_enabled is None and not isinstance(is_privacy_enabled, bool):
+ if is_privacy_enabled is None or not isinstance(is_privacy_enabled, bool): 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
if not consultation_bed.consultation.patient.has_object_update_permission( | ||||||||||||||||||||||
khavinshankar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
request | ||||||||||||||||||||||
) or request.user.user_type in [ | ||||||||||||||||||||||
User.TYPE_VALUE_MAP["DistrictLabAdmin"], | ||||||||||||||||||||||
User.TYPE_VALUE_MAP["StateLabAdmin"], | ||||||||||||||||||||||
]: | ||||||||||||||||||||||
raise PermissionDenied | ||||||||||||||||||||||
|
||||||||||||||||||||||
consultation_bed.is_privacy_enabled = is_privacy_enabled | ||||||||||||||||||||||
consultation_bed.save(update_fields=["is_privacy_enabled"]) | ||||||||||||||||||||||
|
||||||||||||||||||||||
return Response( | ||||||||||||||||||||||
ConsultationBedSerializer(consultation_bed).data, status=status.HTTP_200_OK | ||||||||||||||||||||||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Generated by Django 5.1.1 on 2024-09-28 17:40 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('facility', '0466_camera_presets'), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name='consultationbed', | ||
name='is_privacy_enabled', | ||
field=models.BooleanField(default=False), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,7 @@ class ConsultationBed(BaseModel): | |
bed = models.ForeignKey(Bed, on_delete=models.PROTECT, null=False, blank=False) | ||
start_date = models.DateTimeField(null=False, blank=False) | ||
end_date = models.DateTimeField(null=True, blank=True, default=None) | ||
is_privacy_enabled = models.BooleanField(default=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting choice to skip the validation rules... The Consider adding:
class ConsultationBed(BaseModel):
+ def clean(self):
+ super().clean()
+ if self.is_privacy_enabled and not self.consultation.facility.privacy_enabled:
+ raise ValidationError(
+ {"is_privacy_enabled": "Facility must have privacy features enabled"}
+ )
+
+ def save(self, *args, **kwargs):
+ self.full_clean()
+ if self.tracker.has_changed('is_privacy_enabled'):
+ # Log privacy state change
+ pass
+ super().save(*args, **kwargs)
|
||
meta = JSONField(default=dict, blank=True) | ||
assets = models.ManyToManyField( | ||
Asset, through="ConsultationBedAsset", related_name="assigned_consultation_beds" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
from unittest import mock | ||
|
||
from rest_framework.test import APITestCase | ||
|
||
from care.users.models import User | ||
from care.utils.assetintegration.usage_manager import UsageManager | ||
from care.utils.tests.test_utils import TestUtils | ||
|
||
|
||
class UsageManagerTestCase(TestUtils, APITestCase): | ||
@classmethod | ||
def setUpTestData(cls): | ||
cls.state = cls.create_state() | ||
cls.district = cls.create_district(cls.state) | ||
cls.local_body = cls.create_local_body(cls.district) | ||
cls.super_user = cls.create_super_user("su", cls.district) | ||
cls.facility = cls.create_facility(cls.super_user, cls.district, cls.local_body) | ||
cls.asset_location = cls.create_asset_location(cls.facility) | ||
cls.asset = cls.create_asset(cls.asset_location) | ||
|
||
def setUp(self): | ||
self.user1 = self.create_user( | ||
username="test_user_1", | ||
state=self.state, | ||
district=self.district, | ||
user_type=User.TYPE_VALUE_MAP["StateAdmin"], | ||
) | ||
self.user2 = self.create_user( | ||
username="test_user_2", | ||
state=self.state, | ||
district=self.district, | ||
user_type=User.TYPE_VALUE_MAP["StateAdmin"], | ||
) | ||
|
||
self.mock_cache = mock.MagicMock() | ||
self.cache_patcher = mock.patch( | ||
"care.utils.assetintegration.usage_manager.cache", self.mock_cache | ||
) | ||
self.cache_patcher.start() | ||
|
||
self.usage_manager_user1 = UsageManager( | ||
asset_id=self.asset.external_id, user=self.user1 | ||
) | ||
self.usage_manager_user2 = UsageManager( | ||
asset_id=self.asset.external_id, user=self.user2 | ||
) | ||
|
||
self.mock_redis_client = mock.MagicMock() | ||
self.usage_manager_user1.redis_client = self.mock_redis_client | ||
self.usage_manager_user2.redis_client = self.mock_redis_client | ||
|
||
def tearDown(self): | ||
self.cache_patcher.stop() | ||
|
||
def test_has_access(self): | ||
self.mock_cache.get.return_value = None | ||
self.assertTrue(self.usage_manager_user1.has_access()) | ||
|
||
self.mock_cache.get.return_value = self.user1.id | ||
self.assertTrue(self.usage_manager_user1.has_access()) | ||
|
||
self.mock_cache.get.return_value = self.user2.id | ||
self.assertFalse(self.usage_manager_user1.has_access()) | ||
|
||
def test_unlock_camera(self): | ||
self.mock_cache.get.return_value = self.user1.id | ||
|
||
with mock.patch.object( | ||
self.usage_manager_user1, "notify_waiting_list_on_asset_availabe" | ||
) as mock_notify: | ||
self.usage_manager_user1.unlock_camera() | ||
|
||
self.mock_cache.delete.assert_called_once_with( | ||
self.usage_manager_user1.current_user_cache_key | ||
) | ||
|
||
mock_notify.assert_called_once() | ||
|
||
def test_request_access(self): | ||
self.mock_cache.get.return_value = None | ||
self.assertTrue(self.usage_manager_user1.request_access()) | ||
|
||
self.mock_cache.get.return_value = self.user2.id | ||
with mock.patch( | ||
"care.utils.notification_handler.send_webpush" | ||
) as mock_send_webpush: | ||
result = self.usage_manager_user1.request_access() | ||
self.assertFalse(result) | ||
mock_send_webpush.assert_called_once() | ||
|
||
def test_lock_camera(self): | ||
self.mock_cache.get.return_value = None | ||
self.assertTrue(self.usage_manager_user1.lock_camera()) | ||
self.mock_cache.set.assert_called_once_with( | ||
self.usage_manager_user1.current_user_cache_key, | ||
self.user1.id, | ||
timeout=60 * 5, | ||
) | ||
|
||
self.mock_cache.get.return_value = self.user2.id | ||
self.assertFalse(self.usage_manager_user1.lock_camera()) | ||
|
||
def test_current_user(self): | ||
self.mock_cache.get.return_value = self.user1.id | ||
|
||
mock_serializer = mock.MagicMock() | ||
mock_serializer.data = { | ||
"id": self.user1.id, | ||
"username": self.user1.username, | ||
} | ||
|
||
with mock.patch( | ||
"care.facility.api.serializers.asset.UserBaseMinimumSerializer", | ||
return_value=mock_serializer, | ||
): | ||
current_user_data = self.usage_manager_user1.current_user() | ||
self.assertIsNotNone(current_user_data) | ||
self.assertEqual(current_user_data["id"], self.user1.id) | ||
Comment on lines
+55
to
+118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion The happy path tests look great! Though I can't help but notice some missing scenarios. Consider adding tests for:
I'm sure these edge cases aren't important though... until they are. Here's a sample test case for Redis failure: def test_lock_camera_redis_failure(self):
self.mock_redis_client.ping.side_effect = ConnectionError("Redis connection failed")
with self.assertRaises(ConnectionError):
self.usage_manager_user1.lock_camera()
Comment on lines
+1
to
+118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Several methods seem to have slipped through the testing net While the existing tests are... fine, there appear to be some methods that haven't received the attention they deserve:
I'm sure there's a perfectly good reason these methods don't have corresponding test cases, but it would be absolutely wonderful if we could ensure they work as intended. After all, untested code is like a mystery box - you never know what surprises it might contain. 🔗 Analysis chainThe test suite looks... adequate. While the basic functionality is covered, it would be really nice if we could verify the test coverage percentage. You know, just to make sure we're not missing anything important. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check test coverage for the UsageManager class
# Find the corresponding implementation file
implementation_file=$(fd -t f -e py usage_manager)
# Run coverage report if available
if command -v coverage &> /dev/null; then
coverage run -m pytest "$implementation_file"
coverage report --include="$implementation_file"
else
echo "Coverage tool not found. But I'm sure the coverage is fine... probably."
fi
Length of output: 325 Script: #!/bin/bash
# Let's examine the test coverage by analyzing the implementation and test files
# First, let's see what methods are defined in the implementation
echo "=== Implementation Methods ==="
ast-grep --pattern 'def $_($$$) {$$$}' care/utils/assetintegration/usage_manager.py
echo -e "\n=== Test Methods ==="
ast-grep --pattern 'def test_$_($$$) {$$$}' care/facility/tests/test_asset_usage_manager.py
# Let's also check for any untested public methods
echo -e "\n=== Public Methods in Implementation ==="
rg "^[[:space:]]*def [^_].*\(" care/utils/assetintegration/usage_manager.py
Length of output: 990 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,9 @@ | |
from rest_framework import status | ||
from rest_framework.test import APITestCase | ||
|
||
from care.facility.models import Asset, Bed, ConsultationBedAsset | ||
from care.facility.models import Asset, Bed, ConsultationBedAsset, FacilityUser | ||
from care.facility.models.bed import ConsultationBed | ||
from care.users.models import User | ||
from care.utils.tests.test_utils import TestUtils | ||
|
||
|
||
|
@@ -427,3 +428,77 @@ def test_link_asset_to_consultation_bed_with_asset_already_in_use(self): | |
}, | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
||
def test_set_privacy_endpoint(self): | ||
consultation_bed = ConsultationBed.objects.create( | ||
consultation=self.consultation, | ||
bed=self.bed1, | ||
start_date=timezone.now(), | ||
end_date=timezone.now() + timedelta(days=1), | ||
is_privacy_enabled=False, | ||
) | ||
|
||
response = self.client.patch( | ||
f"/api/v1/consultationbed/{consultation_bed.external_id}/set_privacy/", | ||
{}, | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
||
allowed_user_types = [ | ||
"StateAdmin", | ||
"DistrictAdmin", | ||
"Doctor", | ||
] | ||
for user_type in allowed_user_types: | ||
user = self.create_user( | ||
username="test_user_" + user_type.lower(), | ||
state=self.state, | ||
district=self.district, | ||
user_type=User.TYPE_VALUE_MAP[user_type], | ||
home_facility=self.facility, | ||
) | ||
|
||
self.client.force_authenticate(user) | ||
|
||
response = self.client.patch( | ||
f"/api/v1/consultationbed/{consultation_bed.external_id}/set_privacy/", | ||
{"is_privacy_enabled": True}, | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
self.assertTrue( | ||
ConsultationBed.objects.get(id=consultation_bed.id).is_privacy_enabled | ||
) | ||
|
||
response = self.client.patch( | ||
f"/api/v1/consultationbed/{consultation_bed.external_id}/set_privacy/", | ||
{"is_privacy_enabled": False}, | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
self.assertFalse( | ||
ConsultationBed.objects.get(id=consultation_bed.id).is_privacy_enabled | ||
) | ||
|
||
Comment on lines
+447
to
+480
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add test for concurrent privacy modifications The current tests are almost perfect, but they don't verify how the endpoint handles concurrent modifications. This could lead to race conditions in production. Add this test case to check for race conditions: def test_set_privacy_concurrent_modifications(self):
consultation_bed = ConsultationBed.objects.create(
consultation=self.consultation,
bed=self.bed1,
start_date=timezone.now(),
is_privacy_enabled=False,
)
# Simulate concurrent modifications
bed1 = ConsultationBed.objects.get(id=consultation_bed.id)
bed2 = ConsultationBed.objects.get(id=consultation_bed.id)
bed1.is_privacy_enabled = True
bed1.save()
bed2.is_privacy_enabled = False
bed2.save()
# Verify final state
final_state = ConsultationBed.objects.get(id=consultation_bed.id)
self.assertFalse(final_state.is_privacy_enabled) |
||
not_allowed_user_types = [ | ||
user_type | ||
for user_type in User.TYPE_VALUE_MAP | ||
if user_type not in allowed_user_types | ||
] | ||
for user_type in not_allowed_user_types: | ||
user = self.create_user( | ||
username="test_user_" + user_type.lower(), | ||
district=self.district, | ||
user_type=User.TYPE_VALUE_MAP[user_type], | ||
) | ||
FacilityUser.objects.update_or_create( | ||
user=user, | ||
facility=self.facility, | ||
defaults={"created_by": self.super_user}, | ||
) | ||
|
||
self.client.force_authenticate(user) | ||
|
||
response = self.client.patch( | ||
f"/api/v1/consultationbed/{consultation_bed.external_id}/set_privacy/", | ||
{"is_privacy_enabled": True}, | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential issue with unpacking
e.detail
Unpacking
e.detail
using{**e.detail}
assumes it's a dictionary, bute.detail
might be a string. This could raise aTypeError
. You might want to verify the type ofe.detail
and adjust accordingly to ensure proper error handling.Apply this diff to fix the issue:
📝 Committable suggestion