-
Notifications
You must be signed in to change notification settings - Fork 19
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
Deseng 464 : Poll widget UI #2367
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2367 +/- ##
==========================================
- Coverage 73.22% 72.20% -1.03%
==========================================
Files 498 508 +10
Lines 16753 17123 +370
Branches 1227 1290 +63
==========================================
+ Hits 12267 12363 +96
- Misses 4262 4520 +258
- Partials 224 240 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Looks great! Could I ask for a few small changes:
- Correct variable names that use snake_case
- Fix PollDisplay.tsx map keys. Change from using index to a string containing the index
Let's discuss:
- Creating wrapper methods at the Poll resource API layer. Could you just let me know why those are necessary?
if not valid_format: | ||
errors = schema_utils.serialize(errors) | ||
return valid_format, errors | ||
|
||
@staticmethod |
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.
Just curious why we need this wrapper function around is_poll_engagement_published
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.
Will update the structure as other resource file to avoid confusion
def is_poll_engagement_published(poll_id): | ||
"""Check if engagement of this poll is published or not.""" | ||
return WidgetPollService.is_poll_engagement_published(poll_id) | ||
|
||
@staticmethod | ||
def is_poll_limit_exceeded(poll_id, participant_id): |
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.
I know this is from a previous PR but why do we need a wrapper function for this?
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.
Will update the structure as other resource file to avoid confusion
try: | ||
poll = WidgetPollService.get_poll_by_id(poll_id) | ||
engagement = EngagementService().get_engagement(poll.engagement_id) | ||
pub_val = EngagementStatus.Published.value |
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.
Did you save this to a variable to keep the line length short on line 102?
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.
Yes. as the max length of line by flake8 is 79
if engagement is None: | ||
return False | ||
# Check if the engagement's status matches the published value | ||
return engagement.get('status_id') == pub_val |
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.
return engagement.get('status_id') == pub_val | |
return engagement.get('status_id') == EngagementStatus.Published.value |
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.
the max length of the line by flake8 is 79.
answers_data = poll_data.get('answers', []) | ||
PollAnswerService.create_bulk_poll_answers(poll_id, answers_data) | ||
try: | ||
if 'answers' in poll_data and len(poll_data['answers']) > 0: |
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.
By the looks of this, if a user tried to save a poll with no answers then it would fail silently, correct?
This probably won't happen that often but we should probably notify them that their save was unsuccessful. Does that happen somewhere? Or does the frontend block saving if no answers are provided?
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 function ensures that if there are no changes to the answers in the API, there is no need to update the answers. Especially, while patching status
only when engagement is published.
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.
Got it, it's clear now!
onSubmit(restructuredData); | ||
}; | ||
|
||
const savePollWidget = (data: DetailsForm) => { |
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.
Could be a way to make this function a bit shorter.
const savePollWidget = (data: DetailsForm) => { | |
const savePollWidget = (data: DetailsForm) => !pollWidget ? createPoll(data) : updatePoll(data); |
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.
thanks. will do that
> | ||
{pollWidget.answers.map((answer, index) => ( | ||
<FormControlLabel | ||
key={index} |
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.
key={index} | |
key={'answer' + index} |
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.
Sure !!
|
||
const fetchPollDetails = async () => { | ||
try { | ||
const poll_widgets = await fetchPollWidgets(widget.id); |
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.
It can be confusing going between JS and Python sometimes I know :P
const poll_widgets = await fetchPollWidgets(widget.id); | |
const pollWidgets = await fetchPollWidgets(widget.id); |
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.
True!!, Thanks for noting that.
setInteractionEnabled(true); | ||
} | ||
// Irrespective of the error mark the poll as submitted to avoid re-submitting | ||
setIsSubmitted(true); |
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.
Is this an anti-spam measure? I'm thinking there may be a situation where someone has loaded the app but then loses connection and tries to submit. The server may not respond but their submission would still be counted toward their total
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.
Yes, its for anti-spamming. But as you pointed out we need to consider server unavailability scenario too. Will update the logic like if limit error, disable poll otherwise keep it enabled for re-submission
interactionEnabled={interactionEnabled && !isSubmitted && !isLoggedIn} | ||
onOptionChange={handleOptionChange} | ||
/> | ||
{!isLoggedIn && ( |
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.
Does this prevent users of the backend from voting?
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.
yes, logged in users cannot vote
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 13 New issues |
Issue #: https://apps.itsm.gov.bc.ca/jira/browse/DESENG-464
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).