Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditionally enable Taxoman for Ficus #201

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions cms/djangoapps/contentstore/courseware_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@
log = logging.getLogger('edx.modulestore')


# Interim code to get courseware_index to work with Taxoman

Choose a reason for hiding this comment

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

If this is interim what is the eventual strategy?

Copy link
Author

Choose a reason for hiding this comment

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

@bryanlandia Good question. What I see as the eventual strategy generally follows the following:

A) Understand/evaluate how Hawthorn does plugins and see if that will aid in including taxoman. See: https://github.com/edx/edx-platform/tree/master/openedx/core/djangoapps/plugins

How far will this go to help integrate custom facets into the course discovery built into edx-platform? We answer this question and see

B) Generalize Taxoman integration in edx-platform and edx-search so that
B.1 edx-platform and edx-search optionally accept a vender generic custom facet provider/handler
B.2 Make Taxoman have an interface to meet this
B.3 Document how the interface works so that it is open and others can write custom facet handlers using the same means

Then we've got something that should be upstreamable (after making sure we also have appropriate tests and documentation)

if hasattr(settings,'FEATURES') and settings.FEATURES.get('ENABLE_TAXOMAN', False):
try:
from taxoman_api.models import Facet, FacetValue, CourseFacetValue
using_taxoman = True
except ImportError:
log.error('Taxoman enabled, but unable to import taxoman_api package (ImportError')
using_taxoman = False
else:
using_taxoman = False


def strip_html_content_to_text(html_content):
""" Gets only the textual part for html content - useful for building text to be searched """
# Removing HTML-encoded non-breaking space characters
Expand Down Expand Up @@ -527,11 +539,31 @@ def from_course_mode(self, **kwargs):

return [mode.slug for mode in CourseMode.modes_for_course(course.id)]

def from_taxoman(self, **kwargs):
'''Fetches the assigned value to the facet in taxoman
'''
if using_taxoman:
course = kwargs.get('course', None)
if not course:
raise ValueError("Context dictionary does not contain expected argument 'course'")
course_facet_value = CourseFacetValue.objects.filter(
course_id=course.id,
facet_value__facet__slug=self.property_name).values_list(
'facet_value__value', flat=True)
return list(course_facet_value)
else:
# Interim hack: return an empty list, which should have a net zero
# effect if not enabling taxoman
return []

# Source location options - either from the course or the about info
FROM_ABOUT_INFO = from_about_dictionary
FROM_COURSE_PROPERTY = from_course_property
FROM_COURSE_MODE = from_course_mode

# Appsembler addition - Interim implementation
FROM_TAXOMAN = from_taxoman


class CourseAboutSearchIndexer(object):
"""
Expand Down Expand Up @@ -573,6 +605,14 @@ class CourseAboutSearchIndexer(object):
AboutInfo("catalog_visibility", AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_PROPERTY),
]

# Appsembler addition
if using_taxoman:
for facet in Facet.objects.all():
print("facet = {}".format(facet))

Choose a reason for hiding this comment

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

Maybe safer to use u"facet = {}".format .... here. Looks like you are using the __unicode__ property of facet here and I think you can get in trouble if that has characters outside of ascii and you are mixing with a format string.

Copy link
Author

Choose a reason for hiding this comment

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

I removed the print statement

Copy link

@bryanlandia bryanlandia Jan 31, 2018

Choose a reason for hiding this comment

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

haha, ok. 👍

ABOUT_INFORMATION_TO_INCLUDE.append(
AboutInfo(facet.slug, AboutInfo.PROPERTY, AboutInfo.FROM_TAXOMAN)
)

@classmethod
def index_about_information(cls, modulestore, course):
"""
Expand Down
2 changes: 2 additions & 0 deletions cms/djangoapps/search_api/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

API_VERSION = 'v0'
24 changes: 24 additions & 0 deletions cms/djangoapps/search_api/api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@

from contentstore.courseware_index import CoursewareSearchIndexer
from opaque_keys.edx.keys import CourseKey
from xmodule.modulestore.django import modulestore

def reindex_course(course_id):
"""
Arguments:
course_id - The course id for a course. This is the 'course_id' property
for the course as returend from:

Choose a reason for hiding this comment

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

quibble: "returned"

Copy link
Author

Choose a reason for hiding this comment

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

Fixed!

<lms-host>/api/courses/v1/courses/

Raises:
InvalidKeyError - if the opaque course

Choose a reason for hiding this comment

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

looks like an unfinished thought here...

Copy link
Author

Choose a reason for hiding this comment

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

Yep. Fixed, thanks!

SearchIndexingError - If the reindexing fails

References
course.py#reindex_course_and_check_access

"""
course_key = CourseKey.from_string(course_id)
with modulestore().bulk_operations(course_key):
return CoursewareSearchIndexer.do_course_reindex(modulestore(),
course_key)
10 changes: 10 additions & 0 deletions cms/djangoapps/search_api/permissions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from rest_framework.permissions import BasePermission


