False-y properties are not added to XML

[PizzaBrandon]

My colleague discovered a bug in the library when attempting to send a property with value false through, but noticed it wasn't in the transaction XML. I think I've identified the bug location: Line 110: properties[prop] ? cb(prop, properties[prop]) : pass();

This will pass over any false-y property value: null, undefined, 0, false, '', etc. Since there are cases where any of these values may be desired, with the exception of undefined, should this be changed to strictly check whether the value is undefined and then represent the other values in the XML?

[SKalt]

Yep. Patch it, add a test case, and I'll merge and publish tonight.

On Thu, Dec 21, 2017, 3:05 PM Brandon Belvin notifications@github.com wrote:

My colleague discovered a bug in the library when attempting to send a property with value false through, but noticed it wasn't in the transaction XML. I think I've identified the bug location: Line 110: properties[prop] ? cb(prop, properties[prop]) : pass(); https://github.com/SKalt/geojson-to-wfs-t-2/blob/master/geojson-to-wfst-2-es6.js#L110

This will pass over any false-y property value: null, undefined, 0, false, '', etc. Since there are cases where any of these values may be desired, with the exception of undefined, should this be changed to strictly check whether the value is undefined and then represent the other values in the XML?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SKalt/geojson-to-wfs-t-2/issues/9, or mute the thread https://github.com/notifications/unsubscribe-auth/AJ9G5cVCaBaU08gI1WURJN60SUd_rFPXks5tCrntgaJpZM4RKSjV .

[PizzaBrandon]

Thanks! We'll get started on developing the fix. I suspect it will be larger in scope since it has trickle-down effects into the XML generation code. I don't have a timeline for correction, but we'll PR it as soon as we have it working.

[PizzaBrandon]

I'm finishing up the patch now. Do you mind if I remove the "No gmlId supplied" warning? We see it consistently as our code runs.

[PizzaBrandon]

Never mind about gmlId, I guess that's coming from the other package.

[SKalt]

Thanks for the contrib! I'll see about purging the gmlId message from the gml converter.

[PizzaBrandon]

Thanks for the merge! If you are satisfied with the change, can you publish it to NPM?

[SKalt]

Published. I'm currently fighting through a flu and interview season, but expect the gml warning squashed and travis->nmp deployment set up in around a week.

[PizzaBrandon]

No worries about the message, it's not impacting the servers, just adding to the logs. Care for yourself first and get better soon!