Commit 3c304ccb by Clinton Blackburn

Merge pull request #257 from edx/course-form-fix

Updated Course/Product saving logic
parents e7501c33 082792b0
......@@ -9,7 +9,7 @@ help:
@echo ' make migrate apply migrations '
@echo ' make serve start the dev server at localhost:8002 '
@echo ' make clean delete generated byte code and coverage reports'
@echo ' make test_javascript run javascript unit tests '
@echo ' make test_js run javascript unit tests '
@echo ' make test_python run Python unit tests with migrations disabled '
@echo ' make fast_test_python run Python unit tests in parallel '
@echo ' make quality run pep8 and pylint '
......@@ -43,7 +43,7 @@ clean:
coverage erase
rm -rf assets/ ecommerce/static/build
test_javascript:
test_js:
gulp test
test_python: clean
......@@ -61,7 +61,7 @@ quality:
pep8 --config=.pep8 ecommerce acceptance_tests
pylint --rcfile=pylintrc ecommerce acceptance_tests
validate: test_python quality test_javascript
validate: test_python quality test_js
static:
$(NODE_BIN)/r.js -o build.js
......@@ -91,5 +91,5 @@ pull_translations:
update_translations: pull_translations generate_fake_translations
# Targets in a Makefile which do not produce an output file with the same name as the target name
.PHONY: help requirements migrate serve clean test_python quality test_javascript validate html_coverage accept \
.PHONY: help requirements migrate serve clean test_python quality test_js validate html_coverage accept \
extract_translations dummy_translations compile_translations fake_translations pull_translations update_translations
define([
'collections/drf_pageable_collection',
'models/product_model',
'models/course_seat_model'
'models/course_seats/course_seat',
'utils/course_utils'
],
function (DrfPageableCollection, ProductModel, CourseSeatModel) {
function (DrfPageableCollection,
Product,
CourseSeat,
CourseUtils) {
'use strict';
return DrfPageableCollection.extend({
mode: 'client',
model: function (attrs, options) {
var modelClass = ProductModel;
var modelClass = Product;
if (attrs.product_class === 'Seat') {
modelClass = CourseSeatModel;
modelClass = CourseUtils.getCourseSeatModel(CourseUtils.getSeatType(attrs));
}
return new modelClass(attrs, options);
}
});
});
}
);
......@@ -24,6 +24,7 @@ require.config({
'text': 'bower_components/text/text',
'underscore': 'bower_components/underscore/underscore',
'underscore.string': 'bower_components/underscore.string/dist/underscore.string',
'utils': 'js/utils',
'views': 'js/views'
},
shim: {
......
define([
'backbone',
'backbone.relational',
'backbone.super',
'backbone.validation',
'jquery-cookie',
'moment',
'underscore',
'collections/product_collection',
'models/course_seat_model'
'models/course_seats/course_seat',
'models/course_seats/honor_seat',
'utils/course_utils'
],
function (Backbone,
BackboneRelational,
BackboneSuper,
BackboneValidation,
$cookie,
moment,
_,
ProductCollection,
CourseSeatModel) {
CourseSeat,
HonorSeat,
CourseUtils) {
'use strict';
Backbone.Validation.configure({
......@@ -52,7 +62,7 @@ define([
},
verification_deadline: {
msg: gettext('Verification deadline is required for course types with verified modes.'),
required: function(value, attr, computedState) {
required: function (value, attr, computedState) {
// TODO Return true if one of the products requires ID verification.
return false;
}
......@@ -67,25 +77,181 @@ define([
},
relations: [{
collectionType: ProductCollection,
type: Backbone.HasMany,
key: 'products',
relatedModel: CourseSeatModel,
relatedModel: CourseSeat,
includeInJSON: false,
parse: true
}],
getSeats: function () {
// Returns the seat products
/**
* Mapping of course type to an array of course seat types.
*/
courseTypeSeatMapping: {
honor: ['honor'],
verified: ['honor', 'verified'],
professional: ['professional'],
credit: ['honor', 'verified', 'credit']
},
initialize: function () {
this.get('products').on('change:id_verification_required', this.triggerIdVerified, this);
this.on('sync', this.removeParentProducts, this);
},
/**
* Alerts listeners that this Course's ID verification status MAY have changed.
*/
triggerIdVerified: function (model, value) {
this.trigger('change:id_verification_required', this.isIdVerified());
},
/**
* Removes the parent products from the Product collection.
*
* This product is never exposed to the user, and should be ignored for all data operations.
*/
removeParentProducts: function () {
var products = this.get('products'),
parents = _.where(products, {structure: 'parent'});
var seats = this.get('products').filter(function (product) {
products.remove(parents);
},
/**
* Returns all seats related to this course
*
* @returns {CourseSeat[]}
*/
seats: function () {
return this.get('products').filter(function (product) {
// Filter out parent products since there is no need to display or modify.
return (product instanceof CourseSeat) && product.get('structure') !== 'parent';
});
},
/**
* Returns an existing CourseSeat corresponding to the given seat type; or creates a new one,
* if one is not found.
*
* @param {String} seatType
* @returns {CourseSeat}
*/
getOrCreateSeat: function (seatType) {
var seatClass,
products = this.get('products'),
seat = products.find(function (product) {
// Filter out parent products since there is no need to display or modify.
return (product instanceof CourseSeatModel) && product.get('structure') !== 'parent';
}),
return (product instanceof CourseSeat) && (product.seatType === seatType);
});
if (!seat) {
seatClass = CourseUtils.getCourseSeatModel(seatType);
seat = new seatClass();
products.add(seat);
}
return seat;
},
// TODO Rename to seatMap
/**
* Returns a mapping of certificate types/modes to CourseSeats.
*
* @returns {Object}
*/
getSeats: function () {
var seats = this.seats(),
seatTypes = _.map(seats, function (seat) {
return seat.get('certificate_type');
return seat.getSeatType();
});
return _.object(seatTypes, seats);
},
/**
* Returns boolean indicating if this Course is verified.
*
* A Course is considered verified if any of its seats requires ID verification.
*
* @returns {boolean}
*/
isIdVerified: function () {
return Boolean(_.find(this.getCleanProducts(), function (seat) {
return seat.get('id_verification_required');
}, this));
},
/**
* Returns an array of seat types relevant to this Course, based on its type.
*
* @returns {String[]} - Array of course seat types, or an empty array if the course type is unrecognized.
*/
validSeatTypes: function () {
return _.result(this.courseTypeSeatMapping, this.get('type'), []);
},
/**
* Returns an array of Products relevant to this Course, based on its course type.
*
* This method is primarily intended to clean Products models created to support
* the course form view.
*
* @returns {Product[]}
*/
getCleanProducts: function () {
return this.seats().filter(function (seat) {
return _.contains(this.validSeatTypes(), seat.getSeatType());
}, this);
},
/**
* Save the Course using the publication endpoint.
*
* We use this endpoint because it saves the data, and publishes it
* to external systems, in an atomic fashion. This is desirable to
* avoid synchronization issues across systems.
*/
save: function (options) {
var verificationDeadline,
data = {
id: this.get('id'),
name: this.get('name'),
verification_deadline: null
};
// Submit only the relevant products
data.products = _.map(this.getCleanProducts(), function (product) {
return product.toJSON();
}, this);
if (this.isIdVerified()) {
verificationDeadline = this.get('verification_deadline');
if (verificationDeadline) {
data.verification_deadline = moment.utc(verificationDeadline).format();
}
}
_.defaults(options || (options = {}), {
// Always use POST to avoid having to create a parameterized URL
type: 'POST',
// Use the publication endpoint
url: '/api/v2/publication/',
// The API requires a CSRF token for all POST requests using session authentication.
headers: {'X-CSRFToken': $.cookie('ecommerce_csrftoken')},
// JSON or bust!
contentType: 'application/json'
});
// Override the data and URL to use the publication endpoint.
options.data = JSON.stringify(data);
return this._super(null, options);
}
});
}
......
define([
'models/course_seats/course_seat'
],
function (CourseSeat) {
'use strict';
return CourseSeat.extend({
defaults: _.extend({}, CourseSeat.prototype.defaults,
{
certificate_type: null,
id_verification_required: false,
price: 0
}
)
}, {seatType: 'audit'});
}
);
define([
'backbone.super',
'models/product_model'
],
function (BackboneSuper,
ProductModel) {
function (ProductModel) {
'use strict';
return ProductModel.extend({
......@@ -11,14 +9,15 @@ define([
certificate_type: null,
expires: null,
id_verification_required: null,
price: null,
price: 0,
product_class: 'Seat'
},
validation: {
certificate_type: {
required: true
},
// TODO Determine how to set this to the model's default.
//certificate_type: {
// required: true
//},
price: {
required: true
},
......@@ -27,9 +26,27 @@ define([
}
},
// TODO Determine how to use the extended seatType attribute of child classes with Backbone.Relational
// http://backbonerelational.org/#RelationalModel-subModelTypes
getSeatType: function () {
switch (this.get('certificate_type')) {
case 'verified':
return 'verified';
case 'credit':
return 'credit';
case 'professional':
case 'no-id-professional':
return 'professional';
case 'honor':
return 'honor';
default:
return 'audit';
}
},
getSeatTypeDisplayName: function () {
switch (this.get('certificate_type')) {
case 'verified':
return gettext('Verified');
case 'credit':
return gettext('Credit');
......
define([
'models/course_seats/course_seat'
],
function (CourseSeat) {
'use strict';
return CourseSeat.extend({
defaults: _.extend({}, CourseSeat.prototype.defaults,
{
certificate_type: 'honor',
id_verification_required: false,
price: 0
}
)
}, {seatType: 'honor'});
}
);
define([
'models/course_seats/course_seat'
],
function (CourseSeat) {
'use strict';
return CourseSeat.extend({
defaults: _.extend({}, CourseSeat.prototype.defaults,
{
certificate_type: 'professional',
id_verification_required: false,
price: 1000
}
)
}, {seatType: 'professional'});
}
);
define([
'models/course_seats/course_seat'
],
function (CourseSeat) {
'use strict';
return CourseSeat.extend({
defaults: _.extend({}, CourseSeat.prototype.defaults,
{
certificate_type: 'verified',
id_verification_required: true,
price: 100
}
)
}, {seatType: 'verified'});
}
);
......@@ -21,7 +21,7 @@ define([
model: this.model
});
this.listenTo(this.model, 'change sync', this.render);
this.listenTo(this.model, 'sync', this.render);
this.model.fetch({
data: {include_products: true}
});
......
......@@ -103,9 +103,7 @@ define([
it('should list the course seats', function () {
var $seats = view.$el.find('.course-seat'),
products = _.filter(data.products, function (product) {
return product.product_class === 'Seat' && product.structure === 'child';
});
products = _.where(data.products, {structure: 'child'});
expect($seats.length).toEqual(products.length);
......
define([
'underscore',
'models/course_seats/audit_seat',
'models/course_seats/course_seat',
'models/course_seats/honor_seat',
'models/course_seats/professional_seat',
'models/course_seats/verified_seat'
],
function (_,
AuditSeat,
CourseSeat,
HonorSeat,
ProfessionalSeat,
VerifiedSeat) {
'use strict';
return {
/**
* Returns a mapping of seat types to CourseSeat classes.
*
* All classes included in the map extend CourseSeat.
*
* @returns {CourseSeat[]}
*/
getSeatModelMap: _.memoize(function () {
return _.indexBy([AuditSeat, HonorSeat, ProfessionalSeat, VerifiedSeat], 'seatType');
}),
/**
* Returns a CourseSeat class corresponding to the seat type.
*
* @param {String} seatType
* @returns {CourseSeat} - CourseSeat subclass, or CourseSeat if seatType is not mapped to a specific class.
*/
getCourseSeatModel: function (seatType) {
return this.getSeatModelMap()[seatType] || CourseSeat;
},
/**
* Returns the seat type for a given model.
*
* @param {Backbone.Model|Object} seat
* @returns {String|null}
*/
getSeatType: function (seat) {
var seatType = seat.seatType;
if (!seatType) {
// Fall back to using certificate type
switch (seat.get('certificate_type') || seat.certificate_type) {
case 'verified':
seatType = 'verified';
break;
case 'credit':
seatType = 'credit';
break;
case 'professional':
case 'no-id-professional':
seatType = 'professional';
break;
case 'honor':
seatType = 'honor';
break;
default:
seatType = 'audit';
break;
}
}
return seatType;
}
}
}
);
\ No newline at end of file
......@@ -23,16 +23,19 @@ define([
this.listenTo(this.model, 'change', this.render);
},
/**
* Returns an array of CourseSeat models, sorted in the order expected for display.
* @returns {CourseSeat[]}
*/
getSeats: function () {
// Returns an array of seats sorted for display
var seats,
sortObj = _.invert(_.object(_.pairs([
'honor', 'verified', 'no-id-professional', 'professional', 'credit'
'audit', 'honor', 'verified', 'no-id-professional', 'professional', 'credit'
])));
seats = _.values(this.model.getSeats());
seats = _.sortBy(seats, function (seat) {
return sortObj[seat.get('certificate_type')]
seats = _.sortBy(this.model.seats(), function (seat) {
return sortObj[seat.getSeatType()];
});
return seats;
......
......@@ -7,7 +7,6 @@ define([
'moment',
'underscore',
'underscore.string',
'models/course_seat_model',
'text!templates/course_form.html',
'text!templates/_course_type_radio_field.html',
'views/course_seat_form_fields/honor_course_seat_form_field_view',
......@@ -24,7 +23,6 @@ define([
moment,
_,
_s,
CourseSeat,
CourseFormTemplate,
CourseTypeRadioTemplate,
HonorCourseSeatFormFieldView,
......@@ -142,13 +140,12 @@ define([
this.editing = options.editing || false;
this.listenTo(this.model, 'change:type', this.renderCourseSeats);
this.listenTo(this.model, 'change:type', this.renderVerificationDeadline);
this.listenTo(this.model, 'change:products', this.updateCourseSeatModels);
this.listenTo(this.model, 'change:type change:id_verification_required', this.renderVerificationDeadline);
// Listen for the sync event so that we can keep track of the original course type.
// This helps us determine which course types the course can be upgraded to.
if (this.editing) {
this.listenTo(this.model, 'sync', this.setLockedCourseType);
this.setLockedCourseType()
}
// Enable validation
......@@ -248,42 +245,13 @@ define([
},
/**
* Returns a boolean indicating if the course being modified requires ID verification.
*/
idVerified: function () {
// TODO Move this to the course model
var activeSeatTypes = this.courseTypeSeatMapping[this.model.get('type')];
return Boolean(_.find(activeSeatTypes, function (seatType) {
return this.courseSeatViews[seatType].idVerificationRequired;
}, this));
},
/**
* Displays, or hides, the verification deadline based on the course type.
*/
renderVerificationDeadline: function () {
var $verificationDeadline = $('.verification-deadline');
var $verificationDeadline = this.$el.find('.verification-deadline');
// TODO Make this display a bit smoother with a slide.
$verificationDeadline.toggleClass('hidden', !Boolean(this.idVerified()));
return this;
},
/**
* Updates the models used by the nested course seat views to be the nested
* seat models of this view's course.
*/
updateCourseSeatModels: function () {
_.each(this.model.getSeats(), function (seat, seatType) {
var view = this.courseSeatViews[seatType];
if (view) {
view.model = seat;
view.render();
}
}, this);
$verificationDeadline.toggleClass('hidden', !this.model.isIdVerified());
return this;
},
......@@ -293,15 +261,18 @@ define([
*/
renderCourseSeats: function () {
var $courseSeats,
seats = this.model.getSeats(),
$courseSeatsContainer = this.$el.find('.course-seats'),
activeSeats = this.courseTypeSeatMapping[this.model.get('type')] || ['empty'];
activeSeats = this.model.validSeatTypes();
// Display a helpful message if the user has not yet selected a course type.
if (activeSeats.length < 1) {
activeSeats = ['empty'];
}
if (_.isEmpty(this.courseSeatViews)) {
_.each(this.courseSeatTypes, function (seatType) {
var view,
price = seatType === 'honor' ? 0 : null,
model = seats[seatType] || new CourseSeat({certificate_type: seatType, price: price}),
model = this.model.getOrCreateSeat(seatType),
viewClass = this.courseSeatViewMappings[seatType];
if (viewClass) {
......@@ -394,12 +365,11 @@ define([
* @param e
*/
submit: function (e) {
var data,
activeSeatTypes,
message,
view,
verificationDeadline,
products = [];
var $buttons,
$submitButton,
btnDefaultText,
self = this,
btnSavingContent = '<i class="fa fa-spinner fa-spin" aria-hidden="true"></i> ' + gettext('Saving...');
e.preventDefault();
......@@ -410,48 +380,41 @@ define([
return;
}
// Get the product data
activeSeatTypes = this.courseTypeSeatMapping[this.model.get('type')];
$buttons = this.$el.find('.form-actions .btn');
$submitButton = $buttons.filter('button[type=submit]');
_.each(activeSeatTypes, function (seatType) {
view = this.courseSeatViews[seatType];
view.updateModel();
products.push(view.model.toJSON());
// Store the default button text, and replace it with the saving state content.
btnDefaultText = $submitButton.text();
$submitButton.html(btnSavingContent);
}, this);
data = {
id: this.getFieldValue('id'),
name: this.getFieldValue('name'),
products: products,
verification_deadline: null
};
// Disable all buttons by setting the attribute (for <button>) and class (for <a>)
$buttons.attr('disabled', 'disabled').addClass('disabled');
verificationDeadline = this.getFieldValue('verification_deadline');
if (this.idVerified() && verificationDeadline) {
data.verification_deadline = moment.utc(this.getFieldValue('verification_deadline')).format();
}
this.model.save({
complete: function () {
// Restore the button text
$submitButton.text(btnDefaultText);
$.ajax({
contentType: 'application/json',
context: this,
data: JSON.stringify(data),
dataType: 'json',
headers: {'X-CSRFToken': $.cookie('ecommerce_csrftoken')},
method: 'POST',
url: '/api/v2/publication/',
success: function (data, textStatus, jqXHR) {
this.goTo(data.id);
// Re-enable the buttons
$buttons.removeAttr('disabled').removeClass('disabled');
},
error: function (jqXHR, textStatus, errorThrown) {
// NOTE: jqXHR.responseJSON maps field names to an array of error messages.
console.debug(jqXHR.responseJSON);
message = jqXHR.responseJSON.error || gettext('An error occurred while saving the data.');
this.clearAlerts();
this.renderAlert('danger', message);
this.$el.animate({scrollTop: 0}, 'slow');
success: function (model, response) {
self.goTo(model.id);
},
error: function (model, response) {
var message = gettext('An error occurred while saving the data.');
if (response.responseJSON && response.responseJSON.error) {
message = response.responseJSON.error;
}
self.clearAlerts();
self.renderAlert('danger', message);
self.$el.animate({scrollTop: 0}, 'slow');
}
});
return this;
}
});
}
......
......@@ -71,8 +71,6 @@ define([
return this.$(_s.sprintf('input[name=%s]', name)).val();
},
// TODO Validate the input: http://thedersen.com/projects/backbone-validation/.
/***
* Return the input data from the form fields.
*/
......@@ -85,10 +83,6 @@ define([
}, this);
return data;
},
updateModel: function () {
this.model.set(this.getData());
}
});
}
......
......@@ -53,5 +53,9 @@
.fields {
width: 50%;
}
.form-actions {
margin-top: spacing-vertical(large);
}
}
}
<div class="row course-seat">
<div class="col-sm-12">
<div class="seat-type"><%= seat.getSeatType() %></div>
<div class="seat-type"><%= seat.getSeatTypeDisplayName() %></div>
</div>
<div class="col-sm-4">
<div class="seat-price"><%= gettext('Price:') + ' $' + Number(seat.get('price')).toLocaleString() %></div>
......
......@@ -29,7 +29,16 @@
<!-- Radio fields will be appended here. -->
<div class="course-types"></div>
</div>
</div>
<div class="form-group course-seats">
<label class="hd-4"><%= gettext('Course Seats') %></label>
<div class="course-seat empty"><em><%= gettext('Select a course type.') %></em></div>
<div class="editable-seats"></div>
</div>
<div class="fields">
<div class="form-group verification-deadline hidden">
<label class="hd-4" for="verification_deadline"><%= gettext('Verification Deadline') %></label>
......@@ -44,18 +53,11 @@
<%= gettext('After this date/time, students can no longer submit photos for ID verification.') %>
</span>
<span class="help-block">
<em>(<%= gettext('This is only required for course modes that require ID verification.') %>)</em>
<em>(<%= gettext('This is field is optional for course seats that require ID verification.') %>)</em>
</span>
</div>
</div>
<div class="form-group course-seats">
<label class="hd-4"><%= gettext('Course Seats') %></label>
<div class="course-seat empty"><em><%= gettext('Select a course type.') %></em></div>
<div class="editable-seats"></div>
</div>
<div class="form-actions">
<button type="submit" class="btn btn-primary"><%= gettext(editing ? 'Save Changes' : 'Create Course') %></button>
<a class="btn btn-default" href="/courses/<% if(id){ print(id + '/'); } %>"><%= gettext('Cancel') %></a>
......
......@@ -16,7 +16,7 @@
</div>
</div>
<div class="form-group expires">
<div class="expires">
<label for="expires"><%= gettext('Upgrade Deadline') %></label>
<div class="input-group" data-toggle="tooltip" title="<%= gettext('Professional education courses have no upgrade deadline.') %>">
......
......@@ -17,7 +17,7 @@
</div>
</div>
<div class="form-group expires">
<div class="expires">
<label for="expires"><%= gettext('Upgrade Deadline') %></label>
<div class="input-group">
......
......@@ -17,10 +17,7 @@ module.exports = function(config) {
files: [
{pattern: 'ecommerce/static/vendor/**/*.js', included: false},
{pattern: 'ecommerce/static/bower_components/**/*.js', included: false},
{pattern: 'ecommerce/static/js/models/**/*.js', included: false},
{pattern: 'ecommerce/static/js/views/**/*.js', included: false},
{pattern: 'ecommerce/static/js/collections/**/*.js', included: false},
{pattern: 'ecommerce/static/js/test/specs/*.js', included: false},
{pattern: 'ecommerce/static/js/**/*.js', included: false},
{pattern: 'ecommerce/static/templates/**/*.html', included: false},
'ecommerce/static/js/config.js',
'ecommerce/static/js/test/spec-runner.js'
......@@ -33,9 +30,7 @@ module.exports = function(config) {
// preprocess matching files before serving them to the browser
// available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor
preprocessors: {
'ecommerce/static/js/views/**/*.js': ['coverage'],
'ecommerce/static/js/models/**/*.js': ['coverage'],
'ecommerce/static/js/collections/**/*.js': ['coverage']
'ecommerce/static/js/!(test)/**/*.js': ['coverage']
},
// enabled plugins
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment