-
Notifications
You must be signed in to change notification settings - Fork 481
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 students: hook up new component, remove old code #22208
Conversation
… manageStudentsRedux
return false; | ||
} | ||
}; | ||
}]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const nameCells = wrapper.find('.uitest-name-cell'); | ||
expect(nameCells.at(0).text()).to.equal('studentc'); | ||
expect(nameCells.at(1).text()).to.equal('studentb'); | ||
expect(nameCells.at(2).text()).to.equal('studenta'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're also using chai-enzyme
which enables enzyme-specific assertions like:
expect(nameCells.at(0)).to.have.text('studentc');
expect(nameCells.at(1)).to.have.text('studentb');
expect(nameCells.at(2)).to.have.text('studenta');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooray for tearing out old code!
I'll check again after the updates to transfers_controller_test.rb.
@islemaster thanks for the feedback! the |
isMoveStudentsEnabled = () => { | ||
const {loginType} = this.props; | ||
return loginType === SectionLoginType.word || loginType === SectionLoginType.picture || loginType === SectionLoginType.email; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one more thing: Can we add a test to ManageStudentsTableTest
asserting this behavior? (Or maybe its inverse - we don't show a move students button for Google or Clever sections.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sure, that's a good call 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is the last in a series to convert 'move students' from Angular to React. See my previous PR or the spec for context.
What does this code do?
/sections/transfers
endpoint. specifically, the angular implementation was sending thestay_enrolled_in_current_section
andstudent_ids
params as stringified boolean and array of integers, respectively. the new implementation sends a JSON request with boolean/[int].Examples
Manage Students tab
Routing to the 'manage students' tab, you should now see the new 'move students' button:
Default alphabetical student sorting
On opening the dialog, the students in the dialog table should be sorted alphabetically by name:
Success Notifications
When students are successfully transferred to a different section, the dialog should close and the
ManageStudentsTable
component (parent ofMoveStudents
) will render a notification. This notification should be different based on whether the students were moved (and subsequently removed from the current section) or copied to the new section.If students were moved to new section:
If students were copied to new section: