Skip to content

Dhm/editable metadata - #844

Merged
dmitchell merged 6 commits into
masterfrom
dhm/editable_metadata
Sep 6, 2013
Merged

Dhm/editable metadata#844
dmitchell merged 6 commits into
masterfrom
dhm/editable_metadata

Conversation

@dmitchell

Copy link
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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

Copy link
Copy Markdown
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
Copy Markdown
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants