Code

Opened 21 months ago

Last modified 9 months ago

#19139 new Bug

OpenLayersWidget's 'Delete all Features' control doesn't respect GeoModelAdmin's modifiable attribute

Reported by: fcurella Owned by: nobody
Component: GIS Version: master
Severity: Normal Keywords: gis admin template
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Description

When GeoModelAdmin's modiable attribute is set to False, the controls for editing the geometry are not present on the change object page, but the 'Delete all Features' control is still there.

Attachments (3)

patch1.diff (784 bytes) - added by fcurella 21 months ago.
patch2.diff (3.2 KB) - added by fcurella 21 months ago.
patch3.diff (3.3 KB) - added by fcurella 16 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 21 months ago by fcurella

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from ``OpenLayersWidget``'s 'Delete all Features' control doesn't respect ``GeoModelAdmin``'s ``modifiable`` attribute to OpenLayersWidget's 'Delete all Features' control doesn't respect GeoModelAdmin's modifiable attribute

Changed 21 months ago by fcurella

comment:2 Changed 21 months ago by fcurella

  • Has patch set

comment:3 Changed 21 months ago by fcurella

  • UI/UX set

comment:4 Changed 21 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

Why do you test wkt == '' in your patch?

comment:5 Changed 21 months ago by lrekucki

  • Needs tests set
  • Patch needs improvement set

Changed 21 months ago by fcurella

comment:6 Changed 21 months ago by fcurella

  • Needs tests unset

@claudep: I was testing 'wkt' as a way to determine if the geometry was new or if it was an already existing one that needs to modified.

After running some tests I've realize that's the wrong attribute to check. On my new patch I pass the field's value to the template and I'm checking directly against that.

Version 0, edited 21 months ago by fcurella (next)

comment:7 Changed 21 months ago by claudep

Oh, now I understand that modifiable allows to create a feature, but not modify it afterwards. I didn't get it at first, and the docs are not clear either about this. This is something we should also fix.

Could you also enlighten me about the difference between testing wkt or value? (it's already late :-) )

comment:8 Changed 21 months ago by fcurella

wkt is 'a string representation of the geometry in WKT format' (https://docs.djangoproject.com/en/1.4/ref/contrib/gis/gdal/#django.contrib.gis.gdal.OGRGeometry.wkt), while value is the actual python object, whose __str__ (or __unicode__) method returns something like POLYGON((0 0, 5 0, 5 5, 0 5)).

I'd rather use value because wrt involves some geographical transformation, and could result in being '' due to errors while converting. (See https://github.com/django/django/blob/master/django/contrib/gis/admin/widgets.py#L67)

comment:9 Changed 16 months ago by claudep

Flavio, sorry for such a long time without news. I revisited this issue today, and I think we should set the modifiable param to True when the widget has no existing value (instead of adding the value). If you can add a new feature, there are no reason you cannot edit it before saving. Thoughts?

comment:10 Changed 16 months ago by fcurella

@claudep

if i'm reading your comment correctly, are you proposing to have the editing tools (including the 'delete all features' button) only on the 'add_view', but not in the 'change_view'?

That would make sense, since 'modifiable' has a different meaning than 'editable'. I think we may need to make this semantic difference more explicit in the docs.

comment:11 Changed 16 months ago by fcurella

sorry, I see what you're saying now. you're proposing to set self.params['modifiable'] = True instead of self.params['value'] = value in widgets.py, and then just use {% if modifiable %} in template, right?

If that's the case, I'm conflicted. On one hand, that's a simpler implementation. On the other hand, feels like hijacking a value explicitly set by the user. Maybe we could set self.params['editable'] = True and check against that?

comment:12 Changed 16 months ago by claudep

modifiable has already the meaning: can add new value, cannot edit existing value. Quoting the docs:

Defaults to True. When set to False, disables editing of existing geometry fields in the admin.

So we are simply pushing that logic a bit further down to the JS code.

Changed 16 months ago by fcurella

comment:13 Changed 9 months ago by timo

  • Easy pickings unset

Patch no longer applies cleanly.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.