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

Revert "Revert "Update PATCH /sections/:id in dashboard, deprecate pegasus endpoint"" #22951

Merged

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Jun 7, 2018

Reverts #22949

The original PR (#22902) that updated PATCH /sections/:id went out with yesterday's deploy with a bug, so Dave did a late deploy to help me revert the original PR. This PR reverts that revert and fixes the bug!

I added "new" comments to the new parts so you can see the changes that differ from the original PR.

Background

PATCH /sections/:id expected to receive params[:script_id], which is what the previous consumer supplied, but the new consumer (teacherSectionsRedux) currently (but not for long...) supplies params[:script][:id], which meant scripts could not be updated.

I added a unit test to catch this case, and will push a follow-up PR with a feature test to cover updating sections as well (once #22955 has been merged, which adds similar feature test coverage)

# This endpoint needs to satisfy two endpoint formats for getting script_id
# This should be updated soon to always expect params[:script_id]
script_id = params[:script][:id] if params[:script]
script_id ||= params[:script_id]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new - fixes bug where one consumer provides params[:script_id] and the other provides params[:script][:id]

assert_response :success
section.reload
assert_equal(@script_in_course.id, section.script_id)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new - tests case where one consumer provides params[:script_id] and the other provides params[:script][:id]

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for covering with a regression test.

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Looks good!

@maddiedierker maddiedierker merged commit adae0f0 into staging Jun 7, 2018
@maddiedierker maddiedierker deleted the revert-22949-revert-22902-update-sections-dashboard branch June 7, 2018 17:57
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

3 participants