I'm guessing DjangoCon.eu week wasn't the ideal time to send this.
Loads of people did check the screenshots out though. Anyone have
concerns or suggestions?
The demo screenshots you provide certainly look good to me; I haven't
done a full teardown on the patch, but a from a quick glance it
certainly looks promising.
A couple of quick questions:
* Why allow edit links on a readonly field? This seems a little
redundant to me?
* On the edit link for ForeignKey (localhost:8000 in your example),
I'd be inclined to stick to just "edit", not "edit <object>" -- that
seems more consistent with the other edit links you have provided.
* I take it that the widget edit links carry through to inlines?
i.e., if i have a foreign key pulldown in an inline, I will get the
edit link?
* How does performance degrade when JavaScript is not available? Do
the links exist at all, or do they become dangling links to the wrong
object?
* In the case of raw-id fields and inlines, is there any reason why
the edit link is separate text, rather than the object name itself
being the link? (ie., rather than "John smith <edit separately>", why
not just "<John Smith>"?
* Permissions - from my initial inspection, it isn't obvious to me
that you are honoring (and/or testing that you are honoring)
permissions. If I don't have permission to edit an object, I shouldn't
get an edit link. Given the addition in 1.2 of object-level
permissions, this means permissions need to be per-object. Have I
missed something in my hasty patch read?
Yours,
Russ Magee %-)
Thanks for your response Russ.
> * Why allow edit links on a readonly field? This seems a little
> redundant to me?
Because whilst the field on that model might be read-only, the related
object itself is not necessarily. In fact in most cases I've found
that this is the case.
> * On the edit link for ForeignKey (localhost:8000 in your example),
> I'd be inclined to stick to just "edit", not "edit <object>" -- that
> seems more consistent with the other edit links you have provided.
But then if you select a different object, "edit" looks like it refers
to the selected one instead of the original. I could have used
JavaScript here to select the dynamically chosen object, but in the
absence of a popup link this would be pointless -- you choose a
different ForeignKey value, then leave the page to edit it thinking
you've saved the value...
> * I take it that the widget edit links carry through to inlines?
> i.e., if i have a foreign key pulldown in an inline, I will get the
> edit link?
Yes.
> * How does performance degrade when JavaScript is not available? Do
> the links exist at all, or do they become dangling links to the wrong
> object?
Solid as a rock; does not use JavaScript.
> * In the case of raw-id fields and inlines, is there any reason why
> the edit link is separate text, rather than the object name itself
> being the link? (ie., rather than "John smith <edit separately>", why
> not just "<John Smith>"?
Yes; because you're already editing John smith, but if you want to
edit him separately you can go elsewhere to do so with a (probably
more detailed) dedicated form.
> * Permissions - from my initial inspection, it isn't obvious to me
> that you are honoring (and/or testing that you are honoring)
> permissions. If I don't have permission to edit an object, I shouldn't
> get an edit link. Given the addition in 1.2 of object-level
> permissions, this means permissions need to be per-object. Have I
> missed something in my hasty patch read?
Correct; no permissions checking is performed at present. In some
places checking would be almost impossible given the current code
architecture, so I had hoped to avoid it if possible. This was one of
the main points I wanted feedback on.
Simon
Here is a more detailed explanation of why permission checking has
been omitted for now:
* The "add-another" link (green plus) next to ForeignKey widgets is
appended to the HTML output in the render() method. This currently has
no access to the User, so cannot determine permissions. The
"add-another" link (and new "edit" link) will therefore be displayed
regardless of the current user's add/change permissions for the
related object. This should certainly be dealt with, but this ticket
does not appear to the the appropriate place to do so. Ticket #1035
[1] addresses this issue.
* The "edit separately" links on inlines could perform checking using
a templatetag, perhaps. Again, however, I'm not sure that this is the
place to deal with this as larger issues exist -- currently a user can
add/change inlines even if they do not possess permissions for the
inline model. Ticket #8060 [2] addresses this issue.
So the current patch for #13163 and #13165 [3] seems to solve the
issues in a manner which fits with the rest of the admin interface as
it currently stands. Permission issues should certainly be dealt with,
but separately I believe. I think [3] could be committed as is
(pending review), and permission controls added as the other tickets
are resolved. Currently all this means is that people might see an
edit link, click on it, then get a "permission denied" message --
though in the relatively rare cases where a related object is
registered in the same admin site but the user doesn't have permission
to change it.
IMHO the UX improvements in [3] are worth getting on board ASAP.
Simon
[1] http://code.djangoproject.com/ticket/1035
[2] http://code.djangoproject.com/ticket/8060
[3] http://code.djangoproject.com/attachment/ticket/13163/combined_13163_13165.diff
The demo screenshots you provide certainly look good to me; I haven'tdone a full teardown on the patch, but a from a quick glance itcertainly looks promising.
Thanks for your response Russ.* Why allow edit links on a readonly field? This seems a littleredundant to me?
Because whilst the field on that model might be read-only, the related
object itself is not necessarily. In fact in most cases I've found
that this is the case.
* On the edit link for ForeignKey (localhost:8000 in your example),I'd be inclined to stick to just "edit", not "edit <object>" -- thatseems more consistent with the other edit links you have provided.
But then if you select a different object, "edit" looks like it refers
to the selected one instead of the original. I could have used
JavaScript here to select the dynamically chosen object, but in the
absence of a popup link this would be pointless -- you choose a
different ForeignKey value, then leave the page to edit it thinking
you've saved the value...
* In the case of raw-id fields and inlines, is there any reason whythe edit link is separate text, rather than the object name itselfbeing the link? (ie., rather than "John smith <edit separately>", whynot just "<John Smith>"?
Yes; because you're already editing John smith, but if you want to
edit him separately you can go elsewhere to do so with a (probably
more detailed) dedicated form.
* Permissions - from my initial inspection, it isn't obvious to methat you are honoring (and/or testing that you are honoring)permissions. If I don't have permission to edit an object, I shouldn'tget an edit link. Given the addition in 1.2 of object-levelpermissions, this means permissions need to be per-object. Have Imissed something in my hasty patch read?
Correct; no permissions checking is performed at present. In some
places checking would be almost impossible given the current code
architecture, so I had hoped to avoid it if possible. This was one of
the main points I wanted feedback on.
Sent from my iPad
My perspective is the other way around. We already have permission problems, which have been exacerbated by the introduction of row-based permission hooks. IMHO, before we start adding extra entry points where permissions can be wrong, we should get our house in order.
I appreciate that this is an impediment to you getting your changes into trunk, but personally, I'd rather see less bugs (especially when those bugs relate to something as important as permissions) than more features.
> IMHO the UX improvements in [3] are worth getting on board ASAP.
No argument from me on this - absent of other problems, they strike me as worthwhile changes. I just think we should fix the other problems first.
>
Yours,
Russ Magee %-)