Content libraries settings overrides (SOL-46) by bradenmacdonald · Pull Request #6399 · openedx/openedx-platform

bradenmacdonald · 2014-12-30T09:49:41Z

Background: This PR allows course authors to use blocks from a content library but override any Scope.settings fields such as display_name or weight.
Discussions: Basic idea was presented on the wiki page for content libraries and discussed w/ a few people at edX
Dependencies: Depends on the feature branch https://github.com/edx/edx-platform/commits/content-libraries
Dependents: This is currently blocking work on analytics support ( open-craft#26 ), which depends on these changes.
Sandbox URL: http://sandbox3.opencraft.com:18010/ (LMS at http://sandbox3.opencraft.com/ but shouldn't be needed)
Internal code reviews PRs: open-craft#6 open-craft#25
Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

Author notes:

Capa modules unfortunately store some of their content ("markdown") as Scope.settings, which throws a wrench into the works. For this PR I have added a capa-specific workaround (20d07d2).
There is currently no way to indicate in the studio UI which fields are Scope.settings (can be overridden) and which are Scope.content (should not be overridden as changes will be lost whenever the block is refreshed with the latest version from the library). Implementing such a notice to the user would be a complex task and is outside the scope of this PR.

openedx-webhooks · 2014-12-30T09:49:42Z

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=6399

bradenmacdonald · 2014-12-30T09:53:05Z

@dmitchell I've now opened an "official" PR here... It's essentially the same as what I re-wrote based on your feedback at open-craft#25 . Can you please take one more look and give me the thumbs up or let me know what needs changing?

antoviaque · 2014-12-30T11:31:31Z

@chrisndodge Here is the third PR I was mentioning for review yesterday - could you have a look at it too? Thanks!

chrisndodge · 2014-12-30T20:19:18Z

@antoviaque @bradenmacdonald taking a look now. I'm juggling a few things at the same time, unfortunately...

chrisndodge · 2014-12-30T20:21:38Z

I believe I understand this change, but I'm surprised that it was like this earlier. Do you have any context?

This is part of a method that loads multiple "definition" records at once. I don't think it gets used [very much] during normal use of the split modulestore at the moment, because definitions are generally lazy-loaded one at a time as needed. In an earlier version of this PR's code, I had to sometimes disable the lazy-loading and instead load all the definition records at once, but that was causing strange errors which I eventually tracked down to this bug that has existed in the code since Sep. 23 (1a682da).

I think this bug would no longer affect this branch since I re-wrote it and changed my approach, but it may as well be fixed...

chrisndodge · 2014-12-30T20:23:59Z

Looking at the scope of changes here, I don't think I'm going to be a useful initial reviewer as the changes are pretty deep in modulestore and xblocks. Alas, at this point in time, I only have a "consumer" understanding of these frameworks :-)

Can I propose @dmitchell if he has availability?

chrisndodge · 2014-12-30T20:25:25Z

On a general note, I see Jenkins is reporting missing coverage on the change set. I don't know if there is an opportunity to increase the unit test coverage on the PR, but it would certainly be a recommendation.

bradenmacdonald · 2014-12-30T21:47:24Z

@chrisndodge Thanks for taking a look. I have pushed 9fdf62b892be895df6c08852183a3c031e84cec7 and c431d49f3c1751d1c32ec1c122a7a1eeb06774a6 which should increase the coverage a tiny bit. However, most of the other lines showing as missing coverage come from other PRs - because Jenkins is calculating coverage against a diff with master, not a diff with the libraries feature branch.

dmitchell · 2014-12-31T17:22:40Z

When does it make sense for this to be False? all of your code has it as True.

Yeah, I will just remove that. It is always True.

dmitchell · 2014-12-31T19:05:06Z

Done w/ review. Once you're certain of the open questions 👍

bradenmacdonald · 2015-01-02T20:53:24Z

Test failures are unrelated. Just waiting for further feedback/review.

e-kolpakov · 2015-01-05T10:13:56Z

@bradenmacdonald most likely equal to

orig_descendants = list(self.descendants(dest_structure['blocks'], block_key, depth=None, descendent_map={}))

Ya, I'm not really sure why I put a list comprehension there. It's possible it had a use in a previous version of the code. Thanks.

antoviaque · 2015-01-05T13:12:31Z

Sandbox updated with the latest code: http://sandbox3.opencraft.com:18010/

cahrens · 2015-01-05T14:28:59Z

I created https://openedx.atlassian.net/browse/TNL-1109 for the TNL review.

cahrens · 2015-01-05T20:22:28Z

But if the author refreshes their library block to get the most recent version of the library, their changes will not be kept, correct?

Actually, looking at the bok choy test, I see that my assumption was not correct. The changes specified within the course will be preserved, even after refresh from the source content library.

No that's not correct. When a course author refreshes their library block, any Scope.settings changes they've made (display_name, weight, due_date, etc.) will be kept - that's part of the design goal here.

Any Scope.content changes will be lost. In future, we would like to have a way of preventing course authors from even trying to edit Scope.content fields for blocks brought in from a library, but that's currently not technically feasible.

Should you include a test for the override after refresh? That is, change the library's default and refresh the courses use of it and then do the same fetch assertEqual tests you have here?

cahrens · 2015-01-05T20:41:29Z

It would be good to have a second code reviewer of the common modulestore code (someone who isn't me...).

dmitchell · 2015-01-06T14:32:56Z

Calling this _course confused me for a sec as I assumed it was the course root. idk if it was just me this am.

block1_in_course perhaps?

I've renamed it to problem_in_course in the updated tests which I'm about to push.

dmitchell · 2015-01-06T15:00:52Z

👍

cpennington · 2015-01-06T15:05:14Z

Using inherit here worries me, since there's a risk of conflating it with InheritanceMixin and other similar code.

What would be better? copy_from_template?

Yeah, that would be better.

bradenmacdonald · 2015-01-06T20:13:06Z

Pushed some fixes that address almost all comments so far - just waiting for advice on a better name for inherit_copy.

cahrens · 2015-01-06T20:59:58Z

Some more details about this "intermediate" form would be nice, since you understand it so well now. :)

cahrens · 2015-01-06T21:04:00Z

👍 From TNL perspective, once there is a resolution about "inherited" and "inherit_copy".

bradenmacdonald · 2015-01-06T22:09:22Z

@cpennington Thanks. I renamed inherit_copy to copy_from_template in c005bdb5ee453e29d2fd2f2daa6c9e5f04e13c16, then I squashed this PR's commits. Is this good to go now, assuming the build passes?

(Edit: you may want to revisit this question?)

@cahrens Thanks for the review and the thumbs up!

bradenmacdonald · 2015-01-07T05:45:02Z

Just found a bug so I pushed a fix and a test for it ("test_copy_from_template_publish"). While doing that I moved some of the new tests to their own file as Christina had suggested.

…used in a course

…ir own file

cpennington · 2015-01-07T21:07:24Z

👍

bradenmacdonald · 2015-01-07T21:25:00Z

Thanks everyone! Merging now.

…s-overrides Content libraries settings overrides (SOL-46)

bradenmacdonald force-pushed the content_libraries/10-settings-overrides branch from 241e68d to 7f97429 Compare December 30, 2014 09:56

antoviaque mentioned this pull request Dec 30, 2014

Content libraries settings overrides (SOL-46) open-craft/openedx-platform#25

Closed

chrisndodge reviewed Dec 30, 2014
View reviewed changes

antoviaque mentioned this pull request Dec 31, 2014

Content libraries UI Updates (SOL-80) #6388

Merged

dmitchell reviewed Dec 31, 2014
View reviewed changes

bradenmacdonald force-pushed the content-libraries branch from 16809dd to 76d3f35 Compare January 3, 2015 03:45

bradenmacdonald force-pushed the content_libraries/10-settings-overrides branch from 48b4591 to 05a8716 Compare January 3, 2015 05:03

e-kolpakov reviewed Jan 5, 2015
View reviewed changes

cahrens reviewed Jan 5, 2015
View reviewed changes

dmitchell reviewed Jan 6, 2015
View reviewed changes

cpennington reviewed Jan 6, 2015
View reviewed changes

cahrens reviewed Jan 6, 2015
View reviewed changes

bradenmacdonald force-pushed the content_libraries/10-settings-overrides branch 2 times, most recently from c005bdb to 3e7a8ab Compare January 6, 2015 22:07

bradenmacdonald mentioned this pull request Jan 6, 2015

Library Content - Filter library content by CAPA problem type #6346

Merged

bradenmacdonald force-pushed the content_libraries/10-settings-overrides branch from 3e7a8ab to 9dc86d8 Compare January 6, 2015 23:16

bradenmacdonald added 6 commits January 7, 2015 11:33

Support for overriding Scope.settings values when library content is …

a758958

…used in a course

Unrelated: fix two bugs when duplicating a LibraryContentModule

72e5533

Fix two split mongo bugs that were causing problems...

e2773b9

Workaround an issue with capa modules

a739634

Acceptance test

70e6650

Fix bug w/ publishing ( + test), move copy_from_template tests to the…

697adb1

…ir own file

bradenmacdonald force-pushed the content_libraries/10-settings-overrides branch from 61591ba to 697adb1 Compare January 7, 2015 19:46

bradenmacdonald added a commit that referenced this pull request Jan 7, 2015

Merge pull request #6399 from open-craft/content_libraries/10-setting…

924a7a0

…s-overrides Content libraries settings overrides (SOL-46)

bradenmacdonald merged commit 924a7a0 into openedx:content-libraries Jan 7, 2015

bradenmacdonald deleted the content_libraries/10-settings-overrides branch January 8, 2015 18:56

Uh oh!

Conversation

bradenmacdonald commented Dec 30, 2014

Uh oh!

openedx-webhooks commented Dec 30, 2014

Uh oh!

bradenmacdonald commented Dec 30, 2014

Uh oh!

antoviaque commented Dec 30, 2014

Uh oh!

chrisndodge commented Dec 30, 2014

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!

chrisndodge commented Dec 30, 2014

Uh oh!

chrisndodge commented Dec 30, 2014

Uh oh!

bradenmacdonald commented Dec 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitchell commented Dec 31, 2014

Uh oh!

bradenmacdonald commented Jan 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoviaque commented Jan 5, 2015

Uh oh!

cahrens commented Jan 5, 2015

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!

cahrens commented Jan 5, 2015

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!

dmitchell commented Jan 6, 2015

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!

bradenmacdonald commented Jan 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens commented Jan 6, 2015

Uh oh!

bradenmacdonald commented Jan 6, 2015

Uh oh!

bradenmacdonald commented Jan 7, 2015

Uh oh!

cpennington commented Jan 7, 2015

Uh oh!

bradenmacdonald commented Jan 7, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects