diff --git a/RELEASE.rst b/RELEASE.rst index 1029130e..8a706df7 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,19 @@ Release Notes ------------- +Version 0.13.0 +============== + +- Refactored listing code for testing. +- Implemented lazy loading for resource tab. +- Added custom slugify function to allow any name for Repo, Vocab, Term. +- Fixed index mapping of terms and vocabularies. +- Set Elasticsearch log level higher during testing. +- Set Spinner `zIndex` to 0 to not have it float above everything else. +- Added loader on save. +- Removed unused functions. +- Fixed link click behavior for term edit and delete. + Version 0.12.0 ============== diff --git a/learningresources/migrations/0018_fill_empty_slugs.py b/learningresources/migrations/0018_fill_empty_slugs.py new file mode 100644 index 00000000..582b8dfe --- /dev/null +++ b/learningresources/migrations/0018_fill_empty_slugs.py @@ -0,0 +1,37 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import models, migrations +from django.db.models import F + +from rest.util import default_slugify + +# pylint: skip-file + + +def backfill_empty_slugs(apps, schema_editor): + """ + Finds all the objects in the Repository + that do not have a slug and change the name; + this will automatically create a new slug. + """ + Repository = apps.get_model("learningresources", "Repository") + + for repo in Repository.objects.filter(slug=''): + repo.slug = default_slugify( + repo.name, + Repository._meta.model_name, + lambda slug: Repository.objects.filter(slug=slug).exists() + ) + repo.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('learningresources', '0017_learningresource_missing_title_update'), + ] + + operations = [ + migrations.RunPython(backfill_empty_slugs) + ] diff --git a/learningresources/models.py b/learningresources/models.py index 9347cd7d..ed679726 100644 --- a/learningresources/models.py +++ b/learningresources/models.py @@ -11,11 +11,11 @@ from django.db import models from django.db import transaction from django.contrib.auth.models import User -from django.utils.text import slugify from django.utils.encoding import python_2_unicode_compatible from django.shortcuts import get_object_or_404 from audit.models import BaseModel +from rest.util import default_slugify from roles.api import roles_update_repo from roles.permissions import RepoPermission from lore.settings import LORE_PREVIEW_BASE_URL @@ -171,12 +171,11 @@ def save(self, *args, **kwargs): if self.id is not None: is_update = True old_slug = get_object_or_404(Repository, id=self.id).slug - slug = slugify(self.name) - count = 1 - while Repository.objects.filter(slug=slug).exists(): - slug = "{0}{1}".format(slugify(self.name), count) - count += 1 - self.slug = slug + self.slug = default_slugify( + self.name, + Repository._meta.model_name, + lambda slug: Repository.objects.filter(slug=slug).exists() + ) # check if it's necessary to initialize the permissions new_repository = super(Repository, self).save(*args, **kwargs) if is_update: diff --git a/learningresources/tests/test_models.py b/learningresources/tests/test_models.py index ac82f799..17d3725c 100644 --- a/learningresources/tests/test_models.py +++ b/learningresources/tests/test_models.py @@ -111,6 +111,20 @@ def test_repo_slug(self): self.assertEqual(repo.name, "repo name") self.assertEqual(repo.slug, "repo-name") + # using a weird name + repo.name = "&^%$" + repo.save() + self.assertEqual(repo.name, "&^%$") + self.assertEqual(repo.slug, "repository-slug") + # create a new repo with weird name + repo = Repository.objects.create( + name="****&&", + description="description", + created_by_id=self.user.id, + ) + self.assertEqual(repo.name, "****&&") + self.assertEqual(repo.slug, "repository-slug1") + def test_static_asset_basepath(self): """Verify we are setting the path we expect""" filename = 'asdf/asdf.txt' diff --git a/lore/settings.py b/lore/settings.py index 4e8a65bd..38de21b2 100644 --- a/lore/settings.py +++ b/lore/settings.py @@ -15,7 +15,7 @@ from django.core.exceptions import ImproperlyConfigured import yaml -VERSION = '0.12.0' +VERSION = '0.13.0' CONFIG_PATHS = [ os.environ.get('LORE_CONFIG', ''), @@ -277,6 +277,7 @@ def get_var(name, default): # Logging configuration LOG_LEVEL = get_var('LORE_LOG_LEVEL', 'DEBUG') DJANGO_LOG_LEVEL = get_var('DJANGO_LOG_LEVEL', 'DEBUG') +ES_LOG_LEVEL = get_var('ES_LOG_LEVEL', 'INFO') # For logging to a remote syslog host LOG_HOST = get_var('LORE_LOG_HOST', 'localhost') @@ -339,7 +340,7 @@ def get_var(name, default): 'level': 'INFO', }, 'elasticsearch': { - 'level': 'INFO', + 'level': ES_LOG_LEVEL, }, }, } diff --git a/rest/tests/test_misc.py b/rest/tests/test_misc.py index 305dcbe4..b31fa926 100644 --- a/rest/tests/test_misc.py +++ b/rest/tests/test_misc.py @@ -15,6 +15,7 @@ as_json, ) from rest.pagination import LorePagination +from rest.util import default_slugify class TestMisc(RESTTestCase): @@ -105,3 +106,45 @@ def test_pagination(self): LorePagination.max_page_size - 1, LorePagination.max_page_size - 1 ) + + def test_default_slugify(self): + """ + Test for the default slugify function. + Since this function is generic, here we test the generic call + """ + # a normal string + self.assertEqual( + default_slugify( + 'foo', + 'bar', + lambda x: False + ), + 'foo' + ) + # the normal string already exists + self.assertEqual( + default_slugify( + 'foo', + 'bar', + lambda x: x == 'foo' + ), + 'foo1' + ) + # a string that gives an empty slug + self.assertEqual( + default_slugify( + '%^%$', + 'bar', + lambda x: False + ), + 'bar-slug' + ) + # the default slug already exists + self.assertEqual( + default_slugify( + '%^%$', + 'bar', + lambda x: x == 'bar-slug' + ), + 'bar-slug1' + ) diff --git a/rest/tests/test_search.py b/rest/tests/test_search.py index 15747e93..3d777b9d 100644 --- a/rest/tests/test_search.py +++ b/rest/tests/test_search.py @@ -144,7 +144,7 @@ def test_num_queries(self): Make sure we're not hitting the database for the search more than necessary. """ - with self.assertNumQueries(8): + with self.assertNumQueries(9): self.get_results() def test_sortby(self): @@ -289,7 +289,7 @@ def test_facets(self): term1_results = self.get_results( selected_facets=["{v}_exact:{t}".format( v=vocab_key, - t=term1.slug + t=term1.id )] ) self.assertEqual(1, term1_results['count']) @@ -297,7 +297,7 @@ def test_facets(self): term2_results = self.get_results( selected_facets=["{v}_exact:{t}".format( v=vocab_key, - t=term2.slug + t=term2.id )] ) self.assertEqual(1, term2_results['count']) @@ -385,7 +385,7 @@ def test_facets(self): # Facet count facet_counts = self.get_results( selected_facets=["{v}_exact:{t}".format( - v=vocab_key, t=term1.slug + v=vocab_key, t=term1.id )] )['facet_counts'] self.assertEqual( @@ -439,7 +439,7 @@ def test_facets(self): }, 'values': [{ 'count': 1, - 'key': term1.slug, + 'key': str(term1.id), 'label': term1.label }] } @@ -492,14 +492,14 @@ def test_selected_facets(self): selected_facets = self.get_results( selected_facets=["{v}_exact:{t}".format( - v=vocab_key, t=term1.slug + v=vocab_key, t=term1.id )] )['selected_facets'] self.assertEqual( selected_facets, { '{v}'.format(v=vocab_key): {'{t}'.format( - t=term1.slug): True}, + t=term1.id): True}, 'course': {}, 'resource_type': {}, 'run': {} @@ -510,7 +510,7 @@ def test_selected_facets(self): # we should have no checkboxes that show up. selected_facets = self.get_results( selected_facets=["{v}_exact:{t}".format( - v=vocab_key, t=term1.slug + v=vocab_key, t=term1.id ), "run_exact:doesnt_exist"] )['selected_facets'] self.assertEqual( @@ -546,12 +546,17 @@ def test_empty_term_name(self): vocab_dict['learning_resource_types'] = [ self.resource.learning_resource_type.name ] - vocab_slug = self.create_vocabulary(self.repo.slug, vocab_dict)['slug'] - term_slug = self.create_term(self.repo.slug, vocab_slug, { + vocab_result = self.create_vocabulary(self.repo.slug, vocab_dict) + vocab_slug = vocab_result['slug'] + vocab_id = vocab_result['id'] + vocab_key = make_vocab_key(vocab_id) + + term_result = self.create_term(self.repo.slug, vocab_slug, { "label": "empty", "weight": 4 - })['slug'] - vocab_key = make_vocab_key(vocab_slug) + }) + term_id = term_result['id'] + term_slug = term_result['slug'] # There is one resource and it is missing a term. # Test that a missing search will get one result. @@ -566,7 +571,7 @@ def test_empty_term_name(self): results = self.get_results( selected_facets=["{v}_exact:{t}".format( v=vocab_key, - t=term_slug + t=term_id )] ) self.assertEqual(results['count'], 0) @@ -592,7 +597,7 @@ def test_empty_term_name(self): results = self.get_results( selected_facets=["{v}_exact:{t}".format( v=vocab_key, - t=term_slug + t=term_id )] ) self.assertEqual(results['count'], 1) @@ -645,14 +650,14 @@ def test_name_collision(self): self.assertEqual( self.get_results(selected_facets=["{v}_exact:{t}".format( v=vocab1_key, - t=term1.slug + t=term1.id )])['count'], 0 ) self.assertEqual( self.get_results(selected_facets=["{v}_exact:{t}".format( v=vocab2_key, - t=term2.slug + t=term2.id )])['count'], 1 ) @@ -663,14 +668,14 @@ def test_name_collision(self): self.assertEqual( self.get_results(selected_facets=["{v}_exact:{t}".format( v=vocab1_key, - t=term1.slug + t=term1.id )])['count'], 1 ) self.assertEqual( self.get_results(selected_facets=["{v}_exact:{t}".format( v=vocab2_key, - t=term2.slug + t=term2.id )])['count'], 0 ) diff --git a/rest/tests/test_vocabulary.py b/rest/tests/test_vocabulary.py index e2343157..99d69826 100644 --- a/rest/tests/test_vocabulary.py +++ b/rest/tests/test_vocabulary.py @@ -5,6 +5,7 @@ from __future__ import unicode_literals import logging +from copy import deepcopy from rest_framework.status import ( HTTP_200_OK, @@ -24,7 +25,7 @@ VocabularySerializer, TermSerializer, ) -from taxonomy.models import Vocabulary, Term +from taxonomy.models import Vocabulary, Term, make_vocab_key from learningresources.models import LearningResourceType, LearningResource log = logging.getLogger(__name__) @@ -229,6 +230,29 @@ def test_vocabulary_filter_type(self): self.assertEqual(resp.status_code, HTTP_200_OK) self.assertEqual(0, as_json(resp)['count']) + def test_vocabulary_slug(self): + """ + Special test for the creation of vocabularies with name that can result + in empty slugs + """ + vocab_dict = deepcopy(self.DEFAULT_VOCAB_DICT) + # normal name to verify the default behavior + vocab_dict.update({'name': 'foo-test-one'}) + res_dict = self.create_vocabulary(self.repo.slug, vocab_dict) + self.assertEqual(res_dict['slug'], 'foo-test-one') + # weird name #1 + vocab_dict.update({'name': '%$#@'}) + res_dict = self.create_vocabulary(self.repo.slug, vocab_dict) + self.assertEqual(res_dict['slug'], 'vocabulary-slug') + # weird name #2 + vocab_dict.update({'name': '((**))'}) + res_dict = self.create_vocabulary(self.repo.slug, vocab_dict) + self.assertEqual(res_dict['slug'], 'vocabulary-slug1') + # weird name #3 + vocab_dict.update({'name': '!@#$%^&'}) + res_dict = self.create_vocabulary(self.repo.slug, vocab_dict) + self.assertEqual(res_dict['slug'], 'vocabulary-slug2') + def test_term(self): """Test REST access for term""" vocab1_slug = self.create_vocabulary(self.repo.slug)['slug'] @@ -356,6 +380,35 @@ def test_term(self): self.get_term(self.repo.slug, vocab2_slug, term2['slug'], expected_status=HTTP_403_FORBIDDEN) + def test_term_slug(self): + """ + Special test for the creation of terms with name that can result + in empty slugs + """ + # create a vocabulary + vocab_res = self.create_vocabulary(self.repo.slug) + term_dict = deepcopy(self.DEFAULT_TERM_DICT) + # normal name to verify the default behavior + term_dict.update({'label': 'foo-term-one'}) + res_dict = self.create_term( + self.repo.slug, vocab_res['slug'], term_dict) + self.assertEqual(res_dict['slug'], 'foo-term-one') + # weird name #1 + term_dict.update({'label': '%$#@'}) + res_dict = self.create_term( + self.repo.slug, vocab_res['slug'], term_dict) + self.assertEqual(res_dict['slug'], 'term-slug') + # weird name #2 + term_dict.update({'label': '((**))'}) + res_dict = self.create_term( + self.repo.slug, vocab_res['slug'], term_dict) + self.assertEqual(res_dict['slug'], 'term-slug1') + # weird name #3 + term_dict.update({'label': '!@#$%^&'}) + res_dict = self.create_term( + self.repo.slug, vocab_res['slug'], term_dict) + self.assertEqual(res_dict['slug'], 'term-slug2') + def test_delete_propagation(self): """Test delete propagation""" @@ -668,8 +721,8 @@ def test_index_updates_on_create(self): self.get_results()['facet_counts'][vocab_key]['values']]), [] ) - self.assert_results([], vocab_key, term1.slug) - self.assert_results([], vocab_key, term2.slug) + self.assert_results([], vocab_key, term1.id) + self.assert_results([], vocab_key, term2.id) resource1 = LearningResource.objects.all()[0] resource2 = LearningResource.objects.all()[1] @@ -684,11 +737,11 @@ def test_index_updates_on_create(self): self.assertEqual( sorted([t['key'] for t in self.get_results()['facet_counts'][vocab_key]['values']]), - sorted([term1.slug, term2.slug]) + sorted([str(term1.id), str(term2.id)]) ) - self.assert_results([resource1], vocab_key, term1.slug) - self.assert_results([resource2], vocab_key, term2.slug) + self.assert_results([resource1], vocab_key, term1.id) + self.assert_results([resource2], vocab_key, term2.id) def test_index_updates_on_edit(self): """ @@ -732,8 +785,8 @@ def test_index_updates_on_edit(self): self.get_results()['facet_counts'][vocab_key]['values']]), [] ) - self.assert_results([], vocab_key, term1.slug) - self.assert_results([], vocab_key, term2.slug) + self.assert_results([], vocab_key, term1.id) + self.assert_results([], vocab_key, term2.id) resource1 = LearningResource.objects.all()[0] resource2 = LearningResource.objects.all()[1] @@ -748,10 +801,10 @@ def test_index_updates_on_edit(self): self.assertEqual( sorted([t['key'] for t in self.get_results()['facet_counts'][vocab_key]['values']]), - sorted([term1.slug, term2.slug]) + sorted([str(term1.id), str(term2.id)]) ) - self.assert_results([resource1], vocab_key, term1.slug) - self.assert_results([resource2], vocab_key, term2.slug) + self.assert_results([resource1], vocab_key, term1.id) + self.assert_results([resource2], vocab_key, term2.id) # Vocab is edited to remove all learning resource types which will also # remove all links to terms. @@ -765,8 +818,8 @@ def test_index_updates_on_edit(self): self.get_results()['facet_counts'][vocab_key]['values']]), [] ) - self.assert_results([], vocab_key, term1.slug) - self.assert_results([], vocab_key, term2.slug) + self.assert_results([], vocab_key, term1.id) + self.assert_results([], vocab_key, term2.id) def test_index_updates_on_delete(self): """ @@ -825,20 +878,20 @@ def test_index_updates_on_delete(self): self.assertEqual( sorted([t['key'] for t in self.get_results()['facet_counts'][vocab_key]['values']]), - sorted([term1.slug, term2.slug]) + sorted([str(term1.id), str(term2.id)]) ) - self.assert_results([resource1], vocab_key, term1.slug) - self.assert_results([resource2], vocab_key, term2.slug) + self.assert_results([resource1], vocab_key, term1.id) + self.assert_results([resource2], vocab_key, term2.id) # Term is removed from facet list. self.delete_term(self.repo.slug, vocab.slug, term1.slug) self.assertEqual( sorted([t['key'] for t in self.get_results()['facet_counts'][vocab_key]['values']]), - sorted([term2.slug]) + sorted([str(term2.id)]) ) - self.assert_results([], vocab_key, term1.slug) - self.assert_results([resource2], vocab_key, term2.slug) + self.assert_results([], vocab_key, term1.id) + self.assert_results([resource2], vocab_key, term2.id) self.delete_vocabulary(self.repo.slug, vocab.slug) # No vocabs left. @@ -846,8 +899,8 @@ def test_index_updates_on_delete(self): sorted(["course", "run", "resource_type"]), sorted(self.get_results()['facet_counts'].keys()) ) - self.assert_results([], vocab_key, term1.slug) - self.assert_results([], vocab_key, term2.slug) + self.assert_results([], vocab_key, term1.id) + self.assert_results([], vocab_key, term2.id) def test_vocab_num_queries(self): """Make sure number of queries is reasonable for vocab and term.""" @@ -896,6 +949,133 @@ def test_vocab_num_queries(self): with self.assertNumQueries(30): self.delete_vocabulary(self.repo.slug, vocab_slug) + def test_vocabulary_rename(self): + """Test that index updates properly after a vocabulary rename.""" + vocab_result = self.create_vocabulary(self.repo.slug) + vocab_id = vocab_result['id'] + vocab_key = make_vocab_key(vocab_id) + vocab_slug = vocab_result['slug'] + term_result = self.create_term(self.repo.slug, vocab_slug) + term_id = term_result['id'] + term_slug = term_result['slug'] + term_label = term_result['label'] + + # Update vocabulary for resource type, assign term + self.patch_vocabulary(self.repo.slug, vocab_slug, { + "learning_resource_types": [ + self.resource.learning_resource_type.name + ] + }) + self.patch_learning_resource(self.repo.slug, self.resource.id, { + "terms": [term_slug] + }) + + self.assertEqual( + self.get_results()['facet_counts'][vocab_key]['facet']['label'], + vocab_result['name'] + ) + self.assertEqual( + self.get_results()['facet_counts'][vocab_key]['values'], + [{'count': 1, 'key': str(term_id), 'label': term_label}] + ) + self.assertEqual( + self.get_results(selected_facets=["{v}_exact:{t}".format( + v=vocab_key, + t=term_id + )])['count'], + 1 + ) + + # Rename vocabulary + name = "brand new name" + self.patch_vocabulary(self.repo.slug, vocab_slug, { + "name": name + }) + + # Facet counts should not change + self.assertEqual( + self.get_results()['facet_counts'][vocab_key]['facet']['label'], + name + ) + self.assertEqual( + self.get_results()['facet_counts'][vocab_key]['values'], + [{'count': 1, 'key': str(term_id), 'label': term_label}] + ) + self.assertEqual( + self.get_results(selected_facets=["{v}_exact:{t}".format( + v=vocab_key, + t=term_id + )])['count'], + 1 + ) + + def test_term_rename(self): + """Test that index updates properly after a term rename.""" + vocab_result = self.create_vocabulary(self.repo.slug) + vocab_key = make_vocab_key(vocab_result['id']) + vocab_slug = vocab_result['slug'] + term_result = self.create_term(self.repo.slug, vocab_slug) + term_id = term_result['id'] + term_slug = term_result['slug'] + + # Update vocabulary for resource type, assign term + self.patch_vocabulary(self.repo.slug, vocab_slug, { + "learning_resource_types": [ + self.resource.learning_resource_type.name + ] + }) + self.patch_learning_resource(self.repo.slug, self.resource.id, { + "terms": [term_slug] + }) + + self.assertEqual( + self.get_results()['facet_counts'][vocab_key]['values'], + [{'count': 1, 'key': str(term_id), 'label': term_result['label']}] + ) + self.assertEqual( + self.get_results(selected_facets=["{v}_exact:{t}".format( + v=vocab_key, + t=term_id + )])['count'], + 1 + ) + + # Rename term + label = "brand new label" + self.patch_term(self.repo.slug, vocab_slug, term_slug, { + "label": label + }) + + # Facet counts should not change + self.assertEqual( + self.get_results()['facet_counts'][vocab_key]['values'], + [{'count': 1, 'key': str(term_id), 'label': label}] + ) + self.assertEqual( + self.get_results(selected_facets=["{v}_exact:{t}".format( + v=vocab_key, + t=term_id + )])['count'], + 1 + ) + + def test_empty_vocab_facet_count(self): + """ + Test that we get vocabulary label and not something else for + an empty vocabulary. + """ + vocab_dict = dict(self.DEFAULT_VOCAB_DICT) + name = 'name with spaces' + vocab_dict['name'] = name + vocab_result = self.create_vocabulary(self.repo.slug, vocab_dict) + vocab_id = vocab_result['id'] + vocab_key = make_vocab_key(vocab_id) + + self.assertEqual( + self.get_results()['facet_counts'][vocab_key]['facet']['label'], + name + ) + class TestVocabularyAuthorization(RESTAuthTestCase): """ diff --git a/rest/util.py b/rest/util.py index 6bf3c943..609f0f25 100644 --- a/rest/util.py +++ b/rest/util.py @@ -7,6 +7,7 @@ from django.contrib.auth.models import User from django.http.response import Http404 from django.shortcuts import get_object_or_404 +from django.utils.text import slugify from rest_framework.fields import BooleanField, empty from roles.permissions import BaseGroupTypes @@ -60,3 +61,28 @@ def dispatch(self, request, *args, **kwargs): CheckValidMemberParamMixin, self ).dispatch(request, *args, **kwargs) + + +def default_slugify(label, default_name, exists_func): + """ + Function that extends the Django `slugify` to add a default string in case + the slugified string is empty. + It also takes care of collisions. + NOTE: the `exists_func` must be a valid function to verify the existence + of the slug. The implementation is left to the coder's discretion. + + Args: + label (unicode): label to be slugified + default_name (unicode): default base slug to be used if the slugified + label is an empty string + exists_func (function): function to be used to verify if + a slug is already in use + Returns: + slug (unicode): slug for the label + """ + slug = slugified_str = slugify(label) or '{}-slug'.format(default_name) + count = 1 + while exists_func(slug): + slug = "{0}{1}".format(slugified_str, count) + count += 1 + return slug diff --git a/search/tests/base_es.py b/search/tests/base_es.py index 710539c9..778bc47e 100644 --- a/search/tests/base_es.py +++ b/search/tests/base_es.py @@ -46,9 +46,9 @@ def count_results(self, query=None): """Return count of matching indexed records.""" return self.search(query).count() - def count_faceted_results(self, vocab, term): + def count_faceted_results(self, vocab_id, term_id): """Return count of matching indexed records by facet.""" return search_index( repo_slug=self.repo.slug, - terms={make_vocab_key(vocab): term} + terms={make_vocab_key(vocab_id): term_id} ).count() diff --git a/search/tests/test_es_indexing.py b/search/tests/test_es_indexing.py index 95aa75cc..636bdec8 100644 --- a/search/tests/test_es_indexing.py +++ b/search/tests/test_es_indexing.py @@ -160,15 +160,15 @@ def test_index_vocabulary(self): set_cache_timeout(0) term = self.terms[0] self.assertEqual(self.count_faceted_results( - self.vocabulary.name, term.label), 0) + self.vocabulary.id, term.id), 0) self.resource.terms.add(term) refresh_index() self.assertEqual(self.count_faceted_results( - self.vocabulary.name, term.label), 1) + self.vocabulary.id, term.id), 1) self.resource.terms.remove(term) refresh_index() self.assertEqual(self.count_faceted_results( - self.vocabulary.name, term.label), 0) + self.vocabulary.id, term.id), 0) def test_strip_xml(self): """Indexed content_xml should have XML stripped.""" diff --git a/search/tests/test_indexing.py b/search/tests/test_indexing.py index ae0edd69..448dad52 100644 --- a/search/tests/test_indexing.py +++ b/search/tests/test_indexing.py @@ -62,11 +62,11 @@ def test_index_vocabulary(self): vocab_key = self.vocabulary.index_key set_cache_timeout(0) term = self.terms[0] - self.assertEqual(self.count_faceted_results(vocab_key, term.slug), 0) + self.assertEqual(self.count_faceted_results(vocab_key, term.id), 0) self.resource.terms.add(term) - self.assertEqual(self.count_faceted_results(vocab_key, term.slug), 1) + self.assertEqual(self.count_faceted_results(vocab_key, term.id), 1) self.resource.terms.remove(term) - self.assertEqual(self.count_faceted_results(vocab_key, term.slug), 0) + self.assertEqual(self.count_faceted_results(vocab_key, term.id), 0) def test_strip_xml(self): """Indexed content_xml should have XML stripped.""" diff --git a/search/utils.py b/search/utils.py index 6d8bc85a..71e85efa 100644 --- a/search/utils.py +++ b/search/utils.py @@ -34,13 +34,13 @@ PAGE_LENGTH = 10 -def get_vocab_slugs(repo_slug=None): +def get_vocab_ids(repo_slug=None): """Get all vocabulary names in the database.""" if repo_slug is not None: return Vocabulary.objects.filter( - repository__slug=repo_slug).values_list('slug', flat=True) + repository__slug=repo_slug).values_list('id', flat=True) else: - return Vocabulary.objects.all().values_list('slug', flat=True) + return Vocabulary.objects.all().values_list('id', flat=True) def get_conn(verify=True): @@ -99,23 +99,23 @@ def get_resource_terms(resource_ids): Args: resource_ids (iterable of int): Primary keys of LearningResources Returns: - data (dict): Vocab/term data for course. + data (dict): Vocab/term ids for course. """ - vocab_slugs = get_vocab_slugs() + vocab_ids = get_vocab_ids() resource_data = defaultdict(lambda: defaultdict(list)) rels = LearningResource.terms.related.through.objects.select_related( "term__vocabulary").filter(learningresource__id__in=resource_ids) for rel in rels.iterator(): obj = resource_data[rel.learningresource_id] - obj[rel.term.vocabulary.slug].append(rel.term.slug) + obj[rel.term.vocabulary.id].append(rel.term.id) # Replace the defaultdicts with dicts. info = {k: dict(v) for k, v in resource_data.items()} for resource_id in resource_ids: if resource_id not in info.keys(): info[resource_id] = {} - for vocab_slug in vocab_slugs: - if vocab_slug not in info[resource_id].keys(): - info[resource_id][vocab_slug] = [] + for vocab_id in vocab_ids: + if vocab_id not in info[resource_id].keys(): + info[resource_id][vocab_id] = [] return info @@ -183,9 +183,9 @@ def search_index(tokens=None, repo_slug=None, sort_by=None, terms=None): # Always sort by ID to preserve ordering. search = search.sort(sort_by, "id") - vocabs = set(get_vocab_slugs(repo_slug=repo_slug)) - for vocab in vocabs: - vocab_key = make_vocab_key(vocab) + vocab_ids = set(get_vocab_ids(repo_slug=repo_slug)) + for vocab_id in vocab_ids: + vocab_key = make_vocab_key(vocab_id) search.aggs.bucket( "{key}_missing".format(key=vocab_key), "missing", field=vocab_key @@ -309,8 +309,8 @@ def resource_to_dict(resource, term_info): # Index term info. Since these fields all use the "not_analyzed" # index, they must all be exact matches. - for vocab_slug, term_slugs in term_info.items(): - rec[make_vocab_key(vocab_slug)] = term_slugs + for vocab_id, term_ids in term_info.items(): + rec[make_vocab_key(vocab_id)] = term_ids # If the title is empty, sort it to the bottom. See above. if rec["titlesort"] == "0": @@ -506,14 +506,14 @@ def ensure_vocabulary_mappings(term_info): existing_vocabs = set(mapping.to_dict()["learningresource"]["properties"]) # Get all the taxonomy names from the data. - vocab_slugs = set() - for result in term_info.values(): - for key in result.keys(): - vocab_slugs.add(key) + vocab_ids = set() + for vocab_terms in term_info.values(): + for vocab_id in vocab_terms.keys(): + vocab_ids.add(vocab_id) updated = False # Add vocabulary to mapping if necessary. - for slug in vocab_slugs: - vocab_key = make_vocab_key(slug) + for vocab_id in vocab_ids: + vocab_key = make_vocab_key(vocab_id) if vocab_key in existing_vocabs: continue mapping.field(vocab_key, "string", index="not_analyzed") @@ -566,22 +566,19 @@ def convert_aggregate(agg): vocab_lookup = {} term_lookup = {} - for term in Term.objects.select_related("vocabulary").all(): - vocab = term.vocabulary + for term in Term.objects.all(): + term_lookup[term.id] = term.label - for term in vocab.term_set.all(): - term_lookup[term.slug] = term.label - vocab_lookup[vocab.slug] = vocab.name + for vocab in Vocabulary.objects.all(): + vocab_lookup[make_vocab_key(vocab.id)] = vocab.name def get_vocab_label(vocab_key): """Get label for vocab.""" - if vocab_key.startswith("vocab_"): - vocab_key = vocab_key[len("vocab_"):] return vocab_lookup.get(vocab_key, vocab_key) - def get_term_label(term_slug): + def get_term_label(term_id): """Get label for term.""" - return term_lookup.get(term_slug, term_slug) + return term_lookup.get(int(term_id), str(term_id)) def get_builtin_label(key): """Get label for special types.""" diff --git a/taxonomy/api.py b/taxonomy/api.py index 7a5d56ee..f7eae056 100644 --- a/taxonomy/api.py +++ b/taxonomy/api.py @@ -52,32 +52,3 @@ def get_term(repo_slug, user_id, vocab_slug, term_slug): raise NotFound() except Term.DoesNotExist: raise NotFound() - - -# pylint: disable=too-many-arguments -def create_vocabulary( - user_id, repo_slug, name, description, - required=False, vocabulary_type=Vocabulary.MANAGED, weight=0, - multi_terms=False): - """ - Create a new vocabulary. - Args: - user_id (int): primary key of the User - repo_slug (unicode): slug of the repository - name (unicode): vocab name - description (unicode): vocab description - required (bool): is it required? - vocabulary_type (unicode): on of the Vocabulary.vocabulary_type options - weight (int): vocab weight - multi_terms (bool): multiple terms possible on one LearningResource - Returns: - vocab (Vocabulary) - """ - repo = get_repo(repo_slug, user_id) - vocab = Vocabulary.objects.create( - repository_id=repo.id, - name=name, description=description, - required=required, vocabulary_type=vocabulary_type, - weight=weight, multi_terms=multi_terms - ) - return vocab diff --git a/taxonomy/migrations/0008_fill_empty_slugs.py b/taxonomy/migrations/0008_fill_empty_slugs.py new file mode 100644 index 00000000..40fe31ac --- /dev/null +++ b/taxonomy/migrations/0008_fill_empty_slugs.py @@ -0,0 +1,45 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import models, migrations +from django.db.models import F + +from rest.util import default_slugify + +# pylint: skip-file + + +def backfill_empty_slugs(apps, schema_editor): + """ + Finds all the objects in the Repository + that do not have a slug and change the name; + this will automatically create a new slug. + """ + Vocabulary = apps.get_model("taxonomy", "Vocabulary") + Term = apps.get_model("taxonomy", "Term") + + for vocab in Vocabulary.objects.filter(slug=''): + vocab.slug = default_slugify( + vocab.name, + Vocabulary._meta.model_name, + lambda slug: Vocabulary.objects.filter(slug=slug).exists() + ) + vocab.save() + for term in Term.objects.filter(slug=''): + term.slug = default_slugify( + term.label, + Term._meta.model_name, + lambda slug: Term.objects.filter(slug=slug).exists() + ) + term.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('taxonomy', '0007_vocabulary_multi_terms'), + ] + + operations = [ + migrations.RunPython(backfill_empty_slugs) + ] diff --git a/taxonomy/models.py b/taxonomy/models.py index 6bf27534..6c72b669 100644 --- a/taxonomy/models.py +++ b/taxonomy/models.py @@ -3,7 +3,6 @@ from django.db import models, transaction from django.utils.translation import ugettext_lazy as _ -from django.utils.text import slugify from django.shortcuts import get_object_or_404 from audit.models import BaseModel @@ -12,13 +11,14 @@ LearningResourceType, LearningResource, ) +from rest.util import default_slugify -def make_vocab_key(vocab_slug): +def make_vocab_key(vocab_id): """ Create vocab key used for elasticsearch index mapping. """ - return "vocab_{slug}".format(slug=vocab_slug) + return "vocab_{id}".format(id=vocab_id) class Vocabulary(BaseModel): @@ -59,19 +59,18 @@ def save(self, *args, **kwargs): """Handle slugs.""" if self.id is None or self.name != get_object_or_404( Vocabulary, id=self.id).name: - slug = slugify(self.name) - count = 1 - while Vocabulary.objects.filter(slug=slug).exists(): - slug = "{0}{1}".format(slugify(self.name), count) - count += 1 - self.slug = slug + self.slug = default_slugify( + self.name, + Vocabulary._meta.model_name, + lambda slug: Vocabulary.objects.filter(slug=slug).exists() + ) return super(Vocabulary, self).save(*args, **kwargs) @property def index_key(self): """Key used in elasticsearch index.""" - return make_vocab_key(self.slug) + return make_vocab_key(self.id) class Term(BaseModel): @@ -95,10 +94,9 @@ def save(self, *args, **kwargs): """Handle slugs.""" if self.id is None or self.label != get_object_or_404( Term, id=self.id).label: - slug = slugify(self.label) - count = 1 - while Term.objects.filter(slug=slug).exists(): - slug = "{0}{1}".format(slugify(self.label), count) - count += 1 - self.slug = slug + self.slug = default_slugify( + self.label, + Term._meta.model_name, + lambda slug: Term.objects.filter(slug=slug).exists() + ) return super(Term, self).save(*args, **kwargs) diff --git a/tox.ini b/tox.ini index 71f89c07..85976f7f 100644 --- a/tox.ini +++ b/tox.ini @@ -13,6 +13,7 @@ setenv = CELERY_ALWAYS_EAGER=True HAYSTACK_INDEX=testindex LORE_DB_DISABLE_SSL=True + ES_LOG_LEVEL=WARNING [testenv:docs] basepython = python2.7 diff --git a/ui/jstests/test-learning-resource.jsx b/ui/jstests/test-learning-resource.jsx index 085af541..0387bf46 100644 --- a/ui/jstests/test-learning-resource.jsx +++ b/ui/jstests/test-learning-resource.jsx @@ -176,17 +176,128 @@ define(['QUnit', 'jquery', 'react', 'lodash', 'learning_resources', function (assert) { var done = assert.async(); + var vocab = $.extend({}, selectedVocabulary); + var appendTermSelectedVocabulary = function(term) { + vocab.terms = vocab.terms.concat(term); + }; + + var loadedState; + var setLoadedState = function(loaded) { + loadedState = loaded; + }; + var afterMount = function(component) { var $node = $(React.findDOMNode(component)); var $termSelect = $node.find("select"); assert.equal($termSelect.size(), 1); - done(); + var $options = $termSelect.find("option"); + var values = _.map($options, function(option) { + return $(option).val(); + }); + assert.deepEqual(values, ["easy", "hard"]); + // Add a term via free tagging + + $termSelect + .append($('