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 5 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 273 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L273
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 284 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L284
Check warning on line 287 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L286-L287
Check warning on line 289 in care/facility/api/viewsets/bed.py
Codecov / codecov/patch
care/facility/api/viewsets/bed.py#L289
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:
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 39 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L39
Check warning on line 52 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L52
Check warning on line 57 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L57
Check warning on line 66 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L65-L66
Check warning on line 70 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L70
Check warning on line 75 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L75
Check warning on line 81 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L81
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 95 in care/utils/assetintegration/onvif.py
Codecov / codecov/patch
care/utils/assetintegration/onvif.py#L95
Check warning on line 13 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L10-L13
Check warning on line 16 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L16
Check warning on line 18 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L18
Check warning on line 21 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L21
Check warning on line 24 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L24
Check warning on line 28 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L27-L28
Check warning on line 30 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L30
Check warning on line 33 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L33
Check warning on line 36 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L36
Check warning on line 40 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L39-L40
Check warning on line 43 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L43
Check warning on line 46 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L46
Check warning on line 48 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L48
Check warning on line 51 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L51
Check warning on line 53 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L53
Check warning on line 57 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L56-L57
Check warning on line 59 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L59
Check warning on line 63 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L62-L63
Check warning on line 66 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L66
Check warning on line 68 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L68
Check warning on line 78 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L78
Check warning on line 81 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L81
Check warning on line 83 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L83
Check warning on line 86 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L86
Check warning on line 88 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L88
Check warning on line 91 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L91
Check warning on line 93 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L93
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.
Sanitize user input in notification messages.
Directly inserting user-provided data into messages can lead to security vulnerabilities. It's safer to sanitize or escape user inputs.
Consider updating the message construction:
Ensure all user attributes are properly sanitized.
Check warning on line 102 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L102
Check warning on line 105 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L105
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.
This is not how we implement locks in care, refer to other lock implementations
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.
@vigneshhari This implementation was based on the existing implementation of locks in discharge summary (https://github.com/ohcnetwork/care/blob/develop/care/facility/utils/reports/discharge_summary.py#L40)
Check warning on line 110 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L108-L110
Check warning on line 113 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L112-L113
Check warning on line 116 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L116
Check warning on line 120 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L119-L120
Check warning on line 122 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L122
Check warning on line 126 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L126
Check warning on line 129 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L128-L129
Check warning on line 132 in care/utils/assetintegration/usage_manager.py
Codecov / codecov/patch
care/utils/assetintegration/usage_manager.py#L132
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