I'll work on this after I rebase next-gen. I screwed up its history :-(


On Tue, Jul 2, 2013 at 11:11 AM, Calen Pennington
<notifications@github.com>wrote:

> In cms/djangoapps/contentstore/views/item.py:
>
> > @@ -53,10 +54,12 @@ def save_item(request):
> >          existing_item = modulestore().get_item(item_location)
> >
> >          # update existing metadata with submitted metadata (which can be partial)
> > -        # IMPORTANT NOTE: if the client passed pack 'null' (None) for a piece of metadata that means 'remove it'
> > +        # IMPORTANT NOTE: if the client passed 'null' (None) for a piece of metadata that means 'remove it' unless
> > +        # the field is in NULLABLE_FIELDS which means None is an expected explicit value but not necessarily the
> > +        # defulat
>
> I agree with not introducing NULLABLE_FIELD unless we absolutely have to.
> Looking at the code in XBlock, we expect KeyValueStores to raise a
> KeyError if the value hasn't been set (so we aren't explicitly looking
> for None). The only place that None means deleted/missing is in this
> method, I think.
>
> If we take my above suggestion, and replace the 'null means delete'
> semantics in this method, then we can get rid of NULLABLE_FIELDS.
> Instead, the client can just pass the list of fields to delete separately
> from the list of fields to modify, and we can call del in python (as the
> code is doing now when it sees None).
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/edx/edx-platform/pull/270/files#r4986582>
> .
>