Assessment responses API #22772

caleybrock · 2018-05-30T23:44:04Z

Summary

The new assessments tab (behind internal only experiment) requires filtering on assessment and student. The API that exists for assessment responses makes it challenging to do this (its a flat array of individual responses), so I'm creating a similar parallel api, with the intent that the old one will get deleted once we launch.

The object should return something like this:

{
  12: {   <--- a student id
    student_name: "caley",
    responses: {
     4593: <---- a script id referring to an assessment 
       {level_results: [{status: "correct", answer: "A"}], multi_correct: 5, multi_count: 10.......}
    ...other assessments
  }
  ..student ids
}

New redux screenshot:

Where are the tests?

This definitely needs controller tests. I want to do this before the end of the week, but I wanted to have something to unblock Nkiru with on the front end. I added a todo in code for this so that I'm on the hook.

Future thoughts

This does end up being a fairly similar data (although different structure) to section_assessments which is used to get student responses on the current assessments tab.
https://github.com/code-dot-org/code-dot-org/blob/staging/dashboard/app/controllers/api_controller.rb#L409
I plan on not touching section_assessments, and only adding other APIs, so that the current assessments tab remains unchanged while the new one is in development. As we start to work the new API on the front end, I imagine this new API will diverge even more from the one we use in the angular view.

caleybrock · 2018-05-31T00:12:40Z

dashboard/app/controllers/api_controller.rb

+          level_result = {}
+
+          if level_response
+            case level


I thought that some of this makes more sense on a model, but I wasn't sure which one since it has user data and level data. Open to suggestions since this was copied and pasted from the current API.

I actually don't know what the Rails-y answer is for this (@madelynkasula? @aoby?). You're kind of just generating a view on some data based on multiple models, and my instinct would be to delegate that work somehow, and reduce the controller's job to authorization and reading request parameters.

def assessments_responses section = load_section script = load_script(section) render json: AssessmentsResponsesBuilder.new(section, script).responses_by_student end

Again, not sure that's idiomatic Rails.

yep, i believe that's idiomatic rails--"skinny controller, fat model" is usually used to refer to this. even if there are two models in the mix, and moving the logic would mean models having to talk to each other, that can be okay as long as the related logic is isolated as much as possible.

an example of this would be here. a course needs to know if any experiments are enabled for a user (3 models), but that logic is isolated to a single method rather than getting tangled up in other model logic

Agreed. This seems like an awful lot of logic for a controller. Move the complex logic into a model if you can. If it's not appropriate to a single model (which could mean from that model's perspective even if it includes associations, as @madelynkasula suggested), then it can go in a common lib module for standalone code or in a model concern which can be shared between models. When the logic is in the model (or lib) it can also be tested in isolation.

The only difference I'd call out from Brad's suggestion is to use an ActiveModelSerializer instead of a custom builder class to format the response JSON.

islemaster

I understand you're basically pasting in existing code to a new route, so I'm cool with this going in as-is as a starting point. Consider my comments things to consider as you go back, refactor, and add tests for this.

islemaster · 2018-05-31T16:37:01Z

apps/src/templates/sectionAssessments/SectionAssessments.jsx

@@ -19,7 +19,7 @@ class SectionAssessments extends Component {
  static propTypes = {
    // provided by redux
    sectionId: PropTypes.number.isRequired,
-    assessments: PropTypes.array,
+    assessments: PropTypes.object,


It looks like props.assessments is now unused in this component - should we remove it entirely? Or is this a temporary state?

... and if we do leave it in (for future usage), should we define a shape here rather than object?

This entire component is temporary - Nkiru and I will be hooking up tables based on this API tomorrow. At that time, we'll define a set of PropShapes that we can use throughout the assessments tab.

islemaster · 2018-05-31T16:44:45Z

dashboard/config/routes.rb

  get '/dashboardapi/section_assessments/:section_id', to: 'api#section_assessments'
+  # Used in react assessments tab
+  get '/dashboardapi/section_assessments_responses/:section_id', to: 'api#assessments_responses'


👍 Helpful comments here.

islemaster · 2018-05-31T16:47:45Z

dashboard/app/controllers/api_controller.rb

+  # Currently, very similar logic to section_assessments, but will change as we refactor. The format
+  # of the results is different enough that I'm duplicating some code it for now.
+  # TODO(caleybrock): currently only used in internal experiment, must add controller tests.
+  def assessments_responses


A couple things that would be helpful to add to your documentation of this method:

The route that it implements (i.e. GET /dashboardapi/section_assessments_responses/:section_id)

Sample output - you give a paragraph-form description of the return value here but something more diagrammatic might be helpful. Then again, that will be likely to go out-of-date soon enough, and tests would be better documentation; maybe hold off on this until tests are written?

islemaster · 2018-05-31T16:54:09Z

dashboard/app/controllers/api_controller.rb

+          level_result = {}
+
+          if level_response
+            case level


I actually don't know what the Rails-y answer is for this (@madelynkasula? @aoby?). You're kind of just generating a view on some data based on multiple models, and my instinct would be to delegate that work somehow, and reduce the controller's job to authorization and reading request parameters.

def assessments_responses section = load_section script = load_script(section) render json: AssessmentsResponsesBuilder.new(section, script).responses_by_student end

Again, not sure that's idiomatic Rails.

islemaster · 2018-05-31T16:54:52Z

dashboard/app/controllers/api_controller.rb

+      }
+      responses_by_level_group = {}
+
+      level_group_script_levels.each do |script_level|


This method is huge! I'd take each of these subsections where you're building up a hash as a potential method extraction point.

islemaster · 2018-05-31T16:55:08Z

dashboard/app/controllers/api_controller.rb

+              student_result = level_response["result"].split(",").sort.join(",")
+
+              # Convert "0,1,3" to "A, B, D" for teacher-friendly viewing
+              level_result[:student_result] = student_result.split(',').map {|k| Multi.value_to_letter(k.to_i)}.join(', ')


This line is probably a method unto itself. 😁

islemaster · 2018-05-31T16:56:05Z

dashboard/app/controllers/api_controller.rb

+  # Currently, very similar logic to section_assessments, but will change as we refactor. The format
+  # of the results is different enough that I'm duplicating some code it for now.
+  # TODO(caleybrock): currently only used in internal experiment, must add controller tests.
+  def assessments_responses


Is ApiController really the right place for this? It seems like it's become a grab-bag of barely-related routes. Maybe not necessary to fix in this PR, but worth considering a reorganization soon.

I agree - probably worth me discussing this topic with someone. I've been adding new things here because their old versions were already here, but it feels messy.

caleybrock · 2018-05-31T21:35:31Z

All great feedback! Thanks everyone.

I chatted offline with Andrew and have a good idea of a plan to make this a lot better, but since most of this code isn't new, I'm going to do what Brad mentioned and merge now to unblock front-end work. I'll get started on a re-factor this afternoon.

caleybrock added 5 commits May 30, 2018 15:15

Duplicate controller logic into a new end point and use on front end

5aac589

Instead of an array create a hash

a026c1f

Add a todo for testing

a9d86b8

Only return students who have submitted responses

be24531

Add additional todos and comments

5316a59

caleybrock force-pushed the new-responses-api branch from a6faf87 to 5316a59 Compare May 31, 2018 00:01

caleybrock commented May 31, 2018

View reviewed changes

caleybrock changed the title ~~Assessment responses API - WIP~~ Assessment responses API May 31, 2018

caleybrock requested review from islemaster, breville and nkiruka May 31, 2018 00:15

caleybrock added the teacher-dashboard label May 31, 2018

Don't duplicate student id

14d5a16

islemaster approved these changes May 31, 2018

View reviewed changes

caleybrock merged commit a753fd2 into staging May 31, 2018

caleybrock deleted the new-responses-api branch May 31, 2018 21:36

caleybrock mentioned this pull request May 31, 2018

Move assessments out of api_controller #22816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assessment responses API #22772

Assessment responses API #22772

caleybrock commented May 30, 2018 •

edited

caleybrock May 31, 2018

islemaster May 31, 2018

maddiedierker May 31, 2018

aoby May 31, 2018

aoby May 31, 2018

islemaster left a comment

islemaster May 31, 2018

aoby May 31, 2018

caleybrock May 31, 2018

islemaster May 31, 2018

islemaster May 31, 2018

islemaster May 31, 2018

islemaster May 31, 2018

islemaster May 31, 2018

islemaster May 31, 2018

caleybrock May 31, 2018

caleybrock commented May 31, 2018

Assessment responses API #22772

Assessment responses API #22772

Conversation

caleybrock commented May 30, 2018 • edited

Summary

Where are the tests?

Future thoughts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleybrock commented May 31, 2018

caleybrock commented May 30, 2018 •

edited