Opened 11 years ago

Closed 10 years ago

#19139 closed Bug (fixed)

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

Reported by: Flavio Curella Owned by: Claude Paroz
Component: GIS Version: dev
Severity: Normal Keywords: gis admin template
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 Flavio Curella 11 years ago.
patch2.diff (3.2 KB ) - added by Flavio Curella 11 years ago.
patch3.diff (3.3 KB ) - added by Flavio Curella 11 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by Flavio Curella, 11 years ago

Summary: ``OpenLayersWidget``'s 'Delete all Features' control doesn't respect ``GeoModelAdmin``'s ``modifiable`` attributeOpenLayersWidget's 'Delete all Features' control doesn't respect GeoModelAdmin's modifiable attribute

by Flavio Curella, 11 years ago

Attachment: patch1.diff added

comment:2 by Flavio Curella, 11 years ago

Has patch: set

comment:3 by Flavio Curella, 11 years ago

UI/UX: set

comment:4 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

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

comment:5 by Łukasz Rekucki, 11 years ago

Needs tests: set
Patch needs improvement: set

by Flavio Curella, 11 years ago

Attachment: patch2.diff added

comment:6 by Flavio Curella, 11 years ago

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

Last edited 11 years ago by Flavio Curella (previous) (diff)

comment:7 by Claude Paroz, 11 years ago

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 by Flavio Curella, 11 years ago

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 by Claude Paroz, 11 years ago

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 by Flavio Curella, 11 years ago

@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 by Flavio Curella, 11 years ago

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 by Claude Paroz, 11 years ago

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.

by Flavio Curella, 11 years ago

Attachment: patch3.diff added

comment:13 by Tim Graham, 10 years ago

Easy pickings: unset

Patch no longer applies cleanly.

comment:15 by Tim Graham, 10 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Claude, as someone with more experience with GIS, could you check this?

comment:16 by Claude Paroz, 10 years ago

Owner: changed from nobody to Claude Paroz
Status: newassigned

comment:17 by Claude Paroz <claude@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In ce078ef976d83395dc19c9a1576529a5a0678514:

Fixed #19139 -- Made OpenLayersWidget follow GeoModelAdmin's modifiable attribute

Thanks Tim Graham for the review.

Note: See TracTickets for help on using tickets.
Back to Top