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

Levelbuilders can edit courses #15167

Merged
merged 3 commits into from
May 18, 2017
Merged
Show file tree
Hide file tree
Changes from 2 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
3 changes: 2 additions & 1 deletion apps/Gruntfile.js
Expand Up @@ -431,7 +431,8 @@ testsContext.keys().forEach(testsContext);
'scriptOverview': './src/sites/studio/pages/scriptOverview.js',
'home/_homepage': './src/sites/studio/pages/home/_homepage.js',
'courses/index': './src/sites/studio/pages/courses/index.js',
'courses/show': './src/sites/studio/pages/courses/show.js'
'courses/show': './src/sites/studio/pages/courses/show.js',
'courses/edit': './src/sites/studio/pages/courses/edit.js'
};

var otherEntries = {
Expand Down
22 changes: 22 additions & 0 deletions apps/src/sites/studio/pages/courses/edit.js
@@ -0,0 +1,22 @@
import React from 'react';
import ReactDOM from 'react-dom';
import CourseEditor from '@cdo/apps/templates/courseOverview/CourseEditor';

$(document).ready(showCourseEditor);

function showCourseEditor() {
const scriptData = document.querySelector('script[data-course-editor]');
const courseEditorData = JSON.parse(scriptData.dataset.courseEditor);

// Eventually we want to do this all via redux
ReactDOM.render(
<CourseEditor
name={courseEditorData.course_summary.name}
title={courseEditorData.course_summary.title}
descriptionStudent={courseEditorData.course_summary.description_student}
descriptionTeacher={courseEditorData.course_summary.description_teacher}
scriptsInCourse={courseEditorData.course_summary.scripts.map(script => script.name)}
scriptNames={courseEditorData.script_names.sort()}
/>,
document.getElementById('course_editor'));
}
79 changes: 79 additions & 0 deletions apps/src/templates/courseOverview/CourseEditor.js
@@ -0,0 +1,79 @@
import React, { Component, PropTypes } from 'react';
import CourseScriptsEditor from './CourseScriptsEditor';

const styles = {
input: {
width: '100%',
boxSizing: 'border-box',
padding: '4px 6px',
color: '#555',
border: '1px solid #ccc',
borderRadius: 4
},
};

export default class CourseEditor extends Component {
static propTypes = {
name: PropTypes.string.isRequired,
title: PropTypes.string.isRequired,
descriptionStudent: PropTypes.string,
descriptionTeacher: PropTypes.string,
scriptsInCourse: PropTypes.arrayOf(PropTypes.string).isRequired,
scriptNames: PropTypes.arrayOf(PropTypes.string).isRequired,
};

render() {
const {
name,
title,
descriptionStudent,
descriptionTeacher,
scriptsInCourse,
scriptNames,
} = this.props;
return (
<div>
<h1>{name}</h1>
<label>
Title
<input
type="text"
name="title"
defaultValue={title}
style={styles.input}
/>
</label>
<label>
Student Description
<textarea
name="description_student"
defaultValue={descriptionStudent}
rows={5}
style={styles.input}
/>
</label>
<label>
Teacher Description
<textarea
name="description_teacher"
defaultValue={descriptionTeacher}
rows={5}
style={styles.input}
/>
</label>
<label>
Scripts
<div>
The dropdown(s) below represent the orded set of scripts in this course.
To remove a script, just set the dropdown to the default (first) value.
</div>
<CourseScriptsEditor
inputStyle={styles.input}
scriptsInCourse={scriptsInCourse}
scriptNames={scriptNames}
/>
</label>
</div>
);
}
}
66 changes: 66 additions & 0 deletions apps/src/templates/courseOverview/CourseScriptsEditor.js
@@ -0,0 +1,66 @@
import React, { Component, PropTypes } from 'react';
import ReactDOM from 'react-dom';

export default class scriptsInCourseEditor extends Component {
static propTypes = {
inputStyle: PropTypes.object.isRequired,
scriptsInCourse: PropTypes.arrayOf(PropTypes.string).isRequired,
scriptNames: PropTypes.arrayOf(PropTypes.string).isRequired,
};

constructor(props) {
super(props);

this.handleChange = this.handleChange.bind(this);

this.state = {
// want provided script names, plus one empty one
scriptsInCourse: props.scriptsInCourse.concat('')
};
}

handleChange(event) {
const root = ReactDOM.findDOMNode(this);

let selected = Array.prototype.map.call(root.children, child => child.value);
// If the last script has a value, add a new script without one
if (selected[selected.length - 1] !== '') {
selected.push('');
}
this.setState({
scriptsInCourse: selected
});
}

render() {
const { scriptNames } = this.props;
return (
<div>
{this.state.scriptsInCourse.map((selectedScript, index) => (
<select
name="scripts[]"
style={{
...this.props.inputStyle,
opacity: selectedScript === "" ? 0.4 : 1
}}
key={index}
value={selectedScript}
onChange={this.handleChange}
>
<option key="-1" value="">
Select a script to add to course
</option>
{scriptNames.map((name, index) => (
<option
key={index}
value={name}
>
{name}
</option>
))}
</select>
))}
</div>
);
}
}
48 changes: 48 additions & 0 deletions apps/src/templates/courseOverview/CourseScriptsEditor.story.js
@@ -0,0 +1,48 @@
import React from 'react';
import CourseScriptsEditor from './CourseScriptsEditor';

const styles = {
input: {
width: '100%',
boxSizing: 'border-box',
padding: '4px 6px',
color: '#555',
border: '1px solid #ccc',
borderRadius: 4
},
};

const scriptNames = [
'Hour of Code',
'course1',
'course2',
'csp1',
'csp2'
];

export default storybook => {
storybook
.storiesOf('CourseScriptsEditor', module)
.addStoryTable([
{
name: 'no selected scripts',
story: () => (
<CourseScriptsEditor
inputStyle={styles.input}
scriptsInCourse={[]}
scriptNames={scriptNames}
/>
)
},
{
name: 'one selected script',
story: () => (
<CourseScriptsEditor
inputStyle={styles.input}
scriptsInCourse={[scriptNames[1]]}
scriptNames={scriptNames}
/>
)
}
]);
};
20 changes: 19 additions & 1 deletion dashboard/app/controllers/courses_controller.rb
Expand Up @@ -43,7 +43,25 @@ def create
end
end

def update
course = Course.find_by_name!(params[:course_name])
course.update_and_persist(params[:scripts], i18n_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

A pattern we use elsewhere is to have an after_save :write_serialization vs. calling the serialization explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works in places like level, because the serialization is captured entirely in the level object. Course (and script) are weird in that in addition to serialization to course_name.course, we also need to save our localizeable data to a yml file.

We could save our localizeable data explicitly, and then have it do the serialization to course_name.course in an after_save.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you can have multiple after_save methods in a model.

redirect_to course
end

def edit
render text: 'Not yet implemented'
course = Course.find_by_name!(params[:course_name])

# We don't support an edit experience for plc courses
raise ActiveRecord::ReadOnlyRecord if course.try(:plc_course)
render 'edit', locals: {course: course}
end

def i18n_params
params.permit(
:title,
:description_student,
:description_teacher
).to_h
end
end
75 changes: 75 additions & 0 deletions dashboard/app/models/course.rb
Expand Up @@ -24,6 +24,81 @@ def skip_name_format_validation

include SerializedToFileValidation

def to_param
name
end

def self.file_path(name)
Rails.root.join("config/courses/#{name}.course")
end

def self.load_from_path(path)
serialization = File.read(path)
hash = JSON.parse(serialization)
course = Course.find_or_create_by!(name: hash['name'])
course.update_scripts(hash['script_names'])
rescue Exception => e
# print filename for better debugging
new_e = Exception.new("in course: #{path}: #{e.message}")
new_e.set_backtrace(e.backtrace)
raise new_e
end

# Updates courses.en.yml with our new localizeable strings
# @param name [string] - name of the course being updated
# @param course_strings[Hash{String => String}]
def self.update_strings(name, course_strings)
courses_yml = File.expand_path('config/locales/courses.en.yml')
i18n = File.exist?(courses_yml) ? YAML.load_file(courses_yml) : {}

i18n.deep_merge!({'en' => {'data' => {'course' => {'name' => {name => course_strings.to_h}}}}})
File.write(courses_yml, "# Autogenerated scripts locale file.\n" + i18n.to_yaml(line_width: -1))
Copy link
Contributor

Choose a reason for hiding this comment

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

We have this pattern repeated in a number of different models. I wonder if it makes sense to pull it out into a concern or its own model (not ActiveRecord-backed).

I know this is copying over existing code, but this also isn't thread safe.

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've attempted to clean up some of the existing usages (for scripts.en.yml) in the past, and found it quite challenging. I think there is probably opportunity to make this cleaner, but that it's more work than I'm willing to put in right now.

Non-thread safe is a good point, though again this is already a problem with our existing usages for scripts.en.yml (the good news is we expect courses to be edit even more infrequently than scripts).

end

def serialize
JSON.pretty_generate(
{
name: name,
script_names: course_scripts.map(&:script).map(&:name)
}
)
end

# This method updates both our localizeable strings related to this course, and
# the set of scripts that are in the course, then writes out our serialization
# @param scripts [Array<String>] - Updated list of names of scripts in this course
# @param course_strings[Hash{String => String}]
def update_and_persist(scripts, course_strings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not immediately clear that this will mutate scripts. I don't think there's too much harm, but a bit of a weird side effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe name it more clearly, something like update_and_persist_scripts?

Course.update_strings(name, course_strings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be static?

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 made it static because we're updating a file (courses.en.yml) that is shared amongst all courses.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow. Making it static doesn't provide any additional thread safety. As an instance method you don't need to pass in name. No strong preference here, just trying to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't doing it for thread safety. My intent was conceptual clarity. Strings for a course are all stored in a single file, so conceptually it made sense for me to have string persistence be a static method on the Course model rather than a method on the course instance.

update_scripts(scripts)
write_serialization
end

def write_serialization
return unless Rails.application.config.levelbuilder_mode
File.write(Course.file_path(name), serialize)
end

# @param new_scripts [Array<String>]
def update_scripts(new_scripts)
new_scripts.reject!(&:empty?)
# we want to delete existing course scripts that aren't in our new list
scripts_to_delete = course_scripts.map(&:script).map(&:name) - new_scripts

new_scripts.each_with_index do |script_name, index|
script = Script.find_by_name!(script_name)
course_script = CourseScript.find_or_create_by!(course: self, script: script) do |cs|
cs.position = index + 1
end
course_script.update!(position: index + 1)
end

scripts_to_delete.each do |script_name|
script = Script.find_by_name!(script_name)
CourseScript.where(course: self, script: script).destroy_all
end
end

def summarize
{
name: name,
Expand Down
8 changes: 8 additions & 0 deletions dashboard/app/views/courses/edit.html.haml
@@ -0,0 +1,8 @@
- course = local_assigns[:course]
- course_editor_data = {course_summary: course.summarize,
script_names: Script.all.map(&:name)}
- content_for(:head) do
%script{ src: minifiable_asset_path('js/courses/edit.js'), data: {course_editor: course_editor_data.to_json}}
= form_for(course) do |f|
#course_editor
%button.btn.btn-primary{type: 'submit', style: 'margin: 0'} Save Changes
6 changes: 6 additions & 0 deletions dashboard/config/courses/csp.course
@@ -0,0 +1,6 @@
{
"name": "csp",
"script_names": [
"csp1"
]
}
10 changes: 10 additions & 0 deletions dashboard/config/locales/courses.en.yml
@@ -0,0 +1,10 @@
# Autogenerated scripts locale file.
---
en:
data:
course:
name:
csp:
title: Computer Science Principles
description_student: Student description will eventually go here
description_teacher: Teacher description will eventually go here
2 changes: 1 addition & 1 deletion dashboard/test/factories.rb
Expand Up @@ -5,7 +5,7 @@
factory :course_script do
end
factory :course do
name "MyCourseName"
name "my-course-name"
properties nil
end
factory :experiment do
Expand Down