Use XBlock 0.3 #839

cpennington · 2013-08-30T19:46:15Z

@nedbat @sarina @dmitchell: Can you guys review?

Scope of changes:

Migrate Namespaces to mixins
1. all references to <block>.lms.<field> and <block>.cms.<field> should now just be <block>.<field>
2. Anywhere that a modulestore is constructed, it should be passed the appropriate mixin
3. Anywhere a block is constructed, it should be done using on of the construct_block* methods
Split up the xblock modules (core -> core, fields)
Switch to the many -> 1 model for Runtimes and FieldDatas
1. FieldData access should always be through get, set, set_many, delete, rather than the dictionary interface
2. Those methods should always be passed a block as the first argument.
Change module that use Scope.content and store it in SQL to instead use one of the new UserScope.ALL scopes

dmitchell · 2013-09-03T13:24:01Z

cms/djangoapps/contentstore/tests/test_crud.py

-            children.append(new_block)
-            # trigger setter method by using top level field access
-            parent_xblock.children = children
+            parent_xblock.children.append(new_block.scope_ids.usage_id)


Given the order of diffs, it's hard to tell what you've done to create a uuid/cache for in memory usage_ids; so, I'll assume that's handled.

Probably beyond the scope of this PR, but we need a new repr for children addresses which allow structure spanning: pointing to elements in other structures. As a shortcut, we could say that a simple ID implies w/in same structure and have a different repr and logic for pointers into other structures.

I thought that block locators allowed for pointers across structures.

block locators do, but I thought we were just persisting the usage_id not
the whole block usage locator.

On Tue, Sep 3, 2013 at 9:45 AM, Calen Pennington
notifications@github.comwrote:

In cms/djangoapps/contentstore/tests/test_crud.py:

@@ -226,10 +226,7 @@ def load_from_json(json_data, system, default_class=None, parent_xblock=None):

new_block = system.xblock_from_json(class_, usage_id, json_data) if parent_xblock is not None:

children = parent_xblock.children

children.append(new_block)

# trigger setter method by using top level field access

parent_xblock.children = children

parent_xblock.children.append(new_block.scope_ids.usage_id)

I thought that block locators allowed for pointers across structures.

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

It should be whatever id locates a particular usage (meaning, a usage inside a course). So, the usage locator, rather than the usage_id inside the locator, I guess.

sarina · 2013-09-03T15:37:35Z

Rebasing should pull in the changes Jason made to the bulk email tests

dmitchell · 2013-09-03T15:39:45Z

common/lib/xmodule/xmodule/x_module.py

+        return (self.scope_ids == other.scope_ids and
+                self.fields.keys() == other.fields.keys() and
+                all(getattr(self, field.name) == getattr(other, field.name)
+                    for field in self.fields.values()))


Much cleaner and clearer.

No reason to exclude mixin fields.

dmitchell · 2013-09-03T16:13:11Z

We need to check whether there are unit tests and perhaps also manually test all of the views impacted by switching .fields from being local only to the union of all namespaces.

sarina · 2013-09-03T18:31:24Z

lms/djangoapps/courseware/model_data.py

                scope_map[field.scope].add(field)
        return scope_map

    def _cache_key_from_kvs_key(self, key):
        """
-        Return the key used in the ModelDataCache for the specified KeyValueStore key
+        Return the key used in the FieldDataCache for the specified KeyValueStore key
        """


This function now does nothing if scope is either Scope.content or Scope.settings - is that intentional?

Same goes for _cache_key_from_field_object. If this is intentional please add comments as to why.

Ah nm finally got to the docstring for DjangoKeyValueStore

nedbat · 2013-09-03T19:53:56Z

common/lib/xmodule/xmodule/course_module.py

+
+    def __ne__(self, other):
+        return not self == other
+


Why is this addition (good though it looks) in this pull request?

It's used so that the new method of checking XModule testing actually works.

dmitchell · 2013-09-03T21:22:55Z

I can't seem to successfully run cms. I think I need to rebuild my dbs. The manual tests to run:

course advanced settings shows the right attrs (not sure which are "right" tho) PASSED
non-problem leaf components (e.g., video, html) don't have problem metadata attrs in editor FAILED
problem leaf components have "right" attrs (@caherns ran this test and says it failed)
exported courses have all of the right data in the right places

nedbat · 2013-09-04T14:32:46Z

common/lib/xmodule/xmodule/modulestore/mongo/base.py

+
+            # We're loading a descriptor, so student_id is meaningless
+            # We also don't have separate notions of definition and usage ids yet,
+            # so we use the location for both,


…atus Feature/victor/per course status

Remove spaces for pep8

dmitchell reviewed Sep 3, 2013
View reviewed changes

sarina reviewed Sep 3, 2013
View reviewed changes

nedbat reviewed Sep 3, 2013
View reviewed changes

nedbat reviewed Sep 4, 2013
View reviewed changes

Use XBlock 0.3

8201b14

dmitchell merged commit 8201b14 into openedx:master Sep 6, 2013

cpennington deleted the xblock-0.3 branch September 9, 2013 13:16

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

Merge pull request openedx#839 from MITx/feature/victor/per-course-st…

076ef3d

…atus Feature/victor/per course status

ny0m pushed a commit to open-craft/edx-platform that referenced this pull request Aug 31, 2017

MCKIN-5415 Interim Completion API (openedx#839)

81ba963

ny0m pushed a commit to open-craft/edx-platform that referenced this pull request Aug 31, 2017

MCKIN-5415 Interim Completion API (openedx#839)

9fd72a7

ziafazal pushed a commit that referenced this pull request Dec 12, 2017

MCKIN-5415 Interim Completion API (#839)

093924e

kluo pushed a commit to kluo/edx-platform that referenced this pull request Nov 30, 2018

Merge pull request openedx#839 from Stanford-Online/dcadams/remove_space

be07695

Remove spaces for pep8

baby636 mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#22

Open

baby636 mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#33

Open

baby636 mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#41

Open

baby636 mentioned this pull request Nov 28, 2023

[Snyk] Fix for 37 vulnerabilities baby636/edx-platform#54

Open

sumodgeorge mentioned this pull request Nov 28, 2023

[Snyk] Fix for 6 vulnerabilities sumodgeorge/edx-platform#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use XBlock 0.3 #839

Use XBlock 0.3 #839

cpennington commented Aug 30, 2013

dmitchell Sep 3, 2013

cpennington Sep 3, 2013

dmitchell Sep 3, 2013

cpennington Sep 3, 2013

sarina commented Sep 3, 2013

dmitchell Sep 3, 2013

cpennington Sep 3, 2013

dmitchell commented Sep 3, 2013

sarina Sep 3, 2013

sarina Sep 3, 2013

nedbat Sep 3, 2013

cpennington Sep 4, 2013

dmitchell commented Sep 3, 2013

nedbat Sep 4, 2013

Use XBlock 0.3 #839

Use XBlock 0.3 #839

Conversation

cpennington commented Aug 30, 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

sarina commented Sep 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitchell commented Sep 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

dmitchell commented Sep 3, 2013

Choose a reason for hiding this comment