This repository has been archived by the owner. It is now read-only.

Feature/detached published content context #8

Closed
wants to merge 5 commits into
base: develop
from

Conversation

3 participants
@Hendy
Copy link

Hendy commented Mar 25, 2015

Populating values for Parent and SortOrder on DetachedPublishedContent as per issue #7

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 25, 2015

Can we do Level = Parent.Level + 1?

Should we bother copying dates / creators / editors from parent node?

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 25, 2015

@mattbrailsford yes I think that'd do the trick (in the constructor) but I didn't add that, as don't yet have a NC within an NC to test...

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 25, 2015

Sorry, I mean, can you make the Level parameter for the DetachedPublishedContent return _parent.Level + 1. It currently still just returns 0 regardless. (sorry, should have been more clear)

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 25, 2015

sure, do you want the other properties duplicated from the parent too ? (WriterName, CreatorName etc..)

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 25, 2015

Yes please, where it makes sense to do so.

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 25, 2015

cool, will do... this.Umbraco.AssignedContentItem should never be null right ?

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 25, 2015

I'm not sure, it was something on my list to check as not seen it before. What is type is "this.Umbraco"?

@leekelleher

This comment has been minimized.

Copy link
Member

leekelleher commented Mar 25, 2015

You know, now that you've mentioned it - it's worth doing a null-reference check.

(We never know what context that the ValueConverter will be used - e.g. console app, Courier, etc)

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 25, 2015

how about I move the setting of the properties into the constructor, that way can do a single null check on the supplied Parent IPublishedContent ?

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 25, 2015

Yea, better safe than sorry (I need to check what would happen coming from DTGE model)

@leekelleher

This comment has been minimized.

Copy link
Member

leekelleher commented Mar 25, 2015

Hmmm, thinking about Umbraco.AssignedContentItem - that would be the containing node (as in proper/real node) ... so for NC being used within DTGE, it wouldn't be the container "fake" node, but the topmost "proper" node.

I think that's fine, right?

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 25, 2015

arh, in that case if it's the top most node, then will setting the Level for an NC within NC inception work ?

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 25, 2015

is there nothing passed into the converter that gives you a handle on the actual node? (can't recall if we did that in Ditto)

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 25, 2015

yes, we now have the context (arh, actually good point, I'm testing with Ditto 0.7.0 from MyGet)

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 25, 2015

this is only valid though if Umbraco core do the same (ie, pass in the node within a context) I was just wondering if they do or not.

null check on supplied Parent IPublishedContent value
setting properties from Parent (when available) onto DetachedPublishedContent
@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 25, 2015

what about making the DetachedPublishedContent class public so the current IPublishedContent (where ever that may be) can be easily tested to see if it's of type DetachedPublishedContent ? (I'm currently just checking to see if the ID = 0)

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 26, 2015

I've dropped an email to Shan about this. The way we have it now makes sense while the NC property exists on a real node, but the issue is, when NC is a property on a fake node (ie DTGE), the Umbraco.AssignedContentItem would be wrong. Internally, the AssignedContentItem is fetched from the context that is passed into the Umbraco helpers constructor (this is created in the value editor code using the current umbraco context) which would mean the contexts content item would be the top most REAL page, so if NC was a property on a DTGE, Umbraco.AssignedContentItem would be wrong. The only way I can think to get round this, and what I've emailed shan about is, within DTGE rendering, should it be ok to cache the current node in the context, change it during DTGE rendering, and then swap it back at the end. I know this is specific to DTGE here, but it's all key in order to get NC to work without having a special dependency on another plugin.

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 26, 2015

On further reflection, I think anything we do to force in an umbraco node as context is ultimately going to be a hack and therefore compromise the integrity of the package. I think the correct way for this to be resolved is to have the way the value converters worked be changed in core to pass in more contextual information similar to how standard .NET type converters do.

With that in mind, I've raised an issue here http://issues.umbraco.org/issue/U4-6465 and propose we hold off this update until this is addressed.

Anyone got any comments on this?

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 26, 2015

Is it just the .Level property that's in question ? if the AssignedContentItem isn't available (could we put in an additional check to ensure it's a real top level IPublishedContent ?) then none of the additional properties get mapped.

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Mar 26, 2015

Not really. The issue is, we are assuming that the umbraco node in the current context is the ultimate real node, which technically it, but only by coincidence. It would be better if the value converters were given a reference to the node that the property is being converted so it is explicit.

@Hendy

This comment has been minimized.

Copy link

Hendy commented Mar 27, 2015

The SortOrder should be correct tho, regardless of any supplied IPublishedContent.

@leekelleher

This comment has been minimized.

Copy link
Member

leekelleher commented Jun 25, 2015

@mattbrailsford Let's re-review this PR, (it's been a while), see if we can get it into v0.2.0.

@mattbrailsford

This comment has been minimized.

Copy link
Collaborator

mattbrailsford commented Jun 25, 2015

Ok, I've had another think about this, and I think there are other properties we could probably populate from AssignedContentItem, but these would only be things like writer name / creater name etc, more useful things like Parent / Level wouldn't not be assignable, as it could nested within other embedded content data types so these wouldn't be correct.

I think the only on worth setting now is as @Hendy said, the SortOrder. I'll add this in now.

mattbrailsford added a commit that referenced this pull request Jun 25, 2015

@leekelleher

This comment has been minimized.

Copy link
Member

leekelleher commented Jul 14, 2015

Closing this PR, the SortOrder is now populated (see commit a94e2e2).

We can open a new issue/PR if we want to discuss further.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.