[WIP] Online editor for python #1732
Conversation
| } | ||
|
|
||
| .testSelectionRuler { | ||
| border-top: 1px solid #cccccc; |
There was a problem hiding this comment.
Color literals like #cccccc should only be used in variable declarations; they should be referred to via variable everywhere else.
| border: none; | ||
| border-radius: 0; | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
Files should end with a trailing newline
| } | ||
|
|
||
| .addNewTestButton { | ||
| border: none; |
There was a problem hiding this comment.
border: 0 is preferred over border: none
| @@ -0,0 +1,9 @@ | |||
| .addNewTestRow { | |||
| padding: 0 !important; | |||
| when language.is_a?(Coursemology::Polyglot::Language::Python) | ||
| data = python_meta(attachment) | ||
| { editor_mode: 'python', data: data } if data.present? | ||
| else |
| end | ||
|
|
||
| def extract_meta(language, attachment) | ||
| case |
There was a problem hiding this comment.
Do not use empty case condition, instead use an if expression.
| when language.is_a?(Coursemology::Polyglot::Language::Python) | ||
| new_package = python_package(old_attachment, params) | ||
| yield new_package if new_package.present? | ||
| else |
|
|
||
|
|
||
| def package(language, old_attachment, params) | ||
| case |
There was a problem hiding this comment.
Do not use empty case condition, instead use an if expression.
| extend ActiveSupport::Concern | ||
| include Course::Assessment::Question::Programming::Python::PackageConcern | ||
|
|
||
|
|
| :python | ||
| when language.is_a?(Coursemology::Polyglot::Language::JavaScript) | ||
| :javascript | ||
| else |
| @@ -31,18 +33,51 @@ def display_build_log? | |||
| Course::Assessment::ProgrammingEvaluationService::Error.name | |||
| end | |||
|
|
|||
| def editor_mode(language) | |||
| case | |||
There was a problem hiding this comment.
Do not use empty case condition, instead use an if expression.
| end | ||
| render '_props' | ||
| else | ||
| render '_props' |
There was a problem hiding this comment.
Move render '_props' out of the conditional.
| if @programming_question.import_job | ||
| @redirect_url = job_path(@programming_question.import_job) | ||
| end | ||
| render '_props' |
There was a problem hiding this comment.
Move render '_props' out of the conditional.
| render 'edit' | ||
| end | ||
| } | ||
| format.json { |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
| end | ||
|
|
||
| respond_to do |format| | ||
| format.html { |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
| @@ -22,18 +39,43 @@ def create | |||
| end | |||
|
|
|||
| def edit | |||
| respond_to do |format| | |||
| format.html | |||
| format.json { | |||
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
|
The PR is a bit too big. Here I give some general comments first:
|
| @@ -0,0 +1,128 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Create a new service for creating programming packages. Put this logic and the supporting template files with the service.
There was a problem hiding this comment.
Yes, call it package generation service or something, so that you can properly test the service.
There was a problem hiding this comment.
Also, I think you should document the input format of the service and the output package hierarchy.
| test_fn = " def test_#{test_type.to_s}_#{index}(self):\n"\ | ||
| " self.meta['expression'] = '#{escaped_expression}'\n"\ | ||
|
|
||
| if test[:expected].first == '\'' && test[:expected].last == '\'' || |
There was a problem hiding this comment.
This assumes that quote marks can only appear at the start and end of the string. Is this the intention?
|
|
||
| if tests.blank? | ||
| zip.print " def test_public_1(self):\n"\ | ||
| " self.meta['expression'] = 'This testcase has no meaning.'\n"\ |
There was a problem hiding this comment.
These strings need to be translated. I am not sure if we need this, I think the UI should support a mode for non-autograded programming questions ?
| t('course.assessment.question.programming.form.import_result.success') | ||
| def successful_import_alert(json: false) | ||
| cls = ['alert', 'alert-success'] | ||
| msg = t('course.assessment.question.programming.form.import_result.success') |
There was a problem hiding this comment.
Call it klass and message, We don't use shorthand variable names...
| ); | ||
| }; | ||
|
|
||
| $.getJSON('', (data) => { |
There was a problem hiding this comment.
How long does it take to build the data ? If it's not very long it would be better to put the data in the initial request and parse them in the frontend, that will make the user side feels faster.
LessonPlan pages uses 2 requests because the lesson plan data are quite large and it slows down the initial request a lot.
| onSubmit={this.onSubmit.bind(this)} ref={(form) => { this.form = form; }} > | ||
| <input type='hidden' name='authenticity_token' value={formData.get('auth_token')} /> | ||
|
|
||
| { this.renderInputField('title', 'Title', false, 'text', question.get('title') || '') } |
There was a problem hiding this comment.
All these strings need to be translated.
|
|
||
| const propTypes = { | ||
| data: React.PropTypes.shape({ | ||
| question: PropTypes.object.isRequired, |
There was a problem hiding this comment.
Better to validate as PropTypes.instanceOf(Immutable.Map).isRequired
|
|
||
| componentWillReceiveProps(nextProps) { | ||
| this.isLoading = nextProps.data.isLoading; | ||
| this.isEvaluating = nextProps.data.isEvaluating; |
There was a problem hiding this comment.
Is this needed? Possible to do without the assignment and check this.props.isLoading and this.props.isEvaluating directly?
| return <div className="alert alert-warning">Please select a language.</div>; | ||
|
|
||
| default: | ||
| return <div className="alert alert-info">Not yet implemented :(</div>; |
There was a problem hiding this comment.
Some untranslated strings here
|
|
||
| case actionTypes.TEMPLATE_TAB_UPDATE: | ||
| const { selected } = action; | ||
| return $$state.set('package_ui', $$state.get('package_ui').set('selected', selected)); |
| }, | ||
| editTestsOnlineButton: { | ||
| id: 'course.assessment.question.programming.programmingQuestionForm.editTestsOnlineButton', | ||
| defaultMessage: 'Edit Tests Online', |
There was a problem hiding this comment.
The text might not be fitting in the case where we create tests. Will "Use Test Case Editor" or just "Test Case Editor" be better?
| value={this.props.data.get(field)} | ||
| onChange={this.onCodeChange.bind(this, field)} | ||
| editorProps={{$blockScrolling: true}} | ||
| setOptions={{useSoftTabs: true}} |
There was a problem hiding this comment.
Can set this as readOnly when isLoading also.
|
- No longer using the $$ convention - Set Ace Editor to read only when loading - Use jQuery to disable summernote when loading - Change 'Edit Tests Online' to 'Use Test Case Editor' - Validate Immutable.Map props - Move translations under constants
…itor to react components
justwudi
force-pushed
the
wudi/online-editor
branch
from
dd84736 to
7e8f5c3
Compare
| meta | ||
| end | ||
|
|
||
| def python_test_params(params) |
| tmp | ||
| end | ||
|
|
||
| def get_data_files_meta(data_files_to_keep, new_data_files) |
There was a problem hiding this comment.
Assignment Branch Condition size for get_data_files_meta is too high. [15.3/15]
| def generate_zip_file(data_files_to_keep, params) | ||
| tmp = Tempfile.new(['package', '.zip']) | ||
|
|
||
| Zip::OutputStream.open(tmp.path) do |zip| |
| end | ||
| end | ||
|
|
||
| def generate_zip_file(data_files_to_keep, params) |
There was a problem hiding this comment.
Assignment Branch Condition size for generate_zip_file is too high. [66.36/15]
Method has too many lines. [54/10]
| @@ -0,0 +1,221 @@ | |||
| # frozen_string_literal: true | |||
| class Course::Assessment::Question::Programming::Python::PythonPackageService | |||
There was a problem hiding this comment.
Class has too many lines. [163/100]
| Course::Assessment::Question::Programming::Python::PythonPackageService.new | ||
| @editor_mode = 'python' | ||
| else | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Use fail instead of raise to signal exceptions.
| @language = question.language | ||
| @current_attachment = question.attachment | ||
|
|
||
| if python_language |
There was a problem hiding this comment.
Use a guard clause instead of wrapping the code inside a conditional expression.
| @@ -28,6 +30,10 @@ def auto_gradable? | |||
| !test_cases.empty? | |||
| end | |||
|
|
|||
| def edit_online? | |||
| self.package_type == 'online_editor' | |||
New ChangesAdd support for non-autograded programming question
Add support for data files
Multiselect
Front end validation
|
|
@justwudi Thanks for the effort ! Possible to split this big PR into maybe 3+ smaller ones ? You can make them depend on each other or even self contained. It will be easier for us to review and I believe it will make the code be merged faster as well. Also when you fix something, you are supposed to rebase to the previous commit and fix them there. |
Change existing form to React
The following partials have been moved to React:
_form.html.slim->ProgrammingQuestionForm.jsxSimple form has been removed, currently it contains just a
<div>with a spinner that serves as the mounting point for the React component.The import alert has also been moved into
_props.json.jbuilder_form_build_log.html.slim->BuildLog.jsx_form_templates.html.slim->TemplatePackageView.jsx_form_test_cases.html.slim->TemplateTestCaseView.jsxAdd online editor for python test cases
Users will be able to select to either use the online editor or upload a zip package when creating the programming question. This will be stored in in the database under a new column
package_type. When editing the programming question, users will not be able to switch between the two types.The online editor can be found in
OnlineEditorPythonView.jsx. This view will be displayed when the user selectspythonas the language. Other editor views can be added in the future for other languages.On the backend side,
Course::Assessment::Question::Programming::PackageGenerationConcernwill take in theparamsand construct the zip package from the fields of the online editor. This concern will route toCourse::Assessment::Question::Programming::Python::PackageConcernwhen the language ispython.In the generated zip package, the fields used to generate the package are stored in a hidden json file
.meta. This.metafile is used for displaying the fields for editing as well as to check whether there are any changes to the package when updating, the package will only be re-generated when the fields are modified.TODO:
.metafile and compare them.This closes #1568.