GetTagsForEntityType should only return tags from not trashed entites and published content #15554
Conversation
|
Hi there @rammi987, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
There was a problem hiding this comment.
Thanks @rammi987 - just a note on performance and I have a few questions about the test that was updated.
| .InnerJoin<ContentVersionDto>().On<ContentVersionDto, NodeDto>((node, cv) => node.NodeId == cv.NodeId) | ||
| .InnerJoin<DocumentVersionDto>().On<DocumentVersionDto, ContentVersionDto>((dv, cv) => cv.Id == dv.Id && dv.Published); |
There was a problem hiding this comment.
Adding joins is not very performant, I know that in other areas of the CMS we look at the content path to see if the highest ancestor is the Id of the recycle bin (Id: -20, and for media -21 I believe, there's constants for these Ids). I would recommend trying that for this PR first.
I don't know if it would also make sense to do this for media items by the way, I assume they have the same problem?
There was a problem hiding this comment.
The is a few issuese here. We have entity for media, content and member.
The first thing is all can be trashed or not, so to exclude the the nodes witch are trashed i add to this line
https://github.com/umbraco/Umbraco-CMS/pull/15554/files/18823f3d8f7aca32a0a50883f558e64ef027e078#diff-6bfd8066a466dcb406442d00e5991f3f8a798dfa9976124fc128476f681fe88aR408 where i test if it trashed or not.
Therefor i dont need to look into the path or parentid.
The secound thing is the content can also be pulished and unpublished, and we only want the tags from content witch are pulished. The is no way to tell is a NodeDto is un-/ published without the inner joins.
Therefor i added in the condition if (objectType == TaggableObjectTypes.Content) to only make the inner joins if it is an entity for content. I know innerJoins is costly but the is no other way to get the PublishedState
There was a problem hiding this comment.
I see the reasoning for this, but actually when testing - at least as of now, on v13/dev - I don't see tags from unpublished pages included in my result. So I suspect we don't need to filter them out here, rather the act of unpublishing will remove records from the cmsTagRelationship table. This is done within the DocumentRepository.
So it doesn't look like we do need to include these additional joins, but for different reasons.
| content1.PublishCulture(CultureImpact.Invariant); | ||
| content1.PublishedState = PublishedState.Publishing; |
There was a problem hiding this comment.
I am not sure why this needs to be added here?
There was a problem hiding this comment.
if content1.PublishedState = PublishedState.Publishing; is not present the test content is acculty marked as an unpulished node, therefor the test will fail do to the Assert.AreEqual(3, result1.Length); result1.Length will be 0 becouse we dont get tag from unpublished content.
| @@ -711,6 +848,7 @@ public void Can_Get_Tags_For_Entity_Type_For_Group() | |||
| ContentTypeRepository.Save(contentType); | |||
|
|
|||
| var content1 = ContentBuilder.CreateSimpleContent(contentType); | |||
| content1.PublishedState = PublishedState.Publishing; | |||
There was a problem hiding this comment.
I am not sure why this needs to be added here?
There was a problem hiding this comment.
if content1.PublishedState = PublishedState.Publishing; is not present the test content is acculty marked as an unpulished node, therefor the test will fail do to the Assert.AreEqual(3, result1.Length); result1.Length will be 0 becouse we dont get tag from unpublished content.
|
Thanks for this PR @rammi987. I know it's sat for a while, but I came across the issue and saw you had fixed it, so would like to merge in so we get a fix for 13+. I think there have been some changes since you did this that means part isn't required, but I'll take the liberty of updating so we can merge in if that's OK. |
|
It'll actually be easier to close this one I'm afraid. I've pulled the parts we needed into #18164, as it made sense to fix this in 13, and thus we'll get it in higher versions when we merge up. Given this one was targeted at a higher version via the Hope that's OK @rammi987 and thanks again for the contribution. |
Prerequisites
If there's an existing issue for this PR then this fixes #15524
Description
When using GetAllContentTags or GetAllMediaTags it gets all tags even is entity is trashed or the content is unpublished.
We expect it to only return tags for content witch are published and aviable.
So as #15524 (comment) suggest is have add in the where clause to ensure only take tags from content when is it pulished and if entities is not trashed.
I have added 2 more unit test to ensure it works properly
So now then creating 2 content node and give both node a Tag, and render it with the GetAllContentTags it gives 2 tags items back. But do i unpulished or trash a node and ask the GetAllContentTags again it only return a single tag.