Content libraries prototype - #5779
Conversation
|
Thanks for the pull request, @bradenmacdonald! I've created OSPR-173 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add information to the ticket which can help Product understand the context for the PR - supporting documentation, edx-code email threads, timeline information ('this must be merged by XX date', and why that is), partner information (this is for a course on edx.org, for example), etc. All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. |
|
@bradenmacdonald I assume this doesn't need any review attention yet; let me know if/when that changes. |
|
Trying to run this in my devstack, I have run into two issues so far. Not sure if others will see the same things since my devstack is pretty old, but wanted to document them.
So far I have no "add library" button, but will dig into that further (stuck at step 5). |
|
@cjshawMIT Thank you for helping to test this! For the issues 1&2, this seem because you are updating an older devstack image, right? Ie, did you had these issues to run the current master too, or just the current branch? For your issue with the 'Add library' missing, I had a quick look at the code, I'll comment inline. |
There was a problem hiding this comment.
@bradenmacdonald Wouldn't that cause the "New library" button to only appear if there are already libraries in the DB? Cf https://github.com/edx/edx-platform/pull/5779/files#diff-968532c6bba75e1f61d1e1ddb51d7842R432
There was a problem hiding this comment.
@antoviaque Yep, good point. That explains @cjshawMIT's issue. Will fix.
There was a problem hiding this comment.
@antoviaque , yes, I think it was johnnycake or pre-johnnycake, not sure. Am trying it again today on kifli. Thanks to both for looking into that!
|
Running on a new kifli devstack, I ran into this npm issue when trying to run |
|
@cjshawMIT The "New Library" button should now show up, if you pull the fix I just pushed. Also, to clarify, the node.js, NPM, and mongo issues are all totally unrelated to libraries, and would apply if updating to the current master as well. |
cc20297 to
2ae05cb
Compare
|
@sarina (cc @dmitchell @antoviaque) I think this is ready for review now. n.b. I have a separate PR that I will open soon that deals with Scope.settings fields and allows courses to override those on library blocks - it turned out to be a complex change so I want to make sure we can review that separately. See the notes under the "Future" heading in this PRs description. |
|
Braden - We've got a WIP PR that touches on scope.settings overrides as well: https://github.com/edx/edx-platform/pull/5802 -- is there some common cause here? When you submit your PR, could you tag @cpennington and @chrisrossi ? Thanks. On Nov 2, 2014, at 3:57 AM, Braden MacDonald notifications@github.com wrote:
|
2ae05cb to
0aa46f0
Compare
|
@bradenmacdonald , I may have done something stupid when trying to rebase this PR, but I now see the + New Library button -- yay! However, I get a missing plugin error when I try to create a new library ... did I miss an install step? This may also be a generic edX platform question, but it seems like the platform does not recognize the new Library entry points in the xmodule/setup.py. They are present in te file -- is there some trick to reloading / refreshing the entry points? Thanks! Error message below: |
|
@cjshawMIT you'll need to reinstall opaque keys and run setup.py. paver should do it for you but may not. Ensure you run either |
|
Thanks, Don, I will try it out as soon as I can get a fresh devstack. It is highly possible this is user error on my part and the paver files take care of everything properly. I am still trying to understand the edX stack and where / how to install various components. —Thanks! |
|
Looks like this pull request needs to be rebased. Can you take care of that, @bradenmacdonald? |
0aa46f0 to
773624e
Compare
|
@cjshawMIT @dmitchell I just confirmed, the normal paver process should set this up properly. @singingwolfboy No problem; rebased now. The conflict was extremely minor - just an adjacent line in requirements/github.txt |
|
@cjshawMIT You should not have run New question answers:
|
|
Hi Braden, thanks for your help -- you hit the nail on the head for both the issues. Apologies for my ignorance with the setup. Working great! |
|
@cjshawMIT You're welcome - thanks a lot for checking this branch out! |
|
@chrisndodge @marcotuts @antoviaque FYI, I just pushed a fix for the other issue seen where |
|
For those interested, implementations of pagination, Scope.settings overrides, and content library permissions are now linked in the "Future" section of this PR's description. I will open PRs for them once this one has been reviewed and merged, so that each one can be reviewed on its own. |
|
BTW, here is the current code coverage report for this PR: I wasn't sure how to find this since the build failed due to pep8/pylint errors. Here is how you can get to the report as long as the "unit" phase passed: |
|
Thanks @cahrens. I noticed however that the coverage report is wrong... for example, it's highlighting the new |
|
I'd recommend you check in with test eng about diff cover issues. I seem to recall that if your unit tests live in CMS but the code you are trying to count coverage for lives in common, the CMS unit tests will not count towards coverage. That may be an issue that you are hitting. |
|
@bradenmacdonald I recommend you verify that all the methods are actually being called in the tests - perhaps by running tests individually and using print statements, or using a debugger and stepping through every function. Sometimes you think things are being executed when they actually aren't. If diff-cover is actually mis-reporting things, I'll help you get in touch with test eng. |
|
Ah, thanks @cahrens, your comment explains it: all of my new tests live in CMS (since libraries are only used within the CMS). I will increase the coverage of the CMS components. I think the changes in common are covered fairly well by the included tests, though they won't show up on coverage reports. BTW, some aspects of this PR won't be covered by tests yet, like permissions and field overrides, since the implementations of those have already been completely changed in the next set of PRs that are waiting on this one. (cc @sarina) |
3e44683 to
1c5c007
Compare
|
I just added some more tests to increase the coverage of this PR (for cms anyways) and rebased it, as there have been a few conflicting changes on master in the meantime. All the tests and quality checks should be passing now, as this now includes the changes from #5814 (ignore TODO in pylint). Edit: Spoke too soon. Apparently 5814 is not applied on the Jenkins server. |
|
Don't neglect Jasmine tests for JavaScript (and bok choy tests where appropriate)-- JavaScript code needs to have test coverage as well. You will find it much easier to write Jasmine tests if you use Backbone models and views. There are many examples in Studio. |
a8237da to
1c5c007
Compare
|
Hey @jzoldak, question for you: does code in |
|
It's preferable to have the coverage local and targeted, but I'm not saying On Mon, Nov 17, 2014 at 10:09 AM, Braden MacDonald <notifications@github.com
|
|
I think it is better to add the tests next to the code. If the code lives On Mon, Nov 17, 2014 at 10:14 AM, Don Mitchell notifications@github.com
|
Background: This is an early-stage prototype implementation of Content Libraries (SOL-11). Once this PR is ready, it is intended to be merged so that it can be used as a common basis for further work on the feature. It is not intended for use on production instances anytime soon, so is disabled by default using a feature flag. Backwards-incompatible/breaking changes may be introduced in the near future.
Discussion: Architecture discussed extensively on the wiki and in meetings, then the revised proposal was presented to the Arch Council on Oct. 21 and given thumbs up. Some open questions remain.
Partner information: 3rd party-hosted open edX instance, for an edX solutions client.
Merge deadline: TBD
Dependencies: Requires openedx/opaque-keys#46, which is already merged.
Status: Demonstrates the very basic features of libraries.
Future: The following will not be part of this PR:
Testing Instructions:
paver install_python_prereqs~/cms.env.jsonand add"ENABLE_CONTENT_LIBRARIES": trueto theFEATURESobject.~/cms.auth.json. Go toMODULESTORE>default>OPTIONS>stores- it should contain two or three entries. Re-order the entries so that the store with"NAME": "split",comes first in thestoreslist. (This makes split the default modulestore for any new courses.)paver devstack studioand go tolocalhost:8001in your browser."library_content"to advanced_modules.library-v1:ProblemX+PR0B. (It's in the URL when you go to the library in studio). Hit Save.localhost:8000. Browse into the course, and you should see that the block is replaced by random blocks from the library. To test the randomization, click the lowest "STAFF DEBUG INFO" link underneath the module, then click "Delete Student State", then refresh the page.