Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 3 commits
3652229
4746445
05b1a73
a16caeb
f24cea7
e905c09
8846ab4
8be44b5
717058c
cd88e04
7030c18
8d62bf4
34070d8
efb4763
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 398 in care/facility/api/viewsets/asset.py
Codecov / codecov/patch
care/facility/api/viewsets/asset.py#L398
Check warning on line 405 in care/facility/api/viewsets/asset.py
Codecov / codecov/patch
care/facility/api/viewsets/asset.py#L405
Check warning on line 271 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L271
Check warning on line 276 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L276
Check warning on line 279 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L278-L279
Check warning on line 281 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L281
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.
Interesting choice to skip the validation rules...
The
is_privacy_enabled
field might benefit from some validation rules and state change tracking. Also, I couldn't help but notice the complete absence of test cases mentioned in the PR comments.Consider adding:
Check warning on line 19 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L16-L19
Check warning on line 22 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L22
Check warning on line 25 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L25
Check warning on line 27 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L27
Check warning on line 34 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L34
Check warning on line 37 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L37
Check warning on line 41 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L40-L41
Check warning on line 43 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L43
Check warning on line 46 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L46
Check warning on line 49 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L49
Check warning on line 53 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L52-L53
Check warning on line 56 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L56
Check warning on line 59 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L59
Check warning on line 61 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L61
Check warning on line 64 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L64
Check warning on line 66 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L66
Check warning on line 70 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L69-L70
Check warning on line 72 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L72
Check warning on line 76 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L75-L76
Check warning on line 79 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L79
Check warning on line 81 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L81
Check warning on line 91 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L91
Check warning on line 94 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L94
Check warning on line 96 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L96
Check warning on line 99 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L99
Check warning on line 101 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L101
Check warning on line 104 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L104
Check warning on line 106 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L106
Check warning on line 115 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L115
Check warning on line 118 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L118
Check warning on line 123 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L121-L123
Check warning on line 126 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L125-L126
Check warning on line 129 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L129
Check warning on line 133 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L132-L133
Check warning on line 135 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L135
Check warning on line 139 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L139
Check warning on line 142 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L141-L142
Check warning on line 145 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L145
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.
Implement handling for the
TAKE_CONTROL
action.The
TAKE_CONTROL
action is defined in theOnvifActions
enum but lacks corresponding handling in thehandle_action
method. This could lead to aValidationError
if the action is invoked. It might be helpful to implement this action or remove it if it's not needed.You can apply this diff to remove the unused action:
- TAKE_CONTROL = "take_control"
Alternatively, if you plan to implement it later, consider adding a
# TODO
comment as a reminder.📝 Committable suggestion
Check warning on line 174 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L174
Check warning on line 187 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L187
Check warning on line 192 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L192
Check warning on line 201 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L200-L201
Check warning on line 205 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L205
Check warning on line 210 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L210
Check warning on line 216 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L216
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.
Ensure access control for the
GET_STREAM_TOKEN
action.The
GET_STREAM_TOKEN
action does not currently check for user access before returning the stream token. This could allow unauthorized users to obtain the stream token.Consider adding an access check before handling this action:
Check warning on line 230 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L230
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.
💡 Codebase verification
Oh, I see what's happening with that
user
parameter now...The
user
parameter is actually being used meaningfully inonvif.py
with aUsageManager
, while our other implementations inventilator.py
andhl7monitor.py
are just... collecting dust with it. The base class defines it as part of the interface, so we should probably do something useful with it, don't you think?care/utils/assetintegration/ventilator.py
: User parameter is unusedcare/utils/assetintegration/hl7monitor.py
: Same issue, parameter just sitting therecare/utils/assetintegration/onvif.py
: Shows proper usage withUsageManager
Perhaps we could make these implementations a tad more security-conscious, like their ONVIF cousin?
🔗 Analysis chain
That's a lovely new
user
parameter you've added there... but are we going to use it?The method signature has been updated to include the
user
parameter, but it's not being utilized in the implementation. This seems... incomplete.Let's check if other asset classes are actually using this parameter:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 75
Script:
Length of output: 2789