-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
61f8fac to
80aa3a2
Compare
|
@ormsbee @bradenmacdonald Low priority, but I pulled the admin pages in from the prototype branch. Could be nice to have merged before people who see our talk start poking around in Learning Core. |
bradenmacdonald
left a comment
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.
Thanks, this is great! I had some feedback but it's super minor - consider it all optional.
| list_display = ["section_id", "key"] | ||
| fields = ["see"] | ||
| readonly_fields = ["see"] | ||
| inlines = [SectionVersionInline] |
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.
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.
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.
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.
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.
OK, that makes sense. Maybe mention that in a comment so others don't have the same question as me in the future?
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.
sure, done
|
|
||
|
|
||
| @admin.register(Section) | ||
| class SectionAdmin(ReadOnlyModelAdmin): |
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.
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.
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.
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.
Maybe have the title at the end?
# <bikeshed>
def __str__(self):
return f"{self.entity.key} @ v{self.version_num} - {self.title}"
# </bikeshed>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.
| Very minimal interface... just direct the admin user's attention towards the related Container model admin. | ||
| """ | ||
| list_display = ["section_id", "key"] | ||
| fields = ["see"] |
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: I found "See" to be a bit ambiguous at first.
I think this would be more clear:
Which can be accomplished by overriding get_form on the Section/Subsection/Unit admin class.
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)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.
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.
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.
| """ | ||
| if draft := obj.versioning.draft: | ||
| return format_html( | ||
| "Version {} ({})", draft.version_num, _entity_list_detail_link(draft.entity_list) |
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 uuid is not very useful, and the title is, so I suggest taking out the former and adding the latter.
| "Version {} ({})", draft.version_num, _entity_list_detail_link(draft.entity_list) | |
| 'Version {} "{}" ({})', draft.version_num, draft.title, _entity_list_detail_link(draft.entity_list) |
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.
|
Nothing to add to @bradenmacdonald's review. It looks great. 😄 |
This adds several admin pages which were previously missing or barebones: Section, Subsection, Unit, Container, and EntityList. The Container and EntityList pages have inlines tables to display their related ContainerVersions and EntityListsRows. This enables developers/operators to browse up and down the content hierarchies that are stored in Learning Core.
80aa3a2 to
13223a7
Compare




Description
This adds several admin pages which were previously missing or barebones: Section, Subsection, Unit, Container, and EntityList. The Container and EntityList pages have inlines tables to display their related ContainerVersions and EntityListsRows. This enables developers/operators to browse up and down the content hierarchies that are stored in Learning Core.
Testing Instructions
Using edx-platform master:
Screenshots
Admin page for a Subsection
Just points you to the admin page for that associated Container, since that's where the interesting info is.
Admin page for that Subsection's associated Container
Each ContainerVersion is linked to an EntityList.
Admin page for the EntityList of a Subsection
Each row is a Container linked to a Unit, with a child EntityList of its own
Entity list of a Unit
Each row is a Component