-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add initial version of wagtail-treemodeladmin #1
Conversation
A screenshot or two in the readme would be nice :) |
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.
This is too slick for words.
I'd like to spend more time wth the PR, but I have some testing notes to pass along:
- I had to add
coverage
to the tox.ini [testenv] deps to get tox to run. - If we want to test against wagtail20 we'll need to handle the admin module's name change from
wagtail.wagtailadmin
towagtail.admin
- I had to adjust your steps a bit to get scenario 1 (isolation) to run, and needed to add a
migrate
step to set up a test db:
export DJANGO_SETTINGS_MODULE=treemodeladmin.tests.settings
export PYTHONPATH='[PATH TO PROJECT]'
django-admin.py migrate
django-admin.py loaddata treemodeladmin_test
django-admin.py runserver
@Scotchester Once we get the styling and appearance nailed down, I'll make sure to include one.
Weird, @higs4281. I wiped everything out to be sure, but
Embarrassing oversight on my part! It's now Wagtail 2.0-compatible (with some heavy borrowing from @chosak's wagtail-sharing settings)! |
Is that something you'd like help with? |
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.
Can you confirm: does this package create any models of its own, or only add admin/wrappings around models from other apps? I ask for database migration considerations.
README.md
Outdated
|
||
> Keep the README fresh! It's the first thing people see and will make the initial impression. | ||
Wagtail-TreeModelAdmin is an extension for Wagtail's ModelAdmin that allows for a page explorer-like navigation of Django model relationships within the Wagtail admin. |
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.
Link to ModelAdmin docs?
README.md
Outdated
``` | ||
|
||
---- | ||
2. Add `treemodeladmin` as an installed app in your Django `settings.py`: |
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.
On wagtail-sharing I added explicit documentation about its dependency on Wagtail ModelAdmin, as ModelAdmin is optional and isn't part of the standard Wagtail setup.
README.md
Outdated
|
||
## Getting help | ||
Then visit the Wagtail admin. `Library` will be in the menu, and will give you a list of authors, and each author will have a link that will take to their books. |
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.
Typo? "that will take [you] to their books".
setup.py
Outdated
|
||
try: | ||
import pypandoc | ||
long_description = pypandoc.convert('README.md', 'rst') |
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.
Is this conversion to RST still necessary now that PyPI properly supports Markdown READMEs?
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.
Probably not. I'd like to do some experimentation though, considering that the Markdown support didn't work with the latest release of wagtail-flags.
install_command=pip install -e ".[testing]" -U {opts} {packages} | ||
commands= | ||
coverage erase | ||
coverage run --source='treemodeladmin' {envbindir}/django-admin.py test {posargs} |
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.
How would you feel about adding a third coverage report
line here? I've been thinking lately that this would be a good pattern to adopt in our packages, and I think I've seen @higs4281 use this. An advantage of including the report as part of the tox execution is that then you don't need to have coverage installed outside of the tox env to view the reports; a downside is for tox setups with many environments you might see the report many times (although, maybe this is good to ensure that the coverage is consistent across versions).
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 actually like this idea a lot. I'll do it!
@@ -0,0 +1,8 @@ | |||
{% load i18n treemodeladmin_tags %} |
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.
Is treemodeladmin_tags
needed here?
@@ -0,0 +1,24 @@ | |||
{% load i18n modeladmin_tags treemodeladmin_tags %} |
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.
Is treemodeladmin_tags
needed here?
@chosak That is correct. The test app, treemodeladmintest, has migrations, but that won't be used in any deployed environment. |
I'll rope @contolini into it when the time comes. I think we're just going to have to compile and provide our own custom CSS for it. |
After re-testing, I only encountered this when running bare If others encounter this, maybe we could add a testing note about matrix options. |
Addressed review feedback. I'd like to merge this so that we can include it in cfgov-refresh and iterate on it with regulations3k. @higs4281 @contolini any objections? @chosak @Scotchester? |
Haven't tested it, but no objection. |
This PR adds the initial version of wagtail-treemodeladmin. The README hopefully provides a decent overview of what this is.
Implementationally, I've tried to make this as light-weight as possible. It extends ModelAdmin as little as possible to allow page explorer-style navigation from one model to another via a configured
ForeignKey
one-to-many relationship. It does this by having the fields for that relationship explicitly configured on theTreeModelAdmin
subclass, and then adding the>
column to the right of the object listing table as a link to the "child" model listing filtered for the parent. For example, given the models in the quickstart section of the README, the destination of the>
link for the author with id1
in the author list at the URL/admin/libraryapp/author/
will be/admin/libraryapp/book/?author=1
.Testing
There are a couple ways to see it work.
In isolation, install Django and Wagtail into a virtual environment (or activate one of the tox virtualenvs), and then run:
Then visit http://localhost:8000/admin. Author will be available under the "TreeModelAdmin Test" menu item.
As part of cfgov-refresh, assuming a working docker version with a recent database, checkout the
treemodeladmin
branch, rebuild yourpython
docker container or otherwise installrequirements/libraries.txt
in your virtualenv:Then visit http://localhost:8000/admin. The Regulations 3000 models will be navigable via TreeModelAdmin under the "Regulations" menu item.
Todos
There is more to do after this PR, including:
Make breadcrumbs respect the parent-child relationships.
Currently the breadcrumb links are simply to the listing of all objects (so
/admin/libraryapp/book/
instead of/admin/libraryapp/book/?author=1
).Make pre-fill parents in their respective fields in the Add view of child objects.
Currently if you're at
/admin/libraryapp/book/?author=1
and you click the "Add" button, the author field is blank.Bring button styles/spacing more in line with the page explorer.
The particular styles of the header on the page explorer are not available outside a specific class ancestry that seems too fragile to try to imitate in the ModelAdmin-like templates. This may require us to include out own stylesheet.
But this is intended to be a first iteration and proof-of-concept that we can work with for now as we fill out those features before releasing wagtail-treemodeladmin.
Checklist