Making easy to add new views to a ModelAdmin2 - implemented AdminView #218
Conversation
generating the get_urls dynamically and support permissions.
…into admin_view
|
Apologies for not reviewing this sooner. I'm pushing it to the 0.5.0 release (0.4.0 is coming out today) and will review it very soon. |
|
@pydanny no problem :) |
ghost
assigned 
| create_view = views.AdminView(r'^create/$', views.ModelAddFormView, name='create') | ||
| update_view = views.AdminView(r'^(?P<pk>[0-9]+)/$', views.ModelEditFormView, name='update') | ||
| detail_view = views.AdminView(r'^(?P<pk>[0-9]+)/update/$', views.ModelDetailView, name='detail') | ||
| delete_view = views.AdminView(r'^(?P<pk>[0-9]+)/delete/$', views.ModelDeleteView, name='delete') |
There was a problem hiding this comment.
@andrewsmedina - Can you explain the reasoning for this method for URL definition instead of the earlier one? Some concerns that I have:
- Performance
- Scoping issues with multiple users acting on the same or unrelated views.
For reference, I'm not opposed, I'm just looking at this and worrying about the implications. I will admit I haven't had all the time necessary to properly evaluate your code.
There was a problem hiding this comment.
I'm just using the new AdminView for all 'admin views'. With it, is easier to replace an admin model view.
About performance, I will make a benchmark to measure the impact. Ok?
There was a problem hiding this comment.
Actually, performance isn't as much an issue as the second one, which is the scope concern.
I'm still trying to be certain about the scope and thread safety involved. Remember, the reason why Django didn't get CBVs for a long time was because this is a hard problem. Heck, Marty Alchin even got it wrong in 1st edition Pro-Django. You've got a neat idea, I'm just trying to be certain it's safe. And checking for safety in this case isn't easy. 🍂
There was a problem hiding this comment.
@pydanny, I remember the CBV case and the as_view was the solution for thread safety. I will check this possible problem :)
| class ModelAdmin2(object): | ||
| """ | ||
| Warning: This class is targeted for reduction. | ||
| It's bloated and ugly. | ||
| """ | ||
| __metaclass__ = ModelAdminBase2 |
There was a problem hiding this comment.
Have you tested your code on Python 3? The syntax for metaclasses has changed in Python 3. You might want to use the six library to support Python 2 and 3.
There was a problem hiding this comment.
Indeed. In fact, at some point soon we should add Python 3 as a Travis environment.
|
Nice one! I reviewed the patch and approve the changes from my side. I just added on inline comment regarding Python 3. However some documentation about the view definitions would be reeeally sweet 🍩 |
|
@gregmuellegger What about concerns on thread safety? That's my bottleneck for this one. I keep meaning to find the time to really dig in and analyze it. I agree with you on documentation. For example:
|
|
I don't see any issues with thread safety. See https://github.com/pydanny/django-admin2/pull/218/files#L4R181 the line is: view = model_admin.update_view.view.as_view(
**model_admin.get_update_kwargs())The view is instantiated via Hope it's clear what I mean. Just hit me with a stick and I'll try to write it down again in more detail. |
|
That's what I thought. Digging in too and it's doing what we think it's doing. I wanted more eyes on this so thanks! @andrewsmedina Want to update to the current code base and fill out some of the docs? 🍷 |
|
@gregmuellegger and @pydanny thanks for this review! :) @pydanny I will update the code base today. I was thinking of sending the documentation in another pull request. What do you think? |
|
Same pull request please! On Fri, Jul 26, 2013 at 8:41 PM, Andrews Medina notifications@github.comwrote:
'Knowledge is Power' |
|
@pydanny roger that! |
Conflicts: djadmin2/types.py
|
@pydanny docs and the python3 fix commited. |
|
Wahoo!!! Awesome work:exclamation: :fire: :fireworks: |
it fix the #142.