-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: request error for empty user list responses #291
Conversation
Thanks for the pull request, @DmytroAlipov! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
1 similar comment
Thanks for the pull request, @DmytroAlipov! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #291 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 109 109
Lines 1078 1079 +1
Branches 159 160 +1
=========================================
+ Hits 1078 1079 +1 ☔ View full report in Codecov by Sentry. |
Hi @openedx/content-aurora! This is ready for review. |
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.
While this solves the error message, it doesn't fix the problem completely: there's still no user feedback when the button is pressed with an empty list of results.
I believe a better fix would be to show a message in the ShowReview modal to the effect that no responses were selected. Is this something you can take on?
Or, for a possibly an easier fix, to grey out the |
Hi, @arbrandes! |
d1adcb7
to
3302aae
Compare
@arbrandes I made the button inactive. Now you can't send a request if the list is empty. |
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.
Brilliant! Not to be too picky, but do you mind adding a unit test for when the button should be disabled (and when it shouldn't)?
@arbrandes Now this is implemented as a snapshot (button enabled). If I make a test also based on a snapshot with only the button disabled, will such a test be ok? |
3302aae
to
20cd6bc
Compare
@arbrandes I added a test😉 |
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.
Alright, looks good, thank you for bearing with me!
@DmytroAlipov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description:
"An unexpected error occurred" if you try to get all responses when the users' list is empty:
I suggest not sending a request if the users are not on the list.