Repository navigation
feat: Browsable Django Admin interface for Containers #330
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,4 @@ | |
| Open edX Learning ("Learning Core"). | ||
| """ | ||
|
|
||
| __version__ = "0.26.0" | ||
| __version__ = "0.27.0" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| """ | ||
| Django admin for sections models | ||
| """ | ||
| from django.contrib import admin | ||
| from django.utils.safestring import SafeText | ||
|
|
||
| from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin, model_detail_link | ||
|
|
||
| from .models import Section, SectionVersion | ||
|
|
||
|
|
||
| class SectionVersionInline(admin.TabularInline): | ||
| """ | ||
| Minimal table for subsecdtion versions in a subsection | ||
| """ | ||
| model = SectionVersion | ||
|
|
||
|
|
||
| @admin.register(Section) | ||
| class SectionAdmin(ReadOnlyModelAdmin): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Counterproposal... given that PEs already stringify to their key: class PublishableEntity(models.Model):
...
def __str__(self):
return f"{self.key}"how about I make PEVs stringify like this: class PublishableEntityVersion(models.Model):
...
def __str__(self):
return f"{self.entity.key} @ v{self.version_num}"and then make it so their mixin models default to the same behavior? class PublishableEntityMixin(models.Model):
...
def __str__(self) -> str:
return str(self.publishable_entity)
class PublishableEntityVersionMixin(models.Model):
...
def __str__(self) -> str:
return str(self.publishable_entity_version)I know that the titles are nicer sometimes, but I feel that the keys and version nums better represent the models.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe have the title at the end? # <bikeshed>
def __str__(self):
return f"{self.entity.key} @ v{self.version_num} - {self.title}"
# </bikeshed>
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| """ | ||
| Very minimal interface... just direct the admin user's attention towards the related Container model admin. | ||
| """ | ||
| list_display = ["section_id", "key"] | ||
| fields = ["see"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I found "See" to be a bit ambiguous at first.
I think this would be more clear:
Which can be accomplished by overriding def get_form(self, request, obj=None, change=False, **kwargs):
help_texts = {'container': 'To see details of this section, click above to see its container view.'}
kwargs.update({'help_texts': help_texts})
return super().get_form(request, obj, **kwargs)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I applied this to each core container type, with some tweaks to ensure that it doesn't seem like "Container" refers to some container that holds this as a child.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| readonly_fields = ["see"] | ||
| inlines = [SectionVersionInline] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these inlines are very useful. The one you see when you click on the "See" link to view the Container admin has way more data and is much more useful. So I think it's better to just remove this so people don't waste their time reviewing the data on this page, and click through to the Container page.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had initially omitted this inline, but I ended up needing to add it in order to diagnose an bug where the modulestore_migrator was creating ContainerVersions but not their connected subclasses (SectionVersion, etc.). As long as it's possible to create a section's ContainerVersion but not its SectionVersion, I'd like to keep this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, that makes sense. Maybe mention that in a comment so others don't have the same question as me in the future?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, done |
||
|
|
||
| def section_id(self, obj: Section) -> int: | ||
| return obj.pk | ||
|
|
||
| def key(self, obj: Section) -> SafeText: | ||
| return model_detail_link(obj.container, obj.container.key) | ||
|
|
||
| def see(self, obj: Section) -> SafeText: | ||
| return self.key(obj) | ||






There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit/optional: I think the
uuidis not very useful, and the title is, so I suggest taking out the former and adding the latter.Screenshot:
vs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Changed for both Draft and Published.
I also removed all other occurrences of UUID from the various container-related admin lists, detail views, and inlines. We're not using the UUIDs for anything right now, so I'd rather it not clutter up the useful information with something irrelevant. If an operator really needs to know something's UUID, it is available on the linked PublishableEntity detail view.