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

Call API to import a Google Classroom on "Import Section" click #16385

Merged
merged 6 commits into from Jul 13, 2017

Conversation

joshlory
Copy link
Contributor

@joshlory joshlory commented Jul 12, 2017

Connects the "Import Section" button to ApiController#import_google_classroom. Follow up to PR #16342.

screen shot 2017-07-12 at 2 16 56 pm


export const sectionShape = PropTypes.shape({
id: PropTypes.number.isRequired,
name: PropTypes.string.isRequired,
// Though we validate valid login types here, the server actually owns the
// canonical list, and passes us the list of valid login types.
loginType: PropTypes.oneOf(['word', 'email', 'picture']).isRequired,
loginType: PropTypes.oneOf(Object.keys(SectionLoginType)).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


$.getJSON('/dashboardapi/import_google_classroom', { courseId }).then(() => {
this.setState({rosterDialogOpen: false});
this.setState({sectionsLoaded: false});
Copy link
Contributor

Choose a reason for hiding this comment

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

These two can be combined into one line (i.e. you can set multiple state field in a single call to setState)

$.getJSON("/v2/sections/").done(results => {
this.props.setSections(results, true);
this.setState({sectionsLoaded: true});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these APIs set sectionsLoaded. If the call to /v2/sections finishes first, we end up with sectionsLoaded = true. If import_google_classroom finishes first, we end up with sectionsLoaded = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow. The JSON calls are nested, so the /v2/sections request won't start until after the first request resolves.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. Didn't realize they were nested.

@@ -18,7 +18,7 @@ export const setValidAssignments = (validCourses, validScripts) => ({
validCourses,
validScripts
});
export const setSections = sections => ({ type: SET_SECTIONS, sections });
export const setSections = (sections, reset = false) => ({ type: SET_SECTIONS, sections, reset });
Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to see a comment somewhere about what reset means

@Bjvanminnen
Copy link
Contributor

Would like to see some tests.

@joshlory joshlory force-pushed the import-google-classroom-next branch from f1bec8e to 2674059 Compare July 13, 2017 00:51
return {
...state,
sectionIds: state.sectionIds.concat(sections.map(section => section.id)),
sectionIds: prevSectionIds.concat(sections.map(section => section.id)),
Copy link
Contributor

Choose a reason for hiding this comment

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

might be good to get some test coverage of this reset behavior in teacherSectionsReduxTest.js

@joshlory joshlory merged commit 7f54e2c into staging Jul 13, 2017
@joshlory joshlory deleted the import-google-classroom-next branch July 13, 2017 17:08
@Bjvanminnen
Copy link
Contributor

Was hoping to still see some test coverage of reset behavior in teacherSectionsRedux.

@joshlory
Copy link
Contributor Author

Ok, I'll create a follow up PR.

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

2 participants