Opened 8 years ago

Closed 8 years ago

#142 closed defect (fixed)

feature store is too aggressive with default values

Reported by: tschaub Owned by:
Priority: major Milestone: 0.6
Component: GeoExt.data.FeatureStore Version: trunk
Keywords: Cc:
State: Commit

Description

In the onFeatureModified method, the feature reader overwrites any attribute values that evaluate to false with the field defaults. This means that attribute values cannot be updated to things like "" and 0.

Attachments (1)

142.patch (1.9 KB) - added by tschaub 8 years ago.
limit what is updated

Download all attachments as: .zip

Change History (6)

Changed 8 years ago by tschaub

limit what is updated

comment:1 Changed 8 years ago by tschaub

  • State changed from None to Review

Tests demonstrate the problem. With changes, all pass. Thanks for the review.

comment:2 Changed 8 years ago by elemoine

if key isn't in attributes, shouldn't we do record.set(field.name, field.defaultValue)?

comment:3 follow-up: Changed 8 years ago by tschaub

This patch changes what happens in the featuremodified listener. This listener should get called when the feature geometry or any attribute changes (which imply a state change in some cases). The purpose of the listener is to change field values in the record when the attribute is updated. The first section of the listener is about changing field values based on attribute changes. The second section is about changing geometry, state, or fid.

In the section of the listener that deals with updated attributes, it doesn't make any sense to do anything with field names (or mappings) that aren't attribute keys. Doing something like setting default field values might make sense in a reader, but I don't think it makes sense to be setting default values for fields that don't map to attribute keys in the portion of the listener that deals with updating attribute related fields.

Eric, if you think it makes sense to be setting default field values when a feature is modified, please explain where this is important.

comment:4 in reply to: ↑ 3 Changed 8 years ago by elemoine

  • State changed from Review to Commit

Replying to tschaub:

This patch changes what happens in the featuremodified listener. This listener should get called when the feature geometry or any attribute changes (which imply a state change in some cases). The purpose of the listener is to change field values in the record when the attribute is updated. The first section of the listener is about changing field values based on attribute changes. The second section is about changing geometry, state, or fid.

In the section of the listener that deals with updated attributes, it doesn't make any sense to do anything with field names (or mappings) that aren't attribute keys. Doing something like setting default field values might make sense in a reader, but I don't think it makes sense to be setting default values for fields that don't map to attribute keys in the portion of the listener that deals with updating attribute related fields.

Eric, if you think it makes sense to be setting default field values when a feature is modified, please explain where this is important.

Thanks for the explanation. I hadn't realized that this code was executed on featuremodified events only. This is making sense, please commit.

comment:5 Changed 8 years ago by tschaub

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

(In [1324]) Making it so the featuremodified listener doesn't set default values for keys that don't exist in feature.attributes. r=elemoine (closes #142)

Note: See TracTickets for help on using tickets.