Django Related-Object Links in Admin

[Simon Meers]

I've uploaded some screenshots [1] of the new patch for #13163 [2] and
#13165 [3] in action, to allow people to see the affect without
necessarily applying the changes.

These enhancements have *vastly* improved the navigability of the
admin interface between related objects.

Please have a look and let me know if you have concerns or suggestions
for improvement. The design decisions are documented in the tickets.

[1] http://share.simonmeers.com/django_related_changelinks/
[2] http://code.djangoproject.com/ticket/13163
[3] http://code.djangoproject.com/ticket/13165

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

[Simon Meers]

On 25 May 2010 07:50, Simon Meers <drm...@gmail.com> wrote:
>
> I've uploaded some screenshots [1] of the new patch for #13163 [2] and
> #13165 [3] in action, to allow people to see the affect without
> necessarily applying the changes.
>
> These enhancements have *vastly* improved the navigability of the
> admin interface between related objects.
>
> Please have a look and let me know if you have concerns or suggestions
> for improvement. The design decisions are documented in the tickets.
>
> [1] http://share.simonmeers.com/django_related_changelinks/
> [2] http://code.djangoproject.com/ticket/13163
> [3] http://code.djangoproject.com/ticket/13165

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?

[Russell Keith-Magee]

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 %-)

[Simon Meers]

> 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.

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

[Simon Meers]

>>  * 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.

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

[Russell Keith-Magee]

Sent from my iPad

On 09/06/2010, at 8:33 PM, Simon Meers <drm...@gmail.com> wrote:

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.

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.

OK - makes sense. 

 * 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...

Hrm. So this means that if I change the FK from John Smith to Bob Jones, the link continue to be a link to John? While I can see why its implemented like this, it seems less than ideal UI to me.

 * 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.

I can see your point. I'd be interested to get some input from someone with some UX credentials on this one. 

 * 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. 

I'll respond to this on your follow up email.

Yours,

Russ Magee %-)

[Russell Keith-Magee]

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 %-)