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

Update /v2/sections/valid_scripts dependencies #22669

Merged
merged 7 commits into from May 31, 2018

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented May 24, 2018

This PR is part of the work to move the /v2/sections/valid_scripts endpoint to dashboard. See #22507 and #22655 for more context.

What it does

  • Moves teacherSectionsRedux (including tests and storybook dependencies) to the new endpoint at dashboardapi/sections/valid_scripts
  • Updates the new endpoint to return scripts rather than nested {scripts: scripts} JSON for more straightforward consumption on the frontend

Follow-up Work

  • Finish removing dependencies (as pointed out by @davidsbailey on Move /sections/valid_scripts from pegasus to dashboard #22507). All of these areas will either be fully deprecated by future pegasus-to-dashboard endpoints being moved or the work I will begin soon to move the remainder of teacher dashboard from angular to react, so I'm leaving them for now
  • Deprecate /v2/sections/valid_scripts and log a notification to Honeybadger once all known dependencies have been removed

@@ -115,7 +115,7 @@ def valid_scripts
return head :forbidden unless current_user

scripts = Script.valid_scripts(current_user).map(&:assignable_info)
render json: {scripts: scripts}
render json: scripts
Copy link
Member

Choose a reason for hiding this comment

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

please double check that all of your integers in this json are coming back as integers and not strings. for some reason, I ran into a problem when rendering an array as json (like you have it here), rather than rendering an object (like it was before). past discussion for a bit of context: #22176 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the heads up! i double checked my response and the integers look good (image from the frontend below), but i'll keep an eye out for any weirdness with that

screen shot 2018-05-24 at 2 52 33 pm

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.

Exciting! LGTM after considering the json rendering issue. Presumably you won't be able to merge this until after the other PRs go in and the i18n sync happens on Monday.

@maddiedierker maddiedierker merged commit 57e16fd into staging May 31, 2018
@maddiedierker maddiedierker deleted the valid-scripts-dependencies branch May 31, 2018 20:16
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