Skip to content
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

Handle empty feedback #23554

Merged
merged 6 commits into from Jul 9, 2018
Merged

Handle empty feedback #23554

merged 6 commits into from Jul 9, 2018

Conversation

epeach
Copy link

@epeach epeach commented Jul 6, 2018

Small PR that breaks out the additional edge case handling for when the teacher_feedbacks controller tries to get feedback when there is none.

[Model] The latest scope returns nil when there is no feedback.
[Model] (No Change) The 'latest_by_teacher' scope returns [] when there is no feedback.
[Controller] The (singular) get_feedback_from_teacher returns no_content when there is no feedback.
[Controller] (No Change) The (collection) get_feedbacks returns [] when there is no feedback.

Tests included.

Note: I made no change to the way the empty feedback is handled for the collection (latest_by_teacher and get_feedbacks), since the [] makes sense in this case and allows the UI to easy handle displaying nothing.

@epeach epeach requested a review from aoby July 6, 2018 20:52
sign_in @teacher
get "#{API}/get_feedback_from_teacher", params: {student_id: @student.id, level_id: @level.id, teacher_id: @teacher.id}

assert_response 204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe :no_content here instead of 204 for consistency? Not a big deal either way

@epeach epeach merged commit 1a90c71 into staging Jul 9, 2018
@epeach epeach deleted the handle-empty-feedback branch July 9, 2018 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants