Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetTagsForEntityType should only return tags from not trashed entites and published content #15554

Closed
wants to merge 3 commits into from
Closed

GetTagsForEntityType should only return tags from not trashed entites and published content #15554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47ff054
GetAllContentTags And GetAllMediaTags should not get trashed and unpu…
Jan 10, 2024
18823f3
Fixing a unit test, where content need a pulished status
Jan 10, 2024
52ebf90
Merge branch 'contrib' into git-15524
AndyButland Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TagRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,13 +394,20 @@ public IEnumerable<ITag> GetTagsForEntityType(TaggableObjectTypes objectType, st
{
Sql<ISqlContext> sql = GetTagsSql(culture, true);

if (objectType == TaggableObjectTypes.Content)
{
sql = sql
.InnerJoin<ContentVersionDto>().On<ContentVersionDto, NodeDto>((node, cv) => node.NodeId == cv.NodeId)
.InnerJoin<DocumentVersionDto>().On<DocumentVersionDto, ContentVersionDto>((dv, cv) => cv.Id == dv.Id && dv.Published);
Comment on lines +400 to +401
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

AddTagsSqlWhere(sql, culture);

if (objectType != TaggableObjectTypes.All)
{
Guid nodeObjectType = GetNodeObjectType(objectType);
sql = sql
.Where<NodeDto>(dto => dto.NodeObjectType == nodeObjectType);
.Where<NodeDto>(dto => dto.NodeObjectType == nodeObjectType && !dto.Trashed);
}

if (group.IsNullOrWhiteSpace() == false)
Expand Down
138 changes: 138 additions & 0 deletions ...co.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TagRepositoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,141 @@
}
}

[Test]
public void Can_Get_No_Tags_For_Content_Witch_Are_UnPublished()
{
var provider = ScopeProvider;
using (ScopeProvider.CreateScope())
{
var template = TemplateBuilder.CreateTextPageTemplate();
FileService.SaveTemplate(template);

var contentType = ContentTypeBuilder.CreateSimpleContentType("test", "Test", defaultTemplateId: template.Id);
ContentTypeRepository.Save(contentType);

var content1 = ContentBuilder.CreateSimpleContent(contentType);
content1.PublishCulture(CultureImpact.Invariant);
content1.PublishedState = PublishedState.Publishing;
DocumentRepository.Save(content1);

var content2 = ContentBuilder.CreateSimpleContent(contentType);
content2.PublishCulture(CultureImpact.Invariant);
content2.PublishedState = PublishedState.Unpublishing;
DocumentRepository.Save(content2);

var repository = CreateRepository(provider);
Tag[] tags =
{
new Tag {Text = "tag1", Group = "test"},
new Tag {Text = "tag2", Group = "test1"},
new Tag {Text = "tag3", Group = "test"}
};

repository.Assign(
content1.Id,
contentType.PropertyTypes.First().Id,
tags,
false);

repository.Assign(
content2.Id,
contentType.PropertyTypes.First().Id,
tags,
false);

var result1 = repository.GetTagsForEntityType(TaggableObjectTypes.Content).ToArray();

Assert.AreEqual(3, result1.Length);
Assert.AreEqual(1, result1.Single(x => x.Text == "tag1").NodeCount);
}
}

[Test]
public void Can_Get_Tags_For_Entity_Type_With_Trashed_Entity()
{
var provider = ScopeProvider;
using (ScopeProvider.CreateScope())
{
var template = TemplateBuilder.CreateTextPageTemplate();
FileService.SaveTemplate(template);

var contentType = ContentTypeBuilder.CreateSimpleContentType("test", "Test", defaultTemplateId: template.Id);
ContentTypeRepository.Save(contentType);

var content1 = ContentBuilder.CreateSimpleContent(contentType);
content1.PublishCulture(CultureImpact.Invariant);
content1.PublishedState = PublishedState.Publishing;
DocumentRepository.Save(content1);

var content2 = ContentBuilder.CreateSimpleContent(contentType);
content2.PublishCulture(CultureImpact.Invariant);
content2.PublishedState = PublishedState.Publishing;
content2.Trashed = true;
DocumentRepository.Save(content2);

var mediaType = MediaTypeBuilder.CreateImageMediaType("image2");
MediaTypeRepository.Save(mediaType);

var media1 = MediaBuilder.CreateMediaImage(mediaType, -1);
MediaRepository.Save(media1);

var media2 = MediaBuilder.CreateMediaImage(mediaType, -1);
media2.Trashed = true;
MediaRepository.Save(media2);

var repository = CreateRepository(provider);
Tag[] tags =
{
new Tag {Text = "tag1", Group = "test"},
new Tag {Text = "tag2", Group = "test1"},
new Tag {Text = "tag3", Group = "test"}
};

Tag[] tags2 =
{
new Tag {Text = "tag4", Group = "test"},
new Tag {Text = "tag5", Group = "test1"},
new Tag {Text = "tag6", Group = "test"}
};

repository.Assign(
content1.Id,
contentType.PropertyTypes.First().Id,
tags,
false);

repository.Assign(
content2.Id,
contentType.PropertyTypes.First().Id,
tags2,
false);

repository.Assign(
media1.Id,
contentType.PropertyTypes.First().Id,
tags,
false);

repository.Assign(
media2.Id,
contentType.PropertyTypes.First().Id,
tags2,
false);

var result1 = repository.GetTagsForEntityType(TaggableObjectTypes.Content).ToArray();
var result2 = repository.GetTagsForEntityType(TaggableObjectTypes.Media).ToArray();
var result3 = repository.GetTagsForEntityType(TaggableObjectTypes.All).ToArray();

Assert.AreEqual(3, result1.Length);
Assert.AreEqual(3, result2.Length);
Assert.AreEqual(6, result3.Length);

Assert.AreEqual(1, result1.Single(x => x.Text == "tag1").NodeCount);
Assert.AreEqual(2, result3.Single(x => x.Text == "tag1").NodeCount);
Assert.AreEqual(2, result3.Single(x => x.Text == "tag4").NodeCount);
}
}

Check warning on line 774 in tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TagRepositoryTest.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (contrib)

❌ New issue: Large Method 

Can_Get_Tags_For_Entity_Type_With_Trashed_Entity has 70 lines, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.

[Test]
public void Can_Get_Tags_For_Entity_Type()
{
Expand All @@ -654,6 +789,8 @@
ContentTypeRepository.Save(contentType);

var content1 = ContentBuilder.CreateSimpleContent(contentType);
content1.PublishCulture(CultureImpact.Invariant);
content1.PublishedState = PublishedState.Publishing;
Comment on lines +792 to +793
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this needs to be added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DocumentRepository.Save(content1);

var mediaType = MediaTypeBuilder.CreateImageMediaType("image2");
Expand Down Expand Up @@ -711,6 +848,7 @@
ContentTypeRepository.Save(contentType);

var content1 = ContentBuilder.CreateSimpleContent(contentType);
content1.PublishedState = PublishedState.Publishing;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this needs to be added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DocumentRepository.Save(content1);

var mediaType = MediaTypeBuilder.CreateImageMediaType("image2");
Expand Down
Loading