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

Move assessments out of api_controller #22816

Merged
merged 3 commits into from Jun 1, 2018

Conversation

caleybrock
Copy link
Contributor

@caleybrock caleybrock commented May 31, 2018

What's in this PR?

Going for incremental improvements on the assessments APIs after getting some good feedback in this PR: #22772

This one moves the methods out of api_controller into a new assessments_controller and creates a resource for assessments. I've updated the front end with the necessary changes. These changes were suggested here: #22772 (comment)

I've also updated the comments for both of these methods as suggestion here: #22772 (comment)

What's not in this PR?

I didn't change any of the actual method code at all (I know there are lots of improvements to make here, but this PR is just organizational).

What's next?

Move the bulk of the logic in these methods to a model (maybe level_source) or just in a separate module in lib and add tests for it.

@@ -95,6 +95,15 @@ module OPS
end
end

# Used in react assessments tab
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love feedback about these routes changes in particular. This works, but I haven't written a lot of this code so I'm not sure if this is a reasonable way to go about things.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great :)

Granted if it's only used in one place it doesn't need to be specified in a concern, though also there's no harm, just an extra level of indirection. Presumably we'll want to expose this in /api/v1 as well, at which point the concern will be handy?

section = Section.find(params[:section_id])
authorize! :read, section
section
end
Copy link
Contributor

Choose a reason for hiding this comment

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

When it comes time to refactor, this looks like a perfect candidate for CanCan's load_and_authorize_resource

@@ -95,6 +95,15 @@ module OPS
end
end

# Used in react assessments tab
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great :)

Granted if it's only used in one place it doesn't need to be specified in a concern, though also there's no harm, just an extra level of indirection. Presumably we'll want to expose this in /api/v1 as well, at which point the concern will be handy?

Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

🎉 i love this change!

class Api::V1::AssessmentsController < Api::V1::JsonApiController
include LevelsHelper

# For each assessment in a script, return an object of script_level IDs to question data.
Copy link
Contributor

Choose a reason for hiding this comment

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

these are great comments! 👍

@caleybrock caleybrock merged commit d47ebf0 into staging Jun 1, 2018
@caleybrock caleybrock deleted the refactor-assessments-controller branch June 1, 2018 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants