Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#11810 closed (fixed)

[gis] Typo in OpenLayers Admin JS

Reported by: rcoup Owned by: jbronn
Component: GIS Version: master
Severity: Keywords: admin gis osmgeoadmin
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

There's a minor typo in the GIS admin openlayers javascript, for GeoAdmins where modifiable=False.

Attachments (3)

11810.patch (596 bytes) - added by rcoup 6 years ago.
Against r11479
11810_v2.diff (2.0 KB) - added by jbronn 6 years ago.
11810-r12932.diff (6.3 KB) - added by ramiro 5 years ago.
Justin's patch modified to additionally tests if the field is in the new 1.2 readonly_fields ModelAdmin? option

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by rcoup

Against r11479

comment:1 Changed 6 years ago by jbronn

  • Keywords gis osmgeoadmin added
  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to jbronn
  • Patch needs improvement unset

Obviously this is a bug. Unfortunately, fixing the typo just exposes further bugs -- sigh, thanks rcoup :) . For example, if you already have a feature and modifiable=False, then the following JS error occurs (I'm using the WorldBorders model from the tutorial):

geodjango_mpoly.map.getControlsByClass("OpenLayers.Control.ModifyFeature")[0] is undefined

This is because the control wasn't enabled in the first place (now that the typo is fixed) -- thus, some more work is needed. I've attached a patch that addresses this additional problem, but I want to think about it a while. Some questions to ponder: do we still want to allow drawing of new features if modifiable=False (which the new patch allows)? Should this be an option on OSMGeoAdmin at all when there's the editable Field option?

Changed 6 years ago by jbronn

comment:2 Changed 6 years ago by jbronn

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 6 years ago by rcoup

I do try :) I wanted modifiable=False to act like a read-only field: just show me the geometry on a map. So it actually worked fine with the typo, except for the JS error in the browser console.

I guess this gets back into the read-only-admin-fields issue (#342). Personally, read-only fields make a tonne of sense and even more so for map data. editable=False means "don't display this at all in the Admin", which isn't quite the same thing.

comment:4 Changed 5 years ago by anonymous

  • Version changed from SVN to 1.2-beta

comment:5 Changed 5 years ago by anonymous

  • Version changed from 1.2-beta to SVN

Changed 5 years ago by ramiro

Justin's patch modified to additionally tests if the field is in the new 1.2 readonly_fields ModelAdmin? option

comment:6 Changed 5 years ago by jbronn

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

(In [12995]) Fixed #11810 -- Fixed typo and errors that prevented modifiable from working in the geographic admin. Thanks to Rob Coup for the bug report. Refs #12504.

comment:7 Changed 5 years ago by jbronn

(In [12996]) [1.1.X] Fixed #11810 -- Fixed typo and errors that prevented modifiable from working in the geographic admin. Thanks to Rob Coup for the bug report. Refs #12504.

Backport of r12995 from trunk.

comment:6 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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