Conditionally enable Taxoman for Ficus by johnbaldwin · Pull Request #201 · appsembler/edx-platform

johnbaldwin · 2018-01-23T21:39:22Z

I didn't put effort into abstracting away taxoman inclusions since we need to figure out a much slimmer way to integrate Appsembler specific features into edx-platform, and we also need to march forward with Ginko then Hawthorne when it get released,

What I did do was add conditionals and test that it works in a GCP staging server both with Taxoman enabled and with taxoman disabled and the taxoman and taxoman_api python packages not installed

The Search API app in CMS is mostly unchanged from Dogwood

The search api provides a programmatic interface to course indexing. The driver is to support course reindexing from Taxoma. This commit is just the app. It does not contain the hooks to use the code: * cms/urls.py * cms/envs/common.py

This commit adds required hooks to `cms/urls.py` and `cms/envs/common.py` in order to use the search api

Courseware Index performs the initial indexing. This is what provides the custom facets that the built-in LMS courseware discovery uses

Enables conditional use of Taxoman in devstack and prod/staging environments

johnbaldwin · 2018-01-23T21:40:12Z

(8) Taxoman: update Ficus edx-platform common branch

johnbaldwin · 2018-01-24T15:32:09Z


    # User API
    'rest_framework',
+    'rest_framework.authtoken',


@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):

https://github.com/edx/edx-platform/blob/master/requirements/edx/base.txt#L122

https://github.com/edx/edx-platform/blob/master/lms/envs/common.py#L2100

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

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.

johnbaldwin · 2018-01-24T21:12:24Z

(1) Metacard: Upgrade Taxoman for Ficus for Metalogix

bryanlandia · 2018-01-30T20:08:10Z

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


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.

I removed the print statement

haha, ok. 👍

bryanlandia · 2018-01-30T20:09:28Z

+    """
+    Arguments:
+    	course_id - The course id for a course. This is the 'course_id' property
+    	            for the course as returend from:


quibble: "returned"

bryanlandia · 2018-01-30T20:09:53Z

+    	            	<lms-host>/api/courses/v1/courses/
+
+    Raises:
+    	InvalidKeyError - if the opaque course 


looks like an unfinished thought here...

Yep. Fixed, thanks!

bryanlandia · 2018-01-30T20:11:26Z

+    """
+    def has_permission(self, request, view):
+        return request.user and request.user.is_active and (
+            request.user.is_staff or request.user.is_superuser)


quibble: newline

Added a newline

bryanlandia · 2018-01-30T20:14:24Z

+from .permissions import IsStaffUser
+
+# Restrict access to the LMS server
+ALLOWED_ORIGIN = settings.LMS_BASE


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.

@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

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.

I created an issue: #202

bryanlandia · 2018-01-30T20:21:50Z

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


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

@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?

bryanlandia · 2018-01-30T20:24:45Z

 log = logging.getLogger('edx.modulestore')


+# Interim code to get courseware_index to work with Taxoman


If this is interim what is the eventual strategy?

@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)

johnbaldwin · 2018-02-08T19:25:16Z

@bryanlandia Do you see anything else that prevents this being merged into develop?

bryanlandia · 2018-02-09T02:02:01Z

I think it's good to go, @johnbaldwin

tkeemon

This seems fine as an interim solution, we should definitely look into abstracting some of this into a separate Django package (especially the edxsearch app).

tkeemon · 2018-02-09T07:05:30Z

+    # Appsembler API Extensions
+    'rest_framework',
+    'rest_framework.authtoken',
+    'search_api',


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

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

johnbaldwin added 5 commits January 21, 2018 19:11

Appsembler: Added search api app to CMS

12ab9fa

The search api provides a programmatic interface to course indexing. The driver is to support course reindexing from Taxoma. This commit is just the app. It does not contain the hooks to use the code: * cms/urls.py * cms/envs/common.py

Appsembler: Added hooks for the CMS search api

e73b6a5

This commit adds required hooks to `cms/urls.py` and `cms/envs/common.py` in order to use the search api

Appsembler: added search api to CMS urls.py

e3e0d88

Added support for Taxoman in CMS courseware_index.py

a9f3aaa

Courseware Index performs the initial indexing. This is what provides the custom facets that the built-in LMS courseware discovery uses

Appsembler: Enable conditional use of Taxoman to LMS

6a9954a

Enables conditional use of Taxoman in devstack and prod/staging environments

johnbaldwin requested a review from OmarIthawi January 23, 2018 21:39

johnbaldwin commented Jan 24, 2018

View reviewed changes

johnbaldwin requested a review from tkeemon January 24, 2018 21:01

johnbaldwin changed the title ~~WIP Conditionally enable Taxoman for Ficus~~ Conditionally enable Taxoman for Ficus Jan 24, 2018

bryanlandia self-requested a review January 30, 2018 19:51

bryanlandia suggested changes Jan 30, 2018

View reviewed changes

johnbaldwin mentioned this pull request Jan 31, 2018

CMS Search API does not support multi-site installations #202

Open

bryanlandia approved these changes Feb 9, 2018

View reviewed changes

tkeemon reviewed Feb 9, 2018

View reviewed changes

johnbaldwin merged commit ff1608d into appsembler/ficus/develop Feb 9, 2018

johnbaldwin deleted the appsembler/ficus/feature/taxoman branch February 9, 2018 21:59

		log = logging.getLogger('edx.modulestore')


		# Interim code to get courseware_index to work with Taxoman

Uh oh!

Conversation

johnbaldwin commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnbaldwin commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanlandia Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin commented Feb 8, 2018

Uh oh!

bryanlandia commented Feb 9, 2018

Uh oh!

tkeemon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnbaldwin commented Jan 23, 2018 •

edited

Loading

bryanlandia Jan 31, 2018 •

edited

Loading