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

Edit vocabulary lessons in dialog #38839

Merged
merged 7 commits into from
Feb 4, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/src/lib/levelbuilder/AllVocabulariesEditor.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class AllVocabulariesEditor extends Component {
removeVocabulary: PropTypes.func.isRequired,

courseVersionId: PropTypes.number.isRequired,
courseVersionLessons: PropTypes.arrayOf(PropTypes.object),
courseName: PropTypes.string.isRequired
};

Expand Down Expand Up @@ -181,6 +182,7 @@ class AllVocabulariesEditor extends Component {
})
}
courseVersionId={this.props.courseVersionId}
selectableLessons={this.props.courseVersionLessons}
/>
)}
{this.state.vocabularyForDeletion && (
Expand Down
41 changes: 38 additions & 3 deletions apps/src/lib/levelbuilder/lesson-editor/AddVocabularyDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import DialogFooter from '@cdo/apps/templates/teacherDashboard/DialogFooter';
import color from '@cdo/apps/util/color';
import {vocabularyShape} from '@cdo/apps/lib/levelbuilder/shapes';
import $ from 'jquery';
import Select from 'react-select';
import 'react-select/dist/react-select.css';

const styles = {
dialog: {
Expand Down Expand Up @@ -43,15 +45,17 @@ const initialState = {
word: '',
definition: '',
isSaving: false,
error: ''
error: '',
lessons: []
};

export default class AddVocabularyDialog extends Component {
static propTypes = {
afterSave: PropTypes.func.isRequired,
handleClose: PropTypes.func.isRequired,
editingVocabulary: vocabularyShape,
courseVersionId: PropTypes.number.isRequired
courseVersionId: PropTypes.number.isRequired,
selectableLessons: PropTypes.arrayOf(PropTypes.object)
};

constructor(props) {
Expand Down Expand Up @@ -84,6 +88,10 @@ export default class AddVocabularyDialog extends Component {
this.props.handleClose();
};

handleLessonSelectChange = lessonSelections => {
this.setState({lessons: lessonSelections.map(l => l.value)});
};

saveVocabulary = e => {
this.setState({isSaving: true});
const url = this.props.editingVocabulary
Expand All @@ -98,6 +106,9 @@ export default class AddVocabularyDialog extends Component {
if (this.props.editingVocabulary) {
data['key'] = this.props.editingVocabulary.key;
}
if (this.props.selectableLessons) {
data['lessonIds'] = JSON.stringify(this.state.lessons);
}
$.ajax({
url: url,
method: method,
Expand All @@ -110,7 +121,10 @@ export default class AddVocabularyDialog extends Component {
this.onClose();
})
.fail(error => {
this.setState({isSaving: false, error: error.responseText});
this.setState({
isSaving: false,
error: error.responseText
});
});
};

Expand All @@ -119,13 +133,20 @@ export default class AddVocabularyDialog extends Component {
!this.state.isSaving &&
this.state.word !== '' &&
this.state.definition !== '';
const selectableLessonOptions = this.props.selectableLessons
? this.props.selectableLessons.map(l => ({
label: l.name,
value: l.id
}))
: null;
return (
<BaseDialog isOpen={true} handleClose={this.onClose}>
<h2>
{this.props.editingVocabulary ? 'Edit Vocabulary' : 'Add Vocabulary'}
</h2>

{this.state.error && <h3>{this.state.error}</h3>}

<label style={styles.inputAndLabel}>
Word
<input
Expand All @@ -146,6 +167,20 @@ export default class AddVocabularyDialog extends Component {
style={styles.textInput}
/>
</label>
{this.props.selectableLessons && (
<label>
Lessons this vocabulary is in
<Select
closeMenuOnSelect={false}
options={selectableLessonOptions}
multi={true}
value={this.state.lessons}
onChange={this.handleLessonSelectChange}
className={'lessons-dropdown'}
/>
</label>
)}

<DialogFooter rightAlign>
<button
id="submit-button"
Expand Down
5 changes: 3 additions & 2 deletions apps/src/sites/studio/pages/vocabularies/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {getStore, registerReducers} from '@cdo/apps/redux';

$(document).ready(function() {
const vocabularies = getScriptData('vocabularies');
const courseVersionId = getScriptData('courseVersionId');
const courseVersionData = getScriptData('courseVersionData');
const courseName = getScriptData('courseName');

registerReducers({
Expand All @@ -23,7 +23,8 @@ $(document).ready(function() {
<Provider store={store}>
<AllVocabulariesEditor
vocabularies={vocabularies}
courseVersionId={courseVersionId}
courseVersionId={courseVersionData.id}
courseVersionLessons={courseVersionData.lessons}
courseName={courseName}
/>
</Provider>,
Expand Down
7 changes: 7 additions & 0 deletions apps/style/lessons.scss
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,10 @@ table {
}
}
}


.lessons-dropdown {
.Select-menu, .Select-menu-outer {
z-index: 3000
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ describe('AddVocabularyDialog', () => {
const wrapper = shallow(<AddVocabularyDialog {...defaultProps} />);
expect(wrapper.contains('Add Vocabulary')).to.be.true;
expect(wrapper.find('input').length).to.equal(2);
expect(wrapper.find('Select').length).to.equal(0);
});

it('closes if save is successful', () => {
Expand All @@ -32,8 +33,6 @@ describe('AddVocabularyDialog', () => {
});
instance.forceUpdate();
wrapper.update();
/*const saveVocabularySpy = sinon.stub(instance, 'saveVocabulary');
expect(saveVocabularySpy.calledOnce).to.be.true;*/
let returnData = {
id: 1,
key: 'my vocabulary word',
Expand Down Expand Up @@ -101,4 +100,37 @@ describe('AddVocabularyDialog', () => {
wrapper.update();
expect(wrapper.find('h3').contains('There was an error'));
});

it('renders default props', () => {
const wrapper = shallow(
<AddVocabularyDialog
{...defaultProps}
selectableLessons={[{id: 1, name: 'lesson1'}, {id: 2, name: 'lesson2'}]}
/>
);
expect(wrapper.contains('Add Vocabulary')).to.be.true;
expect(wrapper.find('input').length).to.equal(2);
Copy link
Member

Choose a reason for hiding this comment

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

I think @Hamms has convinced me that these kinds of "brittle expectations" are not very helpful for the following reasons:

  • they can break when someone adds a new input
  • due to the shallow render, they can break when someone wraps an existing component (button, input, etc) within another subcomponent
  • they do not test business logic
  • they are mostly just testing that the React library is working

Generally, they make tests brittle without adding much value. For now, can I recommend removing lines 111 and 112?

Personally, I think there is some value in still having the "renders default props" test case, because in the event of a later test failure, whether this test case fails can immediately help narrow down the root cause. However, this is only valuable if other test cases go on to test actual business logic (which I am not commenting on one way or another in this comment). So let's leave that in for now, but we may keep it on the table whether to remove "renders default props" test cases entirely in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Add Vocabulary" does actually test business logic because the header is different in different cases. I see the point in removing line 112 so I'll go ahead and do this

Copy link
Member

Choose a reason for hiding this comment

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

Ah, great, thank you for pointing that out! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

"Add Vocabulary" does actually test business logic because the header is different in different cases

It does! But I'd argue that means it should belong in a test that's specifically testing that the header is different in different cases, not be disguised as a test that claims to just be testing our ability to render

});

it('displays vocabulary lessons if lessons are selectable', () => {
const existingVocabulary = {
id: 200,
key: 'key',
word: 'existing vocab',
definition: 'existing definition',
lessons: [{id: 1, name: 'lesson1'}]
};
const wrapper = mount(
<AddVocabularyDialog
{...defaultProps}
selectableLessons={[{id: 1, name: 'lesson1'}, {id: 2, name: 'lesson2'}]}
editingVocabulary={existingVocabulary}
/>
);

expect(wrapper.find('Select').length).to.equal(1);
expect(wrapper.find('Select').props().value).to.deep.equal([
{id: 1, name: 'lesson1'}
]);
});
});
13 changes: 10 additions & 3 deletions dashboard/app/controllers/vocabularies_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@ def create
# PUT/PATCH /vocabularies
def update
vocabulary = Vocabulary.find_by_id(vocabulary_params[:id])
if vocabulary && vocabulary.update!(vocabulary_params)
unless vocabulary_params[:lesson_ids].nil?
vocabulary.lessons = vocabulary_params[:lesson_ids].map {|id| Lesson.find(id)}
vocabulary.lessons.each do |lesson|
lesson.script.write_script_json
end
end
if vocabulary && vocabulary.update!(vocabulary_params.except(:lesson_ids))
render json: vocabulary.summarize_for_lesson_edit
else
render json: {status: 404, error: "Vocabulary #{vocabulary_params[:id]} not found"}
Expand All @@ -39,14 +45,15 @@ def update
# GET /courses/:course_name/vocab/edit
def edit
@course_version = find_matching_course_version
@vocabularies = @course_version.vocabularies.order(:word)
@vocabularies = @course_version.vocabularies.order(:word).map(&:summarize_for_edit)
end

private

def vocabulary_params
vp = params.transform_keys(&:underscore)
vp = vp.permit(:id, :key, :word, :definition, :course_version_id)
vp = vp.permit(:id, :key, :word, :definition, :course_version_id, :lesson_ids)
vp[:lesson_ids] = JSON.parse(vp[:lesson_ids]) if vp[:lesson_ids]
vp
end

Expand Down
4 changes: 4 additions & 0 deletions dashboard/app/models/course_version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,8 @@ def destroy_and_destroy_parent_if_empty
destroy!
course_offering.destroy if course_offering && course_offering.course_versions.empty?
end

def contained_lessons
units.map(&:lessons).flatten
end
end
10 changes: 10 additions & 0 deletions dashboard/app/models/vocabulary.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ def summarize_for_lesson_edit
{id: id, key: key, word: word, definition: definition}
end

def summarize_for_edit
{
id: id,
key: key,
word: word,
definition: definition,
lessons: lessons.map(&:id)
}
end

def generate_key
return if key
self.key = word
Expand Down
3 changes: 2 additions & 1 deletion dashboard/app/views/vocabularies/edit.html.haml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
%link{href: asset_path('css/lessons.css'), rel: 'stylesheet', type: 'text/css'}
%script{src: webpack_asset_path('js/vocabularies/edit.js'),
data: {vocabularies: @vocabularies.to_json, courseVersionId: @course_version.id, courseName: params[:course_name].to_json}}
data: {vocabularies: @vocabularies.to_json, courseVersionData: {id: @course_version.id, lessons: @course_version.contained_lessons.map {|l| {id: l.id, name: l.name} }}.to_json, courseName: params[:course_name].to_json}}

#vocabularies-table
18 changes: 16 additions & 2 deletions dashboard/test/controllers/vocabularies_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ class VocabulariesControllerTest < ActionController::TestCase
assert(@response.body.include?('updated definition'))
end

test "can update lessons from params" do
Copy link
Member

Choose a reason for hiding this comment

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

nit: "can update vocab from params"?

sign_in @levelbuilder
course_version = create :course_version
lesson = create :lesson
vocabulary = create :vocabulary, word: 'word', definition: 'draft definition', course_version: course_version
post :update, params: {id: vocabulary.id, word: 'word', definition: 'updated definition', courseVersionId: course_version.id, lessonIds: [lesson.id].to_json}
assert_response :success

vocabulary.reload
assert_equal 'updated definition', vocabulary.definition
assert(@response.body.include?('updated definition'))
assert_equal [lesson], vocabulary.lessons
end
Copy link
Member

Choose a reason for hiding this comment

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

This test looks good! I think it would be a good idea to add a test for saving the lesson after saving the vocab (issue shown in video in other comment). I'm not sure how easy it would be to add a rails unit or integration test, but it should be possible to cover it with a UI test, based on the UI tests we already have for saving the lesson edit page. Up to you whether that belongs in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to look into that bug separately and add tests there.


test "creating vocabulary that already exists results in error" do
sign_in @levelbuilder
course_version = create :course_version
Expand All @@ -53,7 +67,7 @@ class VocabulariesControllerTest < ActionController::TestCase
get :edit, params: {course_name: unit_group.name}
assert_response :success
assert_equal assigns(:course_version), course_version
assert_equal assigns(:vocabularies), [vocabulary]
assert_equal assigns(:vocabularies), [vocabulary.summarize_for_edit]
end

test "can load vocab edit page of standalone script course version" do
Expand All @@ -65,6 +79,6 @@ class VocabulariesControllerTest < ActionController::TestCase
get :edit, params: {course_name: script.name}
assert_response :success
assert_equal assigns(:course_version), course_version
assert_equal assigns(:vocabularies), [vocabulary]
assert_equal assigns(:vocabularies), [vocabulary.summarize_for_edit]
end
end