From c2dca846865d57ab895854822f0b06ca8a1c99f7 Mon Sep 17 00:00:00 2001 From: George Schneeloch Date: Tue, 30 Jun 2015 11:09:06 -0400 Subject: [PATCH 01/42] Added ability to filter vocabularies by learning resource type --- rest/tests/test_rest.py | 43 ++++++++++++++++++++++++++++++++++++++++- rest/views.py | 15 +++++++++++--- 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/rest/tests/test_rest.py b/rest/tests/test_rest.py index 8985353c..249d3b08 100644 --- a/rest/tests/test_rest.py +++ b/rest/tests/test_rest.py @@ -11,7 +11,10 @@ HTTP_405_METHOD_NOT_ALLOWED, ) -from learningresources.models import Repository +from learningresources.models import ( + Repository, + LearningResourceType, +) from taxonomy.models import Vocabulary, Term from rest.serializers import ( @@ -262,6 +265,44 @@ def test_other_vocabulary(self): other_repo_dict['slug'], vocab2_dict['slug'] ) + def test_vocabulary_filter_type(self): + """Test filtering learning resource types for vocabularies""" + self.create_vocabulary(self.repo.slug) + + learning_resource_type = LearningResourceType.objects.first() + + # in the future this should be handled within the API + Vocabulary.objects.first().learning_resource_types.add( + learning_resource_type + ) + + resp = self.client.get("{repo_base}{repo_slug}/vocabularies/".format( + repo_base=REPO_BASE, + repo_slug=self.repo.slug, + )) + self.assertEqual(resp.status_code, HTTP_200_OK) + self.assertEqual(1, as_json(resp)['count']) + + resp = self.client.get( + "{repo_base}{repo_slug}" + "/vocabularies/?type_name={name}".format( + repo_base=REPO_BASE, + repo_slug=self.repo.slug, + name=learning_resource_type.name, + )) + self.assertEqual(resp.status_code, HTTP_200_OK) + self.assertEqual(1, as_json(resp)['count']) + + resp = self.client.get( + "{repo_base}{repo_slug}" + "/vocabularies/?type_name={name}".format( + repo_base=REPO_BASE, + repo_slug=self.repo.slug, + name="missing", + )) + self.assertEqual(resp.status_code, HTTP_200_OK) + self.assertEqual(0, as_json(resp)['count']) + def test_term(self): """Test REST access for term""" vocab_slug = self.create_vocabulary(self.repo.slug)['slug'] diff --git a/rest/views.py b/rest/views.py index 09754a6f..40256974 100644 --- a/rest/views.py +++ b/rest/views.py @@ -42,6 +42,7 @@ ) from rest.util import CheckValidMemberParamMixin from learningresources.models import Repository +from taxonomy.models import Vocabulary from learningresources.api import ( get_repos, ) @@ -104,9 +105,17 @@ class VocabularyList(ListCreateAPIView): ) def get_queryset(self): - """Filter to vocabularies for a repository""" - return Repository.objects.get( - slug=self.kwargs['repo_slug']).vocabulary_set.order_by('id') + """Filter vocabularies by repository ownership and optionally + by learning resource type""" + queryset = Vocabulary.objects.filter( + repository__slug=self.kwargs['repo_slug'] + ) + learning_resource_type = self.request.query_params.get( + 'type_name', None) + if learning_resource_type is not None: + queryset = queryset.filter( + learning_resource_types__name=learning_resource_type) + return queryset.order_by('id') def get_success_headers(self, data): """Add Location header for model create""" From 6348b99e1f80af98c39356cddb1e15697244ffa9 Mon Sep 17 00:00:00 2001 From: George Schneeloch Date: Wed, 1 Jul 2015 15:26:37 -0400 Subject: [PATCH 02/42] Fixed error message behavior for manage taxonomies tab --- ui/static/ui/js/manage_taxonomies.jsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ui/static/ui/js/manage_taxonomies.jsx b/ui/static/ui/js/manage_taxonomies.jsx index 2be05f8b..1dc1697e 100644 --- a/ui/static/ui/js/manage_taxonomies.jsx +++ b/ui/static/ui/js/manage_taxonomies.jsx @@ -67,6 +67,9 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) newTermLabel: null, terms: thiz.state.terms.concat([newTerm]) }); + + // clear errors + thiz.props.reportError(null); }); }, updateAddTermText: function(e) { @@ -104,7 +107,9 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) }, reportError: function(msg) { - console.error(msg); + if (msg) { + console.error(msg); + } this.setState({errorText: msg}); }, getInitialState: function() { From c09fbd69a9dc7108258c23e64419ecb41d6135c5 Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Thu, 2 Jul 2015 09:14:11 -0400 Subject: [PATCH 03/42] Fixed buttons alignment problem in members panel. --- ui/static/ui/js/listing.js | 24 ++++++++++++------------ ui/templates/includes/members_panel.html | 7 ++----- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/ui/static/ui/js/listing.js b/ui/static/ui/js/listing.js index 21c1bac6..6492fc8b 100644 --- a/ui/static/ui/js/listing.js +++ b/ui/static/ui/js/listing.js @@ -22,23 +22,23 @@ define( var $dest = $(dest); for (var i = 0; i < userList.length; i++) { $dest.append( - '
\n' + - '
\n' + + '
' + + '
' + '
' + - userList[i].username + EMAIL_EXTENSION + '
\n' + + userList[i].username + EMAIL_EXTENSION + '
' + '
\n' + - '
\n' + + '
' + '
' + - formatGroupName(userList[i].group_type) + '
\n' + + formatGroupName(userList[i].group_type) + '
' + '
\n' + - '
\n' + + '
' + '\n' + - '
\n' + + 'data-group_type="' + userList[i].group_type + '">' + + '' + + '' + + '
' + '
\n'); } } @@ -54,9 +54,9 @@ define( //reset all the classes $('#members-alert').html( '
\n' + + ' fade in out" data-alert="alert">' + '×\n' + message + '\n' + + 'aria-label="close">×\n' + message + '
'); } diff --git a/ui/templates/includes/members_panel.html b/ui/templates/includes/members_panel.html index 9dc6d043..0025cd54 100644 --- a/ui/templates/includes/members_panel.html +++ b/ui/templates/includes/members_panel.html @@ -26,11 +26,8 @@

Members for Repository: {{ repo.name }}

-
- -
+
From a9424c128b88c5289ebc2e0b713118c0bebcbf1e Mon Sep 17 00:00:00 2001 From: George Schneeloch Date: Thu, 2 Jul 2015 11:18:10 -0400 Subject: [PATCH 04/42] Moved coverage CLI script to utils directory --- tox.ini | 2 +- {ui/jstests => util}/convert_lcov_to_coveralls.js | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename {ui/jstests => util}/convert_lcov_to_coveralls.js (100%) diff --git a/tox.ini b/tox.ini index abd8a9a0..00d6f371 100644 --- a/tox.ini +++ b/tox.ini @@ -28,7 +28,7 @@ commands = {toxinidir}/node_modules/.bin/jshint ui/static/ui/js ui/jstests/ {toxinidir}/node_modules/.bin/jscs ui/static/ui/js ui/jstests/ {toxinidir}/node_modules/.bin/karma start - {toxinidir}/ui/jstests/convert_lcov_to_coveralls.js {toxinidir}/coverage/coverage-js.lcov {toxinidir}/coverage/coverage-js.json + {toxinidir}/util/convert_lcov_to_coveralls.js {toxinidir}/coverage/coverage-js.lcov {toxinidir}/coverage/coverage-js.json whitelist_externals = */karma diff --git a/ui/jstests/convert_lcov_to_coveralls.js b/util/convert_lcov_to_coveralls.js similarity index 100% rename from ui/jstests/convert_lcov_to_coveralls.js rename to util/convert_lcov_to_coveralls.js From 75e68272afb31667fef07f218df2051c3edf90bf Mon Sep 17 00:00:00 2001 From: Amir Qayyum Khan Date: Thu, 2 Jul 2015 15:42:46 +0500 Subject: [PATCH 05/42] Added test case, tested menulay all scenarios --- ui/tests/test_learningresources_views.py | 11 +++++++++++ ui/urls.py | 7 +++++-- ui/views.py | 1 - 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/ui/tests/test_learningresources_views.py b/ui/tests/test_learningresources_views.py index 3eec9576..b865a581 100644 --- a/ui/tests/test_learningresources_views.py +++ b/ui/tests/test_learningresources_views.py @@ -188,6 +188,17 @@ def test_invalid_repo_form(self): self.assertTrue("This field is required." in body) self.assertTrue(Repository.objects.count() == 1) + def test_access_without_login(self): + """ + Tests the repository page without login + """ + self.logout() + response = self.client.get(self.repository_url, follow=True) + self.assertEqual(response.status_code, 200) + # we were redirected to login + self.assertEqual(len(response.redirect_chain), 2) + self.assertTrue(302 in response.redirect_chain[0]) + def test_export_good(self): """Get raw XML of something I should be allowed to see.""" url = reverse("export", args=(self.repo.slug, self.resource.id)) diff --git a/ui/urls.py b/ui/urls.py index 409bdcb2..b15a8b5b 100644 --- a/ui/urls.py +++ b/ui/urls.py @@ -18,6 +18,7 @@ from django.conf.urls import include, url from django.contrib import admin +from django.contrib.auth.decorators import login_required from search.forms import SearchForm from ui.views import ( @@ -38,8 +39,10 @@ url(r'^repositories/new/$', create_repo, name='create_repo'), url( r'^repositories/(?P[-\w]+)/$', - RepositoryView( - form_class=SearchForm, template="repository.html", + login_required( + RepositoryView( + form_class=SearchForm, template="repository.html", + ) ), name='repositories' ), diff --git a/ui/views.py b/ui/views.py index dd0c2df5..b25fc1c2 100644 --- a/ui/views.py +++ b/ui/views.py @@ -220,7 +220,6 @@ def __call__(self, request, repo_slug): self.repo = [x for x in repos if x.slug == repo_slug][0] return super(RepositoryView, self).__call__(request) - @login_required def dispatch(self, *args, **kwargs): """Override for the purpose of having decorators in views.py""" super(RepositoryView, self).dispatch(*args, **kwargs) From 573ef9b072eebc337a30fcf7d228313a65c82de7 Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Thu, 2 Jul 2015 09:55:56 -0400 Subject: [PATCH 06/42] Allow only usernames and not emails in the Members add input --- ui/static/ui/js/listing.js | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/ui/static/ui/js/listing.js b/ui/static/ui/js/listing.js index 6492fc8b..a94ab1b5 100644 --- a/ui/static/ui/js/listing.js +++ b/ui/static/ui/js/listing.js @@ -12,8 +12,7 @@ define( /* This is going to grow up to check whether * the name is a valid Kerberos account */ - function isEmail(username) { - var email = username + EMAIL_EXTENSION; + function isEmail(email) { var regex = /^[-a-z0-9~!$%^&*_=+}{\'?]+(\.[-a-z0-9~!$%^&*_=+}{\'?]+)*@([a-z0-9_][-a-z0-9_]*(\.[-a-z0-9_]+)*\.(aero|arpa|biz|com|coop|edu|gov|info|int|mil|museum|name|net|org|pro|travel|mobi|[a-z][a-z])|([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}))(:[0-9]{1,5})?$/i; return regex.test(email); } @@ -156,10 +155,17 @@ define( var groupType = $('select[name=\'members-group_type\']').val(); // /api/v1/repositories/my-rep/members/groups//users/ url += 'groups/' + groupType + '/users/'; - if (!isEmail(username)) { - var message = '' + username + EMAIL_EXTENSION + + //test that username is not an email + if (isEmail(username)) { + var message = 'Please type only your username before the @'; + showMembersAlert(message, 'warning'); + return; + } + var email = username + EMAIL_EXTENSION; + if (!isEmail(email)) { + var emailMessage = '' + email + ' does not seem to be a valid email'; - showMembersAlert(message, 'danger'); + showMembersAlert(emailMessage, 'danger'); return; } $.ajax({ @@ -171,7 +177,7 @@ define( //reset the values resetUserGroupForm(); //show alert - var message = '' + username + EMAIL_EXTENSION + + var message = '' + email + ' added to group ' + formatGroupName(groupType) + ''; showMembersAlert(message); @@ -180,7 +186,7 @@ define( }) .error(function(data) { //show alert - var message = 'Error adding user ' + username + EMAIL_EXTENSION + + var message = 'Error adding user ' + email + ' to group ' + formatGroupName(groupType); message = message + '
' + data.responseJSON.username[0]; showMembersAlert(message, 'danger'); @@ -191,6 +197,7 @@ define( var url = $('.cd-panel-members').data('members-url'); var username = $(this).data('username'); var groupType = $(this).data('group_type'); + var email = username + EMAIL_EXTENSION; // /api/v1/repositories/my-rep/members/groups//users// url += 'groups/' + groupType + '/users/' + username + '/'; $.ajax({ @@ -199,7 +206,7 @@ define( }) .done(function() { //show alert - var message = '' + username + EMAIL_EXTENSION + + var message = '' + email + ' deleted from group ' + formatGroupName(groupType) + ''; showMembersAlert(message); @@ -209,7 +216,7 @@ define( .error(function(data) { //show alert var message = 'Error deleting user ' + - username + EMAIL_EXTENSION + ' from group ' + + email + ' from group ' + formatGroupName(groupType) + ''; try { message += '
' + data.responseJSON.detail; From 35516525a63896c2b8d3710f1e3ea03d6c70f73c Mon Sep 17 00:00:00 2001 From: George Schneeloch Date: Thu, 2 Jul 2015 12:21:12 -0400 Subject: [PATCH 07/42] Included JSX files in coverage results --- .jshintrc | 3 +- karma.conf.js | 43 +++++++++++++++++++------ ui/jstests/test-listing.js | 27 ---------------- ui/jstests/test-main.js | 33 +++++++++++++++++++ ui/static/ui/js/manage_vocabularies.jsx | 0 ui/static/ui/js/require_paths.js | 15 +++++++++ ui/templates/base.html | 14 ++------ 7 files changed, 87 insertions(+), 48 deletions(-) delete mode 100644 ui/jstests/test-listing.js create mode 100644 ui/jstests/test-main.js delete mode 100644 ui/static/ui/js/manage_vocabularies.jsx create mode 100644 ui/static/ui/js/require_paths.js diff --git a/.jshintrc b/.jshintrc index 4241c119..508bcca9 100644 --- a/.jshintrc +++ b/.jshintrc @@ -92,7 +92,8 @@ "require": false, "define": false, "module": false, - "process": false + "process": false, + "REQUIRE_PATHS": false } // additional predefined global variables } diff --git a/karma.conf.js b/karma.conf.js index d6aeb0f0..3879639e 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -15,30 +15,55 @@ module.exports = function(config) { // list of files / patterns to load in the browser files: [ - 'ui/jstests/test-listing.js', + 'ui/static/ui/js/require_paths.js', + 'ui/jstests/test-main.js', { - pattern: 'ui/static/bower/react/react.js', + pattern: 'ui/static/bower/**/*.js', included: false }, { - pattern: 'ui/static/ui/js/**/*.js*', + pattern: 'ui/static/bower/**/*.jsx', included: false }, { - pattern: 'ui/jstests/**/*.js*', + pattern: 'ui/static/ui/**/*.js', + included: false + }, + { + pattern: 'ui/static/ui/**/*.jsx', + included: false + }, + { + pattern: 'ui/jstests/**/*.js', + included: false + }, + { + pattern:'ui/jstests/**/*.jsx', + included: false + }, + { + pattern: 'node_modules/**/*.js', + included: false + }, + { + pattern: 'node_modules/**/*.jsx', included: false } ], - // list of files to exclude - exclude: [], + // list of files to exclude from coverage and testing + exclude: [ + "ui/static/ui/js/listing.js", + "ui/static/ui/js/csrf.js" + ], // preprocess matching files before serving them to the browser // available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor preprocessors: { - '**/*.jsx': ['react'], - 'ui/static/ui/js/**/*.js*': ['coverage'], - 'ui/jstests/**/*.js*': ['coverage'] + 'ui/static/ui/**/*.jsx': ['react', 'coverage'], + 'ui/jstests/**/*.jsx': ['react', 'coverage'], + 'ui/static/ui/**/*.js': ['coverage'], + 'ui/jstests/**/*.js': ['coverage'] }, reactPreprocessor: { diff --git a/ui/jstests/test-listing.js b/ui/jstests/test-listing.js deleted file mode 100644 index f92791a1..00000000 --- a/ui/jstests/test-listing.js +++ /dev/null @@ -1,27 +0,0 @@ -var allTestFiles = []; -var TEST_REGEXP = /(spec|test)\.js$/i; - -Object.keys(window.__karma__.files).forEach(function(file) { - 'use strict'; - if (TEST_REGEXP.test(file)) { - // Normalize paths to RequireJS module names. - allTestFiles.push(file); - } -}); - -require.config({ - // Karma serves files under /base, which is the basePath from your config file - baseUrl: '/base/', - - // dynamically load all test files - deps: allTestFiles, - - // paths for required libraries - paths: { - QUnit: 'node_modules/qunitjs/qunit/qunit', - react: 'ui/static/bower/react/react' - }, - - // we have to kickoff karma, as it is asynchronous - callback: window.__karma__.start -}); diff --git a/ui/jstests/test-main.js b/ui/jstests/test-main.js new file mode 100644 index 00000000..b7eb862e --- /dev/null +++ b/ui/jstests/test-main.js @@ -0,0 +1,33 @@ +var allFiles = []; +var TEST_EXCLUDE_REGEXP = /(ui\/static\/bower\/)|(node_modules)|(test-main.js)|(require_paths.js)/i; +var JS_INCLUDE_REGEXP = /\.jsx?$/i; + +Object.keys(window.__karma__.files).forEach(function(file) { + 'use strict'; + if (JS_INCLUDE_REGEXP.test(file) && !TEST_EXCLUDE_REGEXP.test(file)) { + // Normalize paths to RequireJS module names. + allFiles.push(file); + } +}); + +var paths = {}; +for (var key in REQUIRE_PATHS) { + if (REQUIRE_PATHS.hasOwnProperty(key)) { + paths[key] = 'ui/static/bower/' + REQUIRE_PATHS[key]; + } +} +paths.QUnit = 'node_modules/qunitjs/qunit/qunit'; + +require.config({ + // Karma serves files under /base, which is the basePath from your config file + baseUrl: '/base/', + + // dynamically load all files + deps: allFiles, + + // paths for required libraries + paths: paths, + + // we have to kickoff karma, as it is asynchronous + callback: window.__karma__.start +}); diff --git a/ui/static/ui/js/manage_vocabularies.jsx b/ui/static/ui/js/manage_vocabularies.jsx deleted file mode 100644 index e69de29b..00000000 diff --git a/ui/static/ui/js/require_paths.js b/ui/static/ui/js/require_paths.js new file mode 100644 index 00000000..57378def --- /dev/null +++ b/ui/static/ui/js/require_paths.js @@ -0,0 +1,15 @@ +// relative to ui/static/bower +/* jshint ignore:start */ +var REQUIRE_PATHS = { + jquery: "jquery/dist/jquery", + bootstrap: "bootstrap/dist/js/bootstrap", + icheck: "icheck/icheck", + retina: "retina.js/dist/retina", + react: "react/react", + reactaddons: "react/react-with-addons", + lodash: "lodash/lodash", + csrf: "../ui/js/csrf", + listing: "../ui/js/listing", + setup_manage_taxonomies: "../ui/js/manage_taxonomies.jsx?noext" +}; +/* jshint ignore:end */ diff --git a/ui/templates/base.html b/ui/templates/base.html index ae47ba78..de3133e2 100644 --- a/ui/templates/base.html +++ b/ui/templates/base.html @@ -103,6 +103,8 @@ + {% endcompress %} From d553b3a139f4d17c9c3c506f2b9c1c822564f8a0 Mon Sep 17 00:00:00 2001 From: George Schneeloch Date: Thu, 2 Jul 2015 15:04:08 -0400 Subject: [PATCH 08/42] Installed JSXHint and configured JSCS to work with JSX files --- .jscsrc | 5 ++- package.json | 2 ++ tox.ini | 1 + ui/static/ui/js/manage_taxonomies.jsx | 51 ++++++++++++++------------- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/.jscsrc b/.jscsrc index e20d9089..19e68fd3 100644 --- a/.jscsrc +++ b/.jscsrc @@ -1,5 +1,8 @@ { "preset": "google", "validateQuoteMarks": null, - "requireCamelCaseOrUpperCaseIdentifiers": "ignoreProperties" + "requireCamelCaseOrUpperCaseIdentifiers": "ignoreProperties", + "fileExtensions": [".js", ".jsx"], + "esprima": "esprima-fb", + "disallowSpacesInAnonymousFunctionExpression": null } diff --git a/package.json b/package.json index 68b59e85..ed65a158 100644 --- a/package.json +++ b/package.json @@ -7,8 +7,10 @@ "devDependencies": { "bower": "1.4.1", "coveralls": "~2.11.2", + "esprima-fb": "^15001.1.0-dev-harmony-fb", "jscs": "^1.13.1", "jshint": "^2.8.0", + "jsxhint": "^0.15.1", "karma": "^0.12.36", "karma-cli": "0.0.4", "karma-coverage": "^0.4.2", diff --git a/tox.ini b/tox.ini index 00d6f371..73f330e5 100644 --- a/tox.ini +++ b/tox.ini @@ -26,6 +26,7 @@ commands= commands = npm install --prefix {toxinidir} {toxinidir}/node_modules/.bin/jshint ui/static/ui/js ui/jstests/ + {toxinidir}/node_modules/.bin/jsxhint 'ui/**/*.jsx' --jsx-only {toxinidir}/node_modules/.bin/jscs ui/static/ui/js ui/jstests/ {toxinidir}/node_modules/.bin/karma start {toxinidir}/util/convert_lcov_to_coveralls.js {toxinidir}/coverage/coverage-js.lcov {toxinidir}/coverage/coverage-js.json diff --git a/ui/static/ui/js/manage_taxonomies.jsx b/ui/static/ui/js/manage_taxonomies.jsx index 1dc1697e..f1a07399 100644 --- a/ui/static/ui/js/manage_taxonomies.jsx +++ b/ui/static/ui/js/manage_taxonomies.jsx @@ -1,4 +1,5 @@ -define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) { +define('setup_manage_taxonomies', ['reactaddons', 'lodash', 'jquery'], + function (React, _, $) { 'use strict'; var API_ROOT_VOCAB_URL; @@ -59,7 +60,7 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) }), contentType: "application/json; charset=utf-8" } - ).fail(function(e) { + ).fail(function() { thiz.props.reportError("Error occurred while adding new term."); }) .done(function(newTerm) { @@ -73,13 +74,13 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) }); }, updateAddTermText: function(e) { - this.setState({newTermLabel: e.target.value}) + this.setState({newTermLabel: e.target.value}); }, getInitialState: function() { return { newTermLabel: "", terms: this.props.terms - } + }; } }); @@ -128,7 +129,7 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) vocabulary_type: 'm', required: false, weight: 2147483647 - } + }; }, updateVocabularyType: function(e) { this.setState({vocabulary_type: e.target.value}); @@ -143,13 +144,15 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) vocabulary_type: this.state.vocabulary_type, required: this.state.required, weight: this.state.weight - } + }; $.ajax({ - type: "POST", - url: API_ROOT_VOCAB_URL, - data: vocabularyData + type: "POST", + url: API_ROOT_VOCAB_URL, + data: vocabularyData }).fail(function(data) { - thiz.setState({errorMessage: 'There was a problem adding the Vocabulary'}) + thiz.setState({ + errorMessage: 'There was a problem adding the Vocabulary' + }); console.error(data); }).done(function(data) { // Reset state (and eventually update the vocab tab @@ -161,18 +164,17 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) ".cd-panel-2 .cd-panel-container .cd-panel-content" ); scrollableDiv.animate( - { scrollTop: scrollableDiv.prop('scrollHeight')}, + {scrollTop: scrollableDiv.prop('scrollHeight')}, 500 - ); + ); }); }, render: function() { - var repoSlug = this.props.repoSlug; - var errorBox = null - if(this.state['errorMessage'] !== undefined) { + var errorBox = null; + if (this.state.errorMessage !== undefined) { errorBox =
{this.state.errorMessage} -
+ ; } return (
@@ -209,7 +211,7 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _)

