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

Deprecate /v2/sections/:section_id/students, update consumer #22740

Merged
merged 3 commits into from May 31, 2018

Conversation

maddiedierker
Copy link
Contributor

GET /dashboardapi/sections/:section_id/students already existed, so this PR:

  • updates the only consumer found (PrintCertificates) to point to dashboard instead of pegasus
  • logs a notification to Honeybadger if this endpoint is hit

@Erin007
Copy link
Contributor

Erin007 commented May 30, 2018

My only concern is that PrintCertificates is used almost exclusively during Hour of Code. Is there any possibility that this route is used in other places, but "seasonally" so we'll miss them with this HB error check?

@@ -38,7 +38,7 @@ class PrintCertificates extends Component {
};

onClickPrintCerts = () => {
$.ajax(`/v2/sections/${this.props.sectionId}/students`).done(result => {
$.ajax(`/dashboardapi/sections/${this.props.sectionId}/students`).done(result => {
const names = result.map(student => student.name);
this.setState({names}, this.submitForm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Any testing for PrintCertificates or anything that confirms this action still works as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently there is this unit test, which mocks the return value for the component, so it doesn't test the endpoint itself. there are also unit tests on GET /dashboardapi/sections/:section_id/students, but no integration tests that i could find for PrintCertificates

Copy link
Member

Choose a reason for hiding this comment

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

+1, please verify PrintCertificates some way before merging (not just in production after)

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.

This seems like a good approach. Given the issue that @Erin007 mentioned, it could be a good idea to check that PrintCertificates works in production after this ships.

There is some documentation at https://github.com/code-dot-org/code-dot-org/blob/staging/docs/account-management-api.md pointing toward the /v2/sections APIs, so it would be nice to update or at least deprecate those at some point.

@Erin007 , to address your other concern of hidden dependencies on this route -- searching the repo for /v2/sections, the only other production code I see referring to it is in apps/src/templates/manageStudents/ShowSecret.jsx, which is pointing to the /update route. While it's possible that someone is constructing a url without explicitly writing /v2/sections, it seems like a best-effort search for these kinds of references is probably a good enough precaution.

@maddiedierker
Copy link
Contributor Author

maddiedierker commented May 30, 2018

@davidsbailey thanks for pointing out the documentation--i'm changing a lot of endpoints in that area, so i'll add a follow-up task to update that.

also, you mentioned that ShowSecret uses the old /update route, which will be gone once i merge #22739!

@davidsbailey
Copy link
Member

@madelynkasula that's awesome, we're so close to getting rid of /v2/sections ! 🔥

@maddiedierker
Copy link
Contributor Author

@Erin007 @davidsbailey i added a feature test to make sure PrintCertificates can still properly consume the new endpoint, so please take a look!

basically, it checks that /students still returns something that can be mapped to an array of student names by PrintCertificates, which is then rendered in a textarea on /certificates

@davidsbailey
Copy link
Member

sounds good, thanks for adding that!

@maddiedierker maddiedierker merged commit c46b236 into staging May 31, 2018
@maddiedierker maddiedierker deleted the section-students-consumer branch May 31, 2018 16:09
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