umco / umbraco-ditto

Ditto - the friendly view-model mapper for Umbraco
http://our.umbraco.org/projects/developer-tools/ditto
MIT License
79 stars 33 forks source link

Custom PublishedContent property nullifies value #118

Closed leekelleher closed 9 years ago

leekelleher commented 9 years ago

@Jeavon raised an issue on Gitter about Ditto not mapping the value of a property when using a custom IPublishedContent that has a property with the same name.

I have modified the UmbracoPropertyValueResolver to handle this scenario, (and included a supporting unit-test).

We first try to get the value from the IPublishedContent object property, then if the value is null, we make a call to the content's GetPropertyValue method.


@JimBobSquarePants @mattbrailsford Could either of you cast an eye over this change and scenario, (most as a sanity check), thanks.

@Jeavon If you want to try out a custom build from this branch to confirm that this works for you?

Jeavon commented 9 years ago

@leekelleher yup custom build works for us, thanks!

JimBobSquarePants commented 9 years ago

Looks good to me! :shipit:

mattbrailsford commented 9 years ago

If recursive is true, shouldn't it check all ancestor properties before checking Geypropertyvalue?

leekelleher commented 9 years ago

@mattbrailsford Trying to get my head around your question... do you mean ancestor properties of the inherited IPublishedContent object? :confused:

mattbrailsford commented 9 years ago

Yea. I'm not sure it's required actually, I just saw the recursive flag on the second if statement and wondered if it should be recursive on the first.

In theory, I'm not sure if a prop should be valueless, although I guess it can be hence the need for this PR.

leekelleher commented 9 years ago

I'll merge this in for a v0.8.3 patch release.

With the recursive question, I'm unsure what it means, but since this PR doesn't change how the values are retrieved, we can move the discussion to a future (0.9.0) release.

mattbrailsford commented 9 years ago

Agreed