- ) + ); } }); @@ -217,20 +219,20 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) getInitialState: function() { return { vocabularies: this.props.vocabularies - } + }; }, addVocabulary: function(vocab) { // Wrap vocab in expected structure - var new_vocab = { + var newVocab = { terms: [], vocabulary: vocab }; var vocabularies = this.state.vocabularies; - vocabularies.push(new_vocab); + vocabularies.push(newVocab); this.setState({vocabularies: vocabularies}); }, render: function() { - return( + return (
- ) + ); } }); return function (repoSlug) { - API_ROOT_VOCAB_URL = '/api/v1/repositories/' + repoSlug + '/vocabularies/' + API_ROOT_VOCAB_URL = '/api/v1/repositories/' + repoSlug + '/vocabularies/'; $.get(API_ROOT_VOCAB_URL) .then(function (data) { var promises = _.map(data.results, function (vocabulary) { @@ -264,8 +266,7 @@ define('setup_manage_taxonomies', ['reactaddons', 'lodash'], function (React, _) var args; if (promises.length === 1) { args = [arguments[0]]; - } - else { + } else { args = arguments; } return _.map(args, function (obj) { From 96efe0f3498362e96441e08bec71d2d160972835 Mon Sep 17 00:00:00 2001 From: Amir Qayyum Khan Date: Fri, 3 Jul 2015 18:13:50 +0500 Subject: [PATCH 09/42] fix css to make list vertical align --- ui/static/ui/css/mit-lore.css | 7 +++++ ui/templates/includes/facet_panel.html | 42 ++++++++++++++------------ 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/ui/static/ui/css/mit-lore.css b/ui/static/ui/css/mit-lore.css index ebc1e243..7d995650 100644 --- a/ui/static/ui/css/mit-lore.css +++ b/ui/static/ui/css/mit-lore.css @@ -169,6 +169,13 @@ a { margin:0 0 10px 0; } +.icheck-list li a span.min-width { + min-width: 30px; + text-align: right; + display: inline-block; + margin-right: 5px; +} + .icheck-list label { font-weight:400; font-size:14px; diff --git a/ui/templates/includes/facet_panel.html b/ui/templates/includes/facet_panel.html index eaa25cac..bd7b051a 100644 --- a/ui/templates/includes/facet_panel.html +++ b/ui/templates/includes/facet_panel.html @@ -64,26 +64,28 @@

{% for resource_type in facets.fields.resource_type %}
  • - {# Select image based on type #} - {% if resource_type.0 == 'chapter' %} - - {% elif resource_type.0 == 'sequential' %} - - {% elif resource_type.0 == 'vertical' %} - - {% elif resource_type.0 == 'problem' %} - - {% elif resource_type.0 == 'video' %} - - {% else %} - - {% endif %} + + {# Select image based on type #} + {% if resource_type.0 == 'chapter' %} + + {% elif resource_type.0 == 'sequential' %} + + {% elif resource_type.0 == 'vertical' %} + + {% elif resource_type.0 == 'problem' %} + + {% elif resource_type.0 == 'video' %} + + {% else %} + + {% endif %} + {{ resource_type.0|title }} {{ resource_type.1 }} From e9acc92990cdd4aa1e37d3879fdcbe3e7fa7c659 Mon Sep 17 00:00:00 2001 From: George Schneeloch Date: Mon, 6 Jul 2015 11:14:32 -0400 Subject: [PATCH 10/42] Removed unused patterns to limit memory use --- karma.conf.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/karma.conf.js b/karma.conf.js index 3879639e..70a2efce 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -40,14 +40,6 @@ module.exports = function(config) { { pattern:'ui/jstests/**/*.jsx', included: false - }, - { - pattern: 'node_modules/**/*.js', - included: false - }, - { - pattern: 'node_modules/**/*.jsx', - included: false } ], From 66f74e23fa7e9f63835a9e1b1cbd5ee9745ade93 Mon Sep 17 00:00:00 2001 From: pwilkins Date: Tue, 23 Jun 2015 16:58:09 -0400 Subject: [PATCH 11/42] Parse static assets from LearningResource Parse static assets from learningresource content_xml field. --- importer/api/__init__.py | 68 +++++++++++++++++++++++++++++++++-- importer/tests/test_import.py | 28 +++++++++++++-- test_requirements.txt | 1 + 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/importer/api/__init__.py b/importer/api/__init__.py index 7d4c30a6..1099645f 100644 --- a/importer/api/__init__.py +++ b/importer/api/__init__.py @@ -42,8 +42,6 @@ def import_course_from_file(filename, repo_id, user_id): None Raises: ValueError: Unable to extract or read archive contents. - - """ tempdir = mkdtemp() @@ -167,3 +165,69 @@ def import_static_assets(path, course): # Remove base path from file name django_file.name = join(root, name).replace(path + sep, '', 1) create_static_asset(course.id, django_file) + + +def parse_static(learning_resource): + """ + Parse static assets from LearningResource XML. + + learning_resource_types can be 'html', 'problem', or 'video'. + + Args: + learning_resource (LearningResource): + + Returns: + List of green, slimy things + """ + tree = etree.fromstring(learning_resource.content_xml) + # expecting only one sub attribute in video LR. + if learning_resource.learning_resource_type.name == "video": + sub = tree.xpath("/video/@sub") + # convert sub into filename + if len(sub) > 0: + filename = _subs_filename(sub) + log.info('subtitle filename is %s'.format(filename)) + + elif learning_resource.learning_resource_type.name == "html": + tree.xpath("/div/") + + elif learning_resource.learning_resource_type.name == "problem": + pass + + +def _subs_filename(subs_id, lang='en'): + """ + Generate proper filename for storage. + + Function copied from: + edx-platform/common/lib/xmodule/xmodule/video_module/transcripts_utils.py + + Args: + subs_id (str): Subs id string + lang (str): Locale language (optional) default: en + + Returns: + filename (str): Filename of subs file + """ + if lang == 'en': + return u'subs_{0}.srt.sjson'.format(subs_id) + else: + return u'{0}_subs_{1}.srt.sjson'.format(lang, subs_id) + + +def _parse_relative_asset_path(path): + """ + Extract path to static asset file from relative edX path + + Static assets whose location are relative to edX datastore such as + ``/c4x/edX/DemoX/asset/images_logic_gate_image.png`` can be + converted to a local path by extracting the portion of the path + after ``asset/``. + + Args: + path (str): + + Returns: + Path to asset relative to ``static`` directory + """ + return path.split('/asset/')[1] diff --git a/importer/tests/test_import.py b/importer/tests/test_import.py index 2a4db618..9c3da947 100644 --- a/importer/tests/test_import.py +++ b/importer/tests/test_import.py @@ -8,6 +8,7 @@ from shutil import rmtree from tempfile import mkstemp, mkdtemp import zipfile +import logging from django.core.files.storage import default_storage from django.core.files.base import ContentFile @@ -16,13 +17,21 @@ from importer.api import ( import_course_from_file, import_course_from_path, - import_static_assets + import_static_assets, + parse_static ) from importer.tasks import import_file from learningresources.api import get_resources -from learningresources.models import Course, StaticAsset, static_asset_basepath +from learningresources.models import ( + Course, + StaticAsset, + static_asset_basepath, + LearningResource +) from learningresources.tests.base import LoreTestCase +log = logging.getLogger(__name__) + class TestImportToy(LoreTestCase): """ @@ -224,3 +233,18 @@ def test_static_import_integration(self): asset.asset.name.replace(base_path, ''), ['test.txt', 'subdir/subtext.txt'] ) + + def test_parse_static(self): + """ + Parse the static assets in the sample course + """ + import_course_from_file(self.course_zip, self.repo.id, self.user.id) + resources = LearningResource.objects.filter( + course__course_number='toy' + ) + self.assertEqual( + resources.count(), + 5 + ) + for item in resources: + parse_static(item) diff --git a/test_requirements.txt b/test_requirements.txt index 850157cd..77d7ded0 100644 --- a/test_requirements.txt +++ b/test_requirements.txt @@ -9,6 +9,7 @@ pytest-capturelog pytest-watch pytest-xdist mock +ipdb ipython urltools semantic_version From f9a82e222fccdc3270c736c8da7b99572ff0d187 Mon Sep 17 00:00:00 2001 From: Shawn Milochik Date: Mon, 6 Jul 2015 11:56:39 -0400 Subject: [PATCH 12/42] Linked video transcripts to learning resources. Closes #230. --- importer/api/__init__.py | 112 ++--------- importer/tests/test_import.py | 74 ++++--- learningresources/api.py | 82 ++++++-- learningresources/models.py | 21 ++ learningresources/tests/base.py | 3 + learningresources/tests/test_api.py | 16 +- learningresources/tests/test_utils.py | 45 +++++ .../toy/static/subs_CCxmtcICYNc.srt.sjson | 185 ++++++++++++++++++ .../testdata/courses/toy/video/Welcome.xml | 2 +- 9 files changed, 396 insertions(+), 144 deletions(-) create mode 100644 learningresources/tests/test_utils.py create mode 100644 learningresources/tests/testdata/courses/toy/static/subs_CCxmtcICYNc.srt.sjson diff --git a/importer/api/__init__.py b/importer/api/__init__.py index 1099645f..560f8db7 100644 --- a/importer/api/__init__.py +++ b/importer/api/__init__.py @@ -8,7 +8,7 @@ import logging from tempfile import mkdtemp from os.path import join, exists, isdir -from os import listdir, walk, sep +from os import listdir from archive import Archive, ArchiveException from django.core.files import File @@ -17,10 +17,10 @@ from xbundle import XBundle, DESCRIPTOR_TAGS from learningresources.api import ( - create_course, - create_resource, - create_static_asset + create_course, create_resource, import_static_assets, + create_static_asset, get_video_sub, ) +from learningresources.models import StaticAsset, course_asset_basepath log = logging.getLogger(__name__) @@ -93,14 +93,12 @@ def import_course_from_path(path, repo_id, user_id): """ bundle = XBundle() bundle.import_from_directory(path) - course = import_course(bundle, repo_id, user_id) static_dir = join(path, 'static') - if isdir(static_dir): - import_static_assets(static_dir, course) + course = import_course(bundle, repo_id, user_id, static_dir) return course -def import_course(bundle, repo_id, user_id): +def import_course(bundle, repo_id, user_id, static_dir): """ Import a course from an XBundle object. @@ -108,6 +106,7 @@ def import_course(bundle, repo_id, user_id): bundle (xbundle.XBundle): Course as xbundle XML repo_id (int): Primary key of repository course belongs to user_id (int): Primary key of Django user doing the import + static_dir (unicode): location of static files Returns: learningresources.models.Course """ @@ -119,6 +118,7 @@ def import_course(bundle, repo_id, user_id): run=src.attrib["semester"], user_id=user_id, ) + import_static_assets(course, static_dir) import_children(course, src, None) return course @@ -142,92 +142,16 @@ def import_children(course, element, parent): content_xml=etree.tostring(element), mpath=mpath, ) + if element.tag == "video": + subname = get_video_sub(element) + if subname != "": + assets = StaticAsset.objects.filter( + course__id=resource.course_id, + asset=course_asset_basepath(course, subname), + ) + for asset in assets: + resource.static_assets.add(asset) + for child in element.getchildren(): if child.tag in DESCRIPTOR_TAGS: import_children(course, child, resource) - - -def import_static_assets(path, course): - """ - Upload all assets and create model records of them for a given - course and path. - - Args: - path (unicode): course specific path to extracted OLX tree. - course (learningresources.models.Course): Course to add assets to. - Returns: - None - """ - for root, _, files in walk(path): - for name in files: - with open(join(root, name), 'r') as open_file: - django_file = File(open_file) - # Remove base path from file name - django_file.name = join(root, name).replace(path + sep, '', 1) - create_static_asset(course.id, django_file) - - -def parse_static(learning_resource): - """ - Parse static assets from LearningResource XML. - - learning_resource_types can be 'html', 'problem', or 'video'. - - Args: - learning_resource (LearningResource): - - Returns: - List of green, slimy things - """ - tree = etree.fromstring(learning_resource.content_xml) - # expecting only one sub attribute in video LR. - if learning_resource.learning_resource_type.name == "video": - sub = tree.xpath("/video/@sub") - # convert sub into filename - if len(sub) > 0: - filename = _subs_filename(sub) - log.info('subtitle filename is %s'.format(filename)) - - elif learning_resource.learning_resource_type.name == "html": - tree.xpath("/div/") - - elif learning_resource.learning_resource_type.name == "problem": - pass - - -def _subs_filename(subs_id, lang='en'): - """ - Generate proper filename for storage. - - Function copied from: - edx-platform/common/lib/xmodule/xmodule/video_module/transcripts_utils.py - - Args: - subs_id (str): Subs id string - lang (str): Locale language (optional) default: en - - Returns: - filename (str): Filename of subs file - """ - if lang == 'en': - return u'subs_{0}.srt.sjson'.format(subs_id) - else: - return u'{0}_subs_{1}.srt.sjson'.format(lang, subs_id) - - -def _parse_relative_asset_path(path): - """ - Extract path to static asset file from relative edX path - - Static assets whose location are relative to edX datastore such as - ``/c4x/edX/DemoX/asset/images_logic_gate_image.png`` can be - converted to a local path by extracting the portion of the path - after ``asset/``. - - Args: - path (str): - - Returns: - Path to asset relative to ``static`` directory - """ - return path.split('/asset/')[1] diff --git a/importer/tests/test_import.py b/importer/tests/test_import.py index 9c3da947..16b12830 100644 --- a/importer/tests/test_import.py +++ b/importer/tests/test_import.py @@ -4,6 +4,7 @@ from __future__ import unicode_literals +from collections import namedtuple import os from shutil import rmtree from tempfile import mkstemp, mkdtemp @@ -18,15 +19,11 @@ import_course_from_file, import_course_from_path, import_static_assets, - parse_static ) from importer.tasks import import_file from learningresources.api import get_resources from learningresources.models import ( - Course, - StaticAsset, - static_asset_basepath, - LearningResource + Course, StaticAsset, static_asset_basepath, LearningResource, ) from learningresources.tests.base import LoreTestCase @@ -140,7 +137,7 @@ def test_import_course_from_path(): with mock.patch('importer.api.import_course') as mock_import: with mock.patch( 'importer.api.import_static_assets' - ) as mock_static: + ): with mock.patch('importer.api.XBundle') as mock_bundle: with mock.patch('importer.api.isdir') as mock_is_dir: mock_import.return_value = True @@ -149,10 +146,8 @@ def test_import_course_from_path(): test_path, test_repo_id, test_user_id ) mock_import.assert_called_with( - mock_bundle(), test_repo_id, test_user_id - ) - mock_static.assert_called_with( - os.path.join(test_path, 'static'), True + mock_bundle(), test_repo_id, + test_user_id, os.path.join(test_path, "static"), ) def test_import_static_assets(self): @@ -166,7 +161,7 @@ def test_import_static_assets(self): with open(os.path.join(temp_dir_path, basename), 'w') as temp: temp.write(file_contents) # All setup, now import - import_static_assets(temp_dir_path, self.course) + import_static_assets(self.course, temp_dir_path) assets = StaticAsset.objects.filter(course=self.course) self.assertEqual(assets.count(), 1) asset = assets[0] @@ -196,7 +191,7 @@ def test_import_static_recurse(self): temp.write(file_contents) # All setup, now import - import_static_assets(temp_dir_path, self.course) + import_static_assets(self.course, temp_dir_path) assets = StaticAsset.objects.filter(course=self.course) self.assertEqual(assets.count(), 1) asset = assets[0] @@ -226,25 +221,60 @@ def test_static_import_integration(self): assets = StaticAsset.objects.filter(course=course) for asset in assets: self.addCleanup(default_storage.delete, asset.asset) - self.assertEqual(assets.count(), 2) + self.assertEqual(assets.count(), 3) for asset in assets: base_path = static_asset_basepath(asset, '') self.assertIn( asset.asset.name.replace(base_path, ''), - ['test.txt', 'subdir/subtext.txt'] + [ + 'test.txt', 'subdir/subtext.txt', + 'subs_CCxmtcICYNc.srt.sjson' + ] ) def test_parse_static(self): """ Parse the static assets in the sample course """ - import_course_from_file(self.course_zip, self.repo.id, self.user.id) - resources = LearningResource.objects.filter( - course__course_number='toy' + def get_counts(): + """Returns counts of resources, videos, and assets.""" + counts = namedtuple("counts", "resources videos assets") + kwargs = {"course__course_number": "toy"} + resources = LearningResource.objects.filter(**kwargs).count() + assets = StaticAsset.objects.filter(**kwargs).count() + kwargs["learning_resource_type__name"] = "video" + videos = LearningResource.objects.filter(**kwargs).count() + return counts(resources, videos, assets) + + # There should be nothing. + counts = get_counts() + self.assertTrue(counts.resources == 0) + self.assertTrue(counts.videos == 0) + self.assertTrue(counts.assets == 0) + # Import the course. + import_course_from_file( + self.get_course_single_tarball(), + self.repo.id, self.user.id ) - self.assertEqual( - resources.count(), - 5 + # There should be something. + counts = get_counts() + self.assertTrue(counts.resources == 5) + self.assertTrue(counts.videos == 2) + self.assertTrue(counts.assets == 3) + # Only one video in the course has subtitles. + self.assertTrue( + StaticAsset.objects.filter( + learningresource__course__id__isnull=False).count() == 1) + + # There should be a single static asset. + course = Course.objects.all().order_by("-id")[0] # latest course + videos = LearningResource.objects.filter( + learning_resource_type__name="video", + course__id=course.id ) - for item in resources: - parse_static(item) + self.assertTrue(videos.count() == 2) + num_assets = sum([ + video.static_assets.count() + for video in videos + ]) + self.assertTrue(num_assets == 1) diff --git a/learningresources/api.py b/learningresources/api.py index ee852d2f..6b813337 100644 --- a/learningresources/api.py +++ b/learningresources/api.py @@ -6,8 +6,11 @@ from __future__ import unicode_literals import logging +from os import walk, sep +from os.path import join from django.contrib.auth.models import User +from django.core.files import File from django.db import transaction from guardian.shortcuts import get_objects_for_user, get_perms @@ -222,30 +225,71 @@ def get_resource(resource_id, user_id): return resource -def create_static_asset(course_id, file_handle): +def create_static_asset(course_id, handle): """ - Create a static asset for a given course. + Create a static asset. + Args: + course_id (int): learningresources.models.Course pk + handle (django.core.files.File): file handle + Returns: + learningresources.models.StaticAsset + """ + with transaction.atomic(): + return StaticAsset.objects.create(course_id=course_id, asset=handle) - Warning: - This takes and open file handle and does not close it. You - will need to handle the opening and closing the - ``file_handle`` outside of this method. - Raises: - ValueError: If a closed handle is passed in +def _subs_filename(subs_id, lang='en'): + """ + Generate proper filename for storage. + + Function copied from: + edx-platform/common/lib/xmodule/xmodule/video_module/transcripts_utils.py Args: - course_id (int): Primary key of the Course to add the asset to. - file_handle (django.core.files.File): - An open file handle to save to the model. + subs_id (str): Subs id string + lang (str): Locale language (optional) default: en Returns: - learningresources.models.StaticAsset + filename (str): Filename of subs file """ - course = Course.objects.get(id=course_id) - with transaction.atomic(): - static_asset, _ = StaticAsset.objects.get_or_create( - course=course, - asset=file_handle - ) - return static_asset + if lang in ('en', "", None): + return u'subs_{0}.srt.sjson'.format(subs_id) + else: + return u'{0}_subs_{1}.srt.sjson'.format(lang, subs_id) + + +def get_video_sub(xml): + """ + Get subtitle IDs from