Non-persisted studio templates #270

cpennington · 2013-06-26T02:10:46Z

@dmitchell: Please validate commit contents, and then find reviewers.

Builds on #269 (net changes: edx/edx-platform@dhm/incidental-functionality-improvements...dhm/non-persisted-studio-templates)

cahrens · 2013-06-27T17:44:51Z

I checked out the branch and did some interactive testing. Here are the results.

Editing existing Blank Simple Problem, Multiple choice with no changes, Multiple choice where I had previously selected and edited in the Advanced Editor, LaTeX with no changes, LaTeX with previous changes, Blank Advanced Problem. These all worked.
Alphabetical ordering of problem types works.
Bringing up previously created advanced components. They came up, though there is a known issue about editing and saving openended. I did not test actually changing other components once on the branch.
Creating a new Blank Advanced problem, editing and saving it, viewing change. This worked.
New video component has name "Video Title".
Display names for newly created Advanced component are now reasonable. Except for perhaps Video Alpha 1-- I think they don't want the "1".
Indenting of template text in video alpha still a bit funky. It's now 6 spaces per indent, which makes it all somewhat ugly.

I did not test things outside of the unit page.

dmitchell · 2013-06-27T19:02:19Z

I fixed the indent and '1'. Does anyone know what 'version: 1' in the old
yaml meant? I don't see a version setting field.

On Thu, Jun 27, 2013 at 1:45 PM, Christina Roberts <[email protected]

wrote:

I checked out the branch and did some interactive testing. Here are the
results.

Editing existing Blank Simple Problem, Multiple choice with no changes,
Multiple choice where I had previously selected and edited in the Advanced
Editor, LaTeX with no changes, LaTeX with previous changes, Blank Advanced
Problem. These all worked.

Alphabetical ordering of problem types works.

Bringing up previously created advanced components. They came up,
though there is a known issue about editing and saving openended. I did not
test actually changing other components once on the branch.

Creating a new Blank Advanced problem, editing and saving it, viewing
change. This worked.

New video component has name "Video Title".

Display names for newly created Advanced component are now reasonable.
Except for perhaps Video Alpha 1-- I think they don't want the "1".

Indenting of template text in video alpha still a bit funky. It's now 6
spaces per indent, which makes it all somewhat ugly.

I did not test things outside of the unit page.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/270#issuecomment-20142267
.

cahrens · 2013-06-27T19:24:29Z

More interactive testing completed:

No errors when changing grading slider or adding new grade levels.
Can change grace period without error.
No errors when going to the Course Update page.
Can edit course handouts. Known indenting issue remains.
Indenting on template text in course overview still slightly off. First line is not indented, all following lines off by 1.

dmitchell · 2013-06-27T20:01:24Z

Fixed #5 (pending checkin)

On Thu, Jun 27, 2013 at 3:24 PM, Christina Roberts <[email protected]

wrote:

More interactive testing completed:

No errors when changing grading slider or adding new grade levels.

Can change grace period without error.

No errors when going to the Course Update page.

Can edit course handouts. Known indenting issue remains.

Indenting on template text in course overview still slightly off. First
line is not indented, all following lines off by 1.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/270#issuecomment-20148790
.

jzoldak · 2013-06-27T21:13:18Z

cms/djangoapps/contentstore/views/item.py

@@ -73,28 +74,38 @@ def save_item(request):

 @login_required
 @expect_json
-def clone_item(request):
+def create_item(request):


create_item needs unit tests

dmitchell · 2013-06-28T15:43:04Z

@cpennington @cahrens @chrisndodge @jzoldak
Ready for review. I need to rebase it and add a few more unit tests.

cahrens · 2013-06-28T16:57:22Z

cms/djangoapps/contentstore/features/problem-editor.py

            [MAXIMUM_ATTEMPTS, "", False],
            [PROBLEM_WEIGHT, "", False],
+            # Not sure why this is True other than via inspection


I assume this is related to your e-mail:

"I'm not sure where to record this, but the problem metadata is explicitly encoding 'rerandomization' and 'showanswer' when they come from defaults; so, the problem metadata editor thinks they've been explicitly set. I can't ferret out where the metadata is being set right now and it seems inconsequential and likely to be completely changed w/ the xblock/xmodule refactoring as well as using split mongo as the backing store."