class IsStaffUser(BasePermission):
"""
Allow access to only staff users
"""
def has_permission(self, request, view):
return request.user and request.user.is_active and (
request.user.is_staff or request.user.is_superuser)

Choose a reason for hiding this comment

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

quibble: newline

Copy link
Author

Choose a reason for hiding this comment

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

Added a newline

9 changes: 9 additions & 0 deletions cms/djangoapps/search_api/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from django.conf.urls import url

from . import API_VERSION, views

urlpatterns = [
url(r'^$', views.SearchIndex.as_view(), name='search_api_index'),
url(r'^{}/reindex-course'.format(API_VERSION),
views.CourseIndexer.as_view(), name='reindex-course'),
]
97 changes: 97 additions & 0 deletions cms/djangoapps/search_api/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@

import json

from django.conf import settings
from django.http import HttpResponse

from rest_framework.authentication import (
BasicAuthentication,
SessionAuthentication,
TokenAuthentication,
)
from rest_framework.decorators import api_view, authentication_classes, permission_classes
from django.views.decorators.csrf import csrf_exempt
from django.views.decorators.http import require_http_methods
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView

from opaque_keys import InvalidKeyError

from . import api
from .permissions import IsStaffUser

# Restrict access to the LMS server
ALLOWED_ORIGIN = settings.LMS_BASE

Choose a reason for hiding this comment

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

This will be an issue with sites using multiple domains, via Site. I suggest changing this to do a lookup of Site.objects.all() and use the domains returned, and then append settings.LMS_BASE just to be sure.

Copy link
Author

Choose a reason for hiding this comment

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

@bryanlandia Great catch!

Right now, this is only an issue if we install Taxoman on multi-site instances since right now search_api is only used by Taxoman. So for now I'll comment inline that this is broken for multi-site instances and create an issue/card to address it

Choose a reason for hiding this comment

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

OK, we are not going to worry about it at this point, since the only customer using at this point doesn't have any additional domains (except Preview, but that's not a worry). This will be good to fix before upstreaming. I'll file an Issue.

Copy link
Author

Choose a reason for hiding this comment

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

I created an issue: #202


description = """
Appembler Open edX search api.
Opens up access to Open edX'sa search infrastructure via HTTP (REST) API interfaces.
"""

class SearchIndex(APIView):
authentication_classes = (
BasicAuthentication,
SessionAuthentication,
TokenAuthentication
)

permission_classes = ( IsAuthenticated, IsStaffUser, )
def get(self, request, format=None):
return Response({
'message': 'CMS Search API',
})


class CourseIndexer(APIView):
authentication_classes = (
BasicAuthentication,
SessionAuthentication,
TokenAuthentication
)

permission_classes = ( IsAuthenticated, IsStaffUser, )

def get(self, request, format=None):
return Response({
'message': 'Course Indexer',
})

def post(self, request, format=None):

request_data = json.loads(request.body)
course_id = request_data.get('course_id')
try:
results = api.reindex_course(course_id)
response_data = {
'course_id': course_id,
'status': 'OK',
'message': 'course reindex initiated',
'results': results,
}
response = Response(response_data)
response['Access-Control-Allow-Origin'] = ALLOWED_ORIGIN
response['Access-Control-Allow-Methods'] = 'GET, POST, OPTIONS'
response['Access-Control-Allow-Headers'] = '*'
return response
except Exception as e:
if isinstance(e, InvalidKeyError):
message = 'InvalidKeyError: Cannot find key for course string ' + \
'"{}"'.format(course_id)
status = 400
else:
message = 'Exception "{}" msg: {}'.format(e.__class__, e.message)
status = 500
return Response(json.dumps({
'course_id': course_id,
'status': 'ERROR',
'message': message,
}), status=status)

def options(self, request, format=None):
response = Response()
response['Access-Control-Allow-Origin'] = ALLOWED_ORIGIN
response['Access-Control-Allow-Methods'] = 'GET, POST, OPTIONS'
# Options do not allow wildcard for access-control-allow-headers
response['Access-Control-Allow-Headers'] = 'Content-Type'
return response
5 changes: 5 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -959,6 +959,11 @@

# Unusual migrations
'database_fixups',

# Appsembler API Extensions
'rest_framework',
'rest_framework.authtoken',
'search_api',
Copy link

Choose a reason for hiding this comment

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

Can we pull these three items into our appsembler_ settings files to minimize the footprint in core edx-platform?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking that initially but since search_api is in the CMS djangoapps, it requires the rest_framework.authtoken. and it seems weird to have to add an installed app via the appsembler app/edx-configs for an app that lives in edx-platform.

Once we move search_api to a pluggable app, then totally, I agree that we pull it out of common and load it via edx-configs

)


Expand Down
5 changes: 5 additions & 0 deletions cms/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,8 @@
url(r'^404$', handler404),
url(r'^500$', handler500),
)

# Appsembler API extensions
urlpatterns += (
url(r'^api/search/', include('search_api.urls')),

Choose a reason for hiding this comment

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

Are we in danger of colliding with another eventual feature using such a generic name for this api, search?

Copy link
Author

Choose a reason for hiding this comment

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

@bryanlandia Good point. Could be. Same thing could also be said for adding a package called search_api to the CMS. I don't think we can 100% future proof this. Right now this is not in edx/edx-platform/master, so we've got time to rework this ahead if this is implemented in the future by edx.org. Plus we have to customize the CMS to use this functionality unless/until we make this a CMS only plugin or move in a different direction with providing course discovery. Thoughts on this?

Do you have any suggestions for a different path?

)
18 changes: 18 additions & 0 deletions lms/djangoapps/courseware/views/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,19 @@

log = logging.getLogger("edx.courseware")

# Interim code to get courseware views to work with Taxoman
if settings.TAXOMAN_ENABLED:
try:
from taxoman_api.models import Facet
using_taxoman = True
except ImportError:
log.error('Taxoman enabled, but unable to import taxoman_api package (ImportError')
using_taxoman = False
else:
using_taxoman = False




# Only display the requirements on learner dashboard for
# credit and verified modes.
Expand Down Expand Up @@ -144,6 +157,11 @@ def courses(request):
courses_list = []
programs_list = []
course_discovery_meanings = getattr(settings, 'COURSE_DISCOVERY_MEANINGS', {})
if using_taxoman:
for facet in Facet.objects.all():
if not course_discovery_meanings.get(facet.slug):
course_discovery_meanings[facet.slug] = { 'name': facet.name }

if not settings.FEATURES.get('ENABLE_COURSE_DISCOVERY'):
courses_list = get_courses(request.user)

Expand Down
16 changes: 16 additions & 0 deletions lms/envs/aws_appsembler.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@
from .aws import *
from .appsembler import *

if FEATURES.get('ENABLE_TAXOMAN', False):
try:
import taxoman.settings
TAXOMAN_ENABLED = True
except ImportError:
TAXOMAN_ENABLED = False
else:
TAXOMAN_ENABLED = False


ENV_APPSEMBLER_FEATURES = ENV_TOKENS.get('APPSEMBLER_FEATURES', {})
for feature, value in ENV_APPSEMBLER_FEATURES.items():
APPSEMBLER_FEATURES[feature] = value
Expand Down Expand Up @@ -132,3 +142,9 @@
except ImportError:
pass


if TAXOMAN_ENABLED:
WEBPACK_LOADER['TAXOMAN_APP'] = {
'BUNDLE_DIR_NAME': taxoman.settings.bundle_dir_name,
'STATS_FILE': taxoman.settings.stats_file,
}
3 changes: 3 additions & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2024,6 +2024,7 @@

# User API
'rest_framework',
'rest_framework.authtoken',
Copy link
Author

Choose a reason for hiding this comment

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

@OmarIthawi Been thinking of just adding webpack_loader here (and in the pip requirements). edx.org has it in their master branch (assume for Hawthorne) (but they're behind the latest 0.5.0, they're using 0.4.1):

Currently I'm specifying webpack loader in the server-vars

Choose a reason for hiding this comment

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

I've had a very busy week juggling between urgent issues, so I can't do anything extra. I might visit it later next week.

'openedx.core.djangoapps.user_api',

# Shopping cart
Expand Down Expand Up @@ -3042,3 +3043,5 @@
############## Settings for the Enterprise App ######################

ENTERPRISE_ENROLLMENT_API_URL = LMS_ROOT_URL + "/api/enrollment/v1/"

WEBPACK_LOADER = {}
21 changes: 21 additions & 0 deletions lms/envs/devstack_appsembler.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,24 @@
# devstack_appsembler.py

import os

from .devstack import *
from .appsembler import *


if FEATURES.get('ENABLE_TAXOMAN', False):
try:
# Just a check, we don't need it for the settings
import taxoman_api
# We need this for webpack loader
import taxoman.settings
TAXOMAN_ENABLED = True
except ImportError:
TAXOMAN_ENABLED = False
else:
TAXOMAN_ENABLED = False


ENV_APPSEMBLER_FEATURES = ENV_TOKENS.get('APPSEMBLER_FEATURES', {})
for feature, value in ENV_APPSEMBLER_FEATURES.items():
APPSEMBLER_FEATURES[feature] = value
Expand Down Expand Up @@ -123,3 +138,9 @@

# override devstack.py automatic enabling of courseware discovery
FEATURES['ENABLE_COURSE_DISCOVERY'] = ENV_TOKENS['FEATURES'].get('ENABLE_COURSE_DISCOVERY', FEATURES['ENABLE_COURSE_DISCOVERY'])

if TAXOMAN_ENABLED:
WEBPACK_LOADER['TAXOMAN_APP'] = {
'BUNDLE_DIR_NAME': taxoman.settings.bundle_dir_name,
'STATS_FILE': taxoman.settings.stats_file,
}