Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dhm/editable metadata #844

Merged
merged 6 commits into from
Sep 6, 2013
Merged

Dhm/editable metadata #844

merged 6 commits into from
Sep 6, 2013

Conversation

dmitchell
Copy link
Contributor

@cpennington I'm sure this collides w/ your PR. Can we get this in first? Please review

@cahrens @chrisndodge @singingwolfboy can one of your review it from the studio perspective?

This PR has xblock compute inherited fields via an override of the default fn. It standardizes the representation of inherited fields through a new KVS called InheritanceKVS and has all of the other ones derive from this or (in the case of LmsKVS) delegate to it when a default is needed.

It replaces all simple dict model data containers w/ real xblock ones esp in the test code (already that way in prod code).

the 2 'x' commits were due to typos and not having lms kvs delegate.

@cpennington
Copy link
Contributor

My upgrade to xblock 0.3 is significantly larger than this PR, so I'd like to merge it first. I think it'll be easier to figure out the conflicts and changes needed on this change than in the other.

self._fields = initial_values or {}

@property
def inherited_settings(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a property, and not just an attribute on the class? The getter and setter don't do anything except get and set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over application of getter/setter pattern. Sorry.

@dmitchell
Copy link
Contributor Author

Rebased on @cpennington's PR and rewrote history some to consolidate changes. Passes all tests locally.

# TODO: Once mixins are defined per-application, rather than per-runtime,
# this should use a cms mixed-in class. (cpennington)
if hasattr(component_class, 'display_name'):
display_name = component_class.display_name.default or 'Blank'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use getattr with 'blank' as the default value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... Can't do that, because we're looking for the display_name attribute, and then getting the .default from it. getattr only supports a single level of lookup.

@jkarni jkarni mentioned this pull request Sep 5, 2013
self.end_page == other.end_page and
# could actually compare structures and ignore whitespace but this seemed easiest if not quite sound
etree.tostring(self.table_of_contents) == etree.tostring(other.table_of_contents))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already implemented in my changes. Don't need 2 copies of the method.

@cpennington
Copy link
Contributor

😵 🐴 : 👍

dmitchell added a commit that referenced this pull request Sep 6, 2013
refactoring of platform to xblock 0.3 w/ refactoring of inheritance in the platform to a consistent representation.
@dmitchell dmitchell merged commit 0848360 into master Sep 6, 2013
@cpennington cpennington deleted the dhm/editable_metadata branch September 9, 2013 13:16
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jun 3, 2016
kluo added a commit to kluo/edx-platform that referenced this pull request Dec 4, 2018
Revive account settings fork for uneditable fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants