Opened 2 years ago

Closed 7 months ago

#19139 closed Bug (fixed)

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

Reported by: fcurella Owned by: claudep
Component: GIS Version: master
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 fcurella 2 years ago.
patch2.diff (3.2 KB) - added by fcurella 2 years ago.
patch3.diff (3.3 KB) - added by fcurella 2 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 2 years 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 2 years ago by fcurella

comment:2 Changed 2 years ago by fcurella

  • Has patch set

comment:3 Changed 2 years ago by fcurella

  • UI/UX set

comment:4 Changed 2 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

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

comment:5 Changed 2 years ago by lrekucki

  • Needs tests set
  • Patch needs improvement set

Changed 2 years ago by fcurella

comment:6 Changed 2 years 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 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 2 years ago by fcurella (previous) (diff)

comment:7 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by fcurella

comment:13 Changed 18 months ago by timo

  • Easy pickings unset

Patch no longer applies cleanly.

comment:15 Changed 7 months ago by timgraham

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

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

comment:16 Changed 7 months ago by claudep

  • Owner changed from nobody to claudep
  • Status changed from new to assigned

comment:17 Changed 7 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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