Deseng-463:Poll Widget: Back-end #2363
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2363 +/- ##
==========================================
+ Coverage 71.96% 72.44% +0.48%
==========================================
Files 488 497 +9
Lines 16045 16395 +350
Branches 1230 1230
==========================================
+ Hits 11546 11877 +331
- Misses 4273 4292 +19
Partials 226 226
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Great work so far! I'd like to discuss a few things before proceeding:
- Can you let me know how the IP comparison is happening? I'm not seeing where the IP address of the current response is being compared to all other unhashed IP fragments. Is that functionality coming in a future PR?
- I don't think we need a method to be able to update poll responses. Let me know if I'm missing something
- I just want to make sure we're only allowing one response from a user for a given poll ID, not 10. I may be missing something though
| return str(err), err.status_code | ||
|
|
||
| @staticmethod | ||
| def validate_request_format(data): |
There was a problem hiding this comment.
Can I ask why you went with a dedicated method for the schema validation?
| @cors_preflight('POST') | ||
| @API.route('/<int:poll_widget_id>/responses') | ||
| class PollResponseRecord(Resource): | ||
| """Resource for recording responses for a poll widget. Not require authentication.""" |
There was a problem hiding this comment.
| """Resource for recording responses for a poll widget. Not require authentication.""" | |
| """Resource for recording responses for a poll widget. Does not require authentication.""" |
|
|
||
| @staticmethod | ||
| def is_poll_limit_exceeded(poll_id, participant_id): | ||
| """Check poll limt execeeded or not.""" |
There was a problem hiding this comment.
| """Check poll limt execeeded or not.""" | |
| """Check poll limit execeeded or not.""" |
| @staticmethod | ||
| def is_poll_limit_exceeded(poll_id, participant_id): | ||
| """Check poll limt execeeded or not.""" | ||
| return WidgetPollService.check_already_polled(poll_id, participant_id, 10) |
There was a problem hiding this comment.
Does this mean that we're checking if a user has polled more than 10 times? We should be only allowing for a single response to a given poll
| raise BusinessException(str(e), HTTPStatus.INTERNAL_SERVER_ERROR) from e | ||
|
|
||
| @staticmethod | ||
| def delete_poll_answers(poll_id: int): |
There was a problem hiding this comment.
If the PO thinks we should allow polls to be changed while they're being voted on, then we may need to do a check to see if answers have been responded to and prevent deletion if so.
| """ | ||
| This module provides utility functions for handling IP addresses in a Flask application. | ||
|
|
||
| It includes a function for hashing IP addresses using SHA256, ensuring secure and consistent |
There was a problem hiding this comment.
Great comments at the top of this file!
| str: The resulting SHA256 hash as a hexadecimal string. | ||
| """ | ||
| # Retrieve the secret key from Flask configuration with a fallback empty string | ||
| secret_key = current_app.config.get('SECRET_KEY', '') |
There was a problem hiding this comment.
We may want to change the naming of this environment variable to something more intuitive. Maybe IP_HASH_KEY ?
| PollResponse.is_deleted == false()).all() | ||
|
|
||
| @classmethod | ||
| def update_response(cls, response_id, response_data: dict) -> PollResponse: |
There was a problem hiding this comment.
I don't think there will be a need to update user responses. Once a user votes, they can't change it. I don't think we need this method
met-api/src/met_api/utils/ip_util.py
Outdated
| secret_key = current_app.config.get('SECRET_KEY', '') | ||
|
|
||
| # Concatenate the IP address and secret key, and hash the resulting string | ||
| return sha256(f'{ip_address}{secret_key}'.encode()).hexdigest() |
There was a problem hiding this comment.
This hashes the IP, but where is the un-hash or IP comparison method? Is that going to come later?
|
Thank you for the review, @Baelx . Quick summary based on our discussion: We're hashing IPs with our secret key and matching them against the database to flag any hash polled over 10 times. This threshold accounts for shared IPs in a network. We decided on the number 10 based on the requirement from the ticket. We plan to add an 'is_deleted' metadata field to poll responses for better handling of suspicious entries, as per the epic ticket's requirements and thats why we had the update function to Poll Response. I've corrected the typos you mentioned and included the missing code for IP fragmentation before hashing. The SECRET_KEY we're using is from the existing Flask configuration, not a new variable. Regarding the logic to prevent admin users from editing/removing poll answers post-publication, I'll include that in the upcoming PR for the Poll widget UI. This was confirmed post the current PR. Appreciate your feedback and support. |
|
Issue #: https://apps.itsm.gov.bc.ca/jira/browse/DESENG-463
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).