Keyed ConfigurationModel + Admin functionality

[bradenmacdonald]

This extends @cpennington's work to allow keyed ConfigurationModels (cpennington@c1abce7). More background here.

I've developed this [further] for use in the ongoing Shibboleth support branch. See #8155 for an example of real world use.

Sandbox (Updated May 26):
There is a demo at http://sandbox5.opencraft.com/admin/third_party_auth/ - user name 'admin', ping Braden via HipChat or IRC or email for the password.

Dependencies: None

Screenshots:
Screenshot of the admin view:

Collapsed - default admin view only shows the current values for each key:

Expanded - shows the complete change history:

Reviewers:
Code: @smarnach, @cpennington

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=8077

[bradenmacdonald]

This was ','.join(repr(arg) for arg in args)

I made this change because the use of repr here was causing inconsistent keys - sometimes I saw keys like .../current/'google' and other times like .../current/u'google'.

[smarnach]

Using str() instead of repr() should do the trick as well: ','.join(str(arg) for arg in args)

[bradenmacdonald]

Well repr is better for use as a key because it's designed to be less ambiguous vs. str is designed for readability:

>>> str(3)==str("3")
True
>>> repr(3)==repr("3")
False

But by using str sometimes and repr others I've perhaps somewhat defeated the purpose.

Maybe repr(unicode(arg)) if isinstance(arg, str) else repr(arg) ?

[smarnach]

A few weird exceptions aside, for builtin types repr() and str() have more or less the same result, except for strings; repr('3') will include the quotes, while str('3') won't. If you use repr() for everything but strings, and str() for strings, you might just as well use str() for all of them.

[bradenmacdonald]

Well, I suppose there won't be models where a key can be either an integer or a string type, since Django doesn't even allow that. So maybe that is fine. But I'll have to use unicode() not str() in case the keys contain unicode values.

[smarnach]

Even if it were possible that keys can have either string or integer type, your code would have exactly the same problem. For built-in types, arg if isinstance(arg, basestring) else repr(arg) and unicode(arg) do virtually the same thing (with the exception of a few edge cases).

You are of course right about unicode() vs str().

[smarnach]

I feel that this should be a feature of the manager of the ConfigurationModel class, since it's common to be interested in only the current values and to perform queries on them.

[bradenmacdonald]

Yep, I thought about that too, and I'm still not sure. My concern is that the performance of this query could be inefficient, so it could be better to encourage the use of .key_values() together with multiple calls to .current(), which would generally retrieve everything from the cache and not need to hit the database. Though if there is any table with "lots" of entries, then this query would be more efficient. It would really depend on use cases.

I certainly can move this to a custom manager on the class, though it would need to be implemented as two different managers - one for "select only active objects" and one for "select all objects and annotate them with is_active", since the syntax required for those queries is unfortunately different.

[bradenmacdonald]

OK I tried it out here: fed9e08 . I think that's better?

[smarnach]

Yes, I'd prefer to have it there.

[smarnach]

@bradenmacdonald I've added a few comments here, please take a look.

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=8077

[smarnach]

Looks good to me now, 👍.

[bradenmacdonald]

Sven, FYI I added i18n and changed to unicode instead of repr plus added a test for the case that brought on that change in the first space.

[bradenmacdonald]

@cpennington This is ready for your review.

[cpennington]

Why aren't these cache deletions happening as part of the model actually being saved?

[cpennington]

It seems like this might be easier to write if we actually had an is_active flag on the configuration that marked the active configuration, rather than using the most recently modified row. As long as that field is properly manipulated during saves, then we should be able to keep in consistent with the dates, I think.

It's a bit ugly to have that duplicated data, though.

[cpennington]

Actually, if we used a save hook, then that might be a good place to update both the cache and the is_active flag (as long as updating the is_active flag didn't trigger the hook again).

[cpennington]

This should document the flat argument.