Why do you think this is specifically related to problems and rerandomization/showanswer? It seems like this could be a general bug. Rerandomize is "always" at the course level (via Advanced Settings); showanswer is "closed" at the course level. We want users to be able to set defaults there, and with this bug, it seems impossible. (And another question-- it looks like you changed the LMS namespace defaults as well, but we still aren't see the new default values in Advanced Settings).

Further investigation needs to be done so that inheritance can work.

cahrens · 2013-06-28T17:27:04Z

General note-- looks like pep8/pylint warnings are up. Also need to look through the diff coverage report and see what can be filled in.

cahrens · 2013-06-28T17:30:46Z

cms/djangoapps/contentstore/views/component.py

+        component_templates[category].append((
+            component_class.display_name.default or 'Blank',
+            category,
+            False,  # No defaults have markdown (hardcoded current default)


I don't understand this. Markdown is not a general thing among components.

cahrens · 2013-07-03T16:37:13Z

common/lib/xmodule/xmodule/word_cloud_module.py

@@ -31,6 +31,11 @@ def pretty_bool(value):

 class WordCloudFields(object):
    """XFields for word cloud."""
+    display_name = String(
+        help="Display name for this module",


Missing the display name (display_name="Display Name"). It's showing up in the metadata editor with key display_name.

cahrens · 2013-07-03T16:42:22Z

There is a bug with existing LaTeX problems. Repro steps:

Create a LaTeX problem on master. Edit the high level source and Save & Compile it.
Switch to this branch and edit that problem. The text is correct in the XML Editor, but when you click "Launch Latex source compiler", you do not see the correct text in that view. It appears to be showing the templated text in that view.

dhm: this makes no sense as the template's only consulted by create_item. There's no way to revert to the template's content. I couldn't reproduce this.

Update (christina): I can't reproduce it now either. Perhaps I edited the text on master directly in the XML Edtior vs the "LaTeX Source Compiler". Sorry for the false alarm.

cahrens · 2013-07-03T16:55:54Z

cms/djangoapps/contentstore/views/component.py

+        # add the default template
+        component_templates[category].append((
+            component_class.display_name.default or 'Blank',
+            category,


I still wonder about this-- do we need to keep the "Blank"?

What do we want it to say if the Descriptor doesn't define a default display_name?

Nothing? Will that blow up?

It will generate a menu item w/o a label.

cahrens · 2013-07-03T17:01:17Z

So it turned out there was no bug in session_kv_store.py? It looks like the file is now gone from the PR.

dhm: it was in the incidental-... PR which got merged yesterday

cpennington · 2013-07-11T19:24:33Z

common/lib/xmodule/xmodule/x_module.py

+                if field in json_metadata:
+                    json_data['_inherited_metadata'][field] = json_metadata[field]
+
+        new_block = system.xblock_from_json(cls, usage_id, json_data)


This is super broken and shouldn't be merged. There are also no tests of this code. Boo.

I am also still concerned about iterating through inheritance.INHERITABLE_METADATA. I'm not sure if that is what your comment refers to.

xblock_from_json isn't defined anywhere. (Except in #271).

I hijacked this methon in x_module b/c it wasn't being used anywhere and
made it serve the new persistence factory and other tests. When I
refactored the existing factory, I made a slightly incompatible api.
Perhaps I should reconcile them, but this change should be innocuous b/c no
code other than the split mongo code and factories calls this method.

On Thu, Jul 11, 2013 at 4:30 PM, Calen Pennington
[email protected]:

In common/lib/xmodule/xmodule/x_module.py:

json_data:

- 'category': the xmodule category (required)

- 'metadata': a dict of locally set metadata (not inherited)

- 'children': a list of children's usage_ids w/in this course

- 'definition':

- '_id' (optional): the usage_id of this. Will generate one if not given one.

"""

usage_id = json_data.get('_id', None)

if not '_inherited_metadata' in json_data and parent_xblock is not None:

json_data['_inherited_metadata'] = parent_xblock.xblock_kvs.get_inherited_metadata().copy()

json_metadata = json_data.get('metadata', {})

for field in inheritance.INHERITABLE_METADATA:

if field in json_metadata:

json_data['_inherited_metadata'][field] = json_metadata[field]

new_block = system.xblock_from_json(cls, usage_id, json_data)

xblock_from_json isn't defined anywhere. (Except in #271 https://github.com/edx/edx-platform/issues/271
).
… <#13fcf6dc92b48ffb_>
On Thu, Jul 11, 2013 at 3:56 PM, Christina Roberts <
[email protected] > wrote: In
common/lib/xmodule/xmodule/x_module.py: > + json_data: > + - 'category':
the xmodule category (required) > + - 'metadata': a dict of locally set
metadata (not inherited) > + - 'children': a list of children's usage_ids
w/in this course > + - 'definition': > + - '_id' (optional): the usage_id
of this. Will generate one if not given one. > + """ > + usage_id =
json_data.get('_id', None) > + if not '_inherited_metadata' in json_data
and parent_xblock is not None: > + json_data['inherited_metadata'] =
parent_xblock.xblock_kvs.get_inherited_metadata().copy() > + json_metadata
= json_data.get('metadata', {}) > + for field in inheritance.INHERITABLE
METADATA: > + if field in json_metadata: > +
json_data['_inherited_metadata'][field] = json_metadata[field] > + > +
new_block = system.xblock_from_json(cls, usage_id, json_data) I am also
still concerned about iterating through inheritance.INHERITABLE_METADATA.
I'm not sure if that is what your comment refers to. — Reply to this email
directly or view it on GitHub<
https://github.com/edx/edx-platform/pull/270/files#r5150865> .

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/270/files#r5151788
.

If this change isn't needed before next-gen-modulestore, let's just move it there, rather having unused broken code in this PR

Ah, yes, it was premature. I didn't realize it was in the wrong PR. I'll
look at whether to make it work w/ either persistence layer first.

On Tue, Jul 16, 2013 at 11:06 AM, Calen Pennington <[email protected]

wrote:

In common/lib/xmodule/xmodule/x_module.py:

json_data:

- 'category': the xmodule category (required)

- 'metadata': a dict of locally set metadata (not inherited)

- 'children': a list of children's usage_ids w/in this course

- 'definition':

- '_id' (optional): the usage_id of this. Will generate one if not given one.

"""

usage_id = json_data.get('_id', None)

if not '_inherited_metadata' in json_data and parent_xblock is not None:

json_data['_inherited_metadata'] = parent_xblock.xblock_kvs.get_inherited_metadata().copy()

json_metadata = json_data.get('metadata', {})

for field in inheritance.INHERITABLE_METADATA:

if field in json_metadata:

json_data['_inherited_metadata'][field] = json_metadata[field]

new_block = system.xblock_from_json(cls, usage_id, json_data)

If this change isn't needed before next-gen-modulestore, let's just move
it there, rather having unused broken code in this PR

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/270/files#r5217901
.

cpennington · 2013-07-12T15:09:53Z

The current state of this PR:

(fixed There is a known bug in from_json. This should be fixed, and tested (diff-cover will verify): moved to next-gen pr)
(fixed: There are tests from master that haven't yet been converted to the new category and boilerplate format
a) Fixing this correctly means setting up the ItemFactory to deal with boilerplates (instead of the older template mechanism for getting particular problem types))

DraftModuleStore was originally designed as a mixin, but never used that way, and with the upcoming changes to use the versioned module store, never will be. This changes removes a circular dependency between mongo.py and draft.py.

This standardizes the XModule field default values to be the same as the values that are presented by studio when a component is added to a course.

Instead, we use XModule field default values when creating an empty XModule. Driven by this use case, we also allow for XModules to be created in memory without being persisted to the database at all. This necessitates a change to the Modulestore api, replacing clone_item with create_draft and save_xmodule.

The defaults used to be rerandomize=always, showanswer=closed. This is preserved for capa problems being imported from XML. However, for courses, and for problems created in Studio, the default has been changed to never/finished, to match the previous defaults used by Studio.

Add boilerplate option to ItemFactory Minor start date fix to not use microsecs

cpennington · 2013-07-16T20:49:00Z

Full speed ahead! Damn the torpedoes! 👍

chrisndodge · 2013-07-16T20:54:01Z

I'll sync up with Mark when he gets back re: updating those metadata fields in prod

Non-persisted studio templates

Use JPGs instead of PNG for course images

…eries-metrics-am course metrics in time series

…agement-commands

…-i18n Ginkgo: Update XBlock Utils to `1.1.1` for better i18n support in XBlock

Remove some edX-specific branding / links (like support URLs) in favor of values from configuration. Images (sample certificates) are still branded for now. We can get them later.

cpennington mentioned this pull request Jun 26, 2013

Next gen modulestore #271

Merged

jzoldak reviewed Jun 27, 2013
View reviewed changes

cahrens reviewed Jun 28, 2013
View reviewed changes

cahrens reviewed Jul 3, 2013
View reviewed changes

cahrens mentioned this pull request Jul 11, 2013

Fix/vik/studio oe #160

Merged

cpennington reviewed Jul 11, 2013
View reviewed changes

Make DraftModuleStore mongo only

ef8618f

DraftModuleStore was originally designed as a mixin, but never used that way, and with the upcoming changes to use the versioned module store, never will be. This changes removes a circular dependency between mongo.py and draft.py.

cpennington mentioned this pull request Jul 16, 2013

Pause install script for Mac users to turn on Mongo #161

Closed

Move defaults from yaml templates to field definitions.

8c904f3

This standardizes the XModule field default values to be the same as the values that are presented by studio when a component is added to a course.

cpennington mentioned this pull request Jul 16, 2013

Lyla/templates #404

Merged

Don Mitchell and others added 7 commits July 16, 2013 14:33

Require markdown to be explicitly set

4b53478

Set about page default content from overview.yml template

f6b7b26

Fix pep8/pylint errors

0a38e49

Don't force field inheritance of default values

92391a2

Refactor tests to no longer use templates

be4fbc5

Add boilerplate option to ItemFactory Minor start date fix to not use microsecs

cpennington added a commit that referenced this pull request Jul 16, 2013

Merge pull request #270 from edx/dhm/non-persisted-studio-templates

d399365

Non-persisted studio templates

cpennington merged commit d399365 into master Jul 16, 2013

cpennington deleted the dhm/non-persisted-studio-templates branch July 16, 2013 21:43

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014

Merge pull request openedx#270 from MITx/dormsbee/course_img_jpg

31c69fb

Use JPGs instead of PNG for course images

Kelketek referenced this pull request in open-craft/edx-platform Oct 21, 2014

Merge pull request #270 from edx-solutions/ziafazal/api-course-time-s…

afcdc50

…eries-metrics-am course metrics in time series

diegomillan referenced this pull request in eduNEXT/edx-platform Sep 14, 2016

Merge pull request #270 from Stanford-Online/azimpradhan/sysadmin-man…

428d0f3

…agement-commands

rediris pushed a commit to gymnasium/edx-platform that referenced this pull request Feb 25, 2021

Merge pull request openedx#270 from appsembler/omar/gk-support-xblock…

66b8666

…-i18n Ginkgo: Update XBlock Utils to `1.1.1` for better i18n support in XBlock

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-persisted studio templates #270

Non-persisted studio templates #270

cpennington commented Jun 26, 2013

cahrens commented Jun 27, 2013

dmitchell commented Jun 27, 2013

cahrens commented Jun 27, 2013

dmitchell commented Jun 27, 2013

jzoldak Jun 27, 2013

dmitchell Jul 3, 2013

dmitchell commented Jun 28, 2013

cahrens Jun 28, 2013

cahrens commented Jun 28, 2013

cahrens Jun 28, 2013

cahrens Jul 3, 2013

dmitchell Jul 3, 2013

cahrens commented Jul 3, 2013

cahrens Jul 3, 2013

dmitchell Jul 3, 2013

cahrens Jul 3, 2013

dmitchell Jul 3, 2013

cahrens commented Jul 3, 2013

cpennington Jul 11, 2013

cahrens Jul 11, 2013

cpennington Jul 11, 2013

dmitchell Jul 16, 2013

cpennington Jul 16, 2013

dmitchell Jul 16, 2013

cpennington commented Jul 12, 2013

cpennington commented Jul 16, 2013

chrisndodge commented Jul 16, 2013

Non-persisted studio templates #270

Non-persisted studio templates #270

Conversation

cpennington commented Jun 26, 2013

cahrens commented Jun 27, 2013

dmitchell commented Jun 27, 2013

cahrens commented Jun 27, 2013

dmitchell commented Jun 27, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitchell commented Jun 28, 2013

Choose a reason for hiding this comment

cahrens commented Jun 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Jul 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Jul 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpennington commented Jul 12, 2013

cpennington commented Jul 16, 2013

chrisndodge commented Jul 16, 2013