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

bugfix[DYN-6189]: remove duplicated code block node #14373

Merged
merged 3 commits into from
Sep 11, 2023
Merged

bugfix[DYN-6189]: remove duplicated code block node #14373

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4327e43
bugfix[library]: remove duplicated code block node
Enzo707 Sep 5, 2023
6cc762d
Merge branch 'DynamoDS:master' into DYN-6189/library/duplicated-code-…
Enzo707 Sep 8, 2023
ff409b7
fix(DynamoModel): included nodes search indexing
Enzo707 Sep 8, 2023
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
28 changes: 6 additions & 22 deletions src/DynamoCore/Models/DynamoModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1594,7 +1594,9 @@ private void InitializeIncludedNodes()
NodeFactory.AddTypeFactoryAndLoader(outputData.Type);
NodeFactory.AddAlsoKnownAs(outputData.Type, outputData.AlsoKnownAs);

SearchModel?.Add(new CodeBlockNodeSearchElement(cbnData, LibraryServices));
Copy link
Contributor

Choose a reason for hiding this comment

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

@Enzo707 after this change, can you test searching for code block node and see if it still works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang if you look at the gif it still appearing in library search and seems to work fine. There's any other place to look at?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, can you take a look at the in canvas search as well, just to make sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang it's failing within canvas search... I'm taking a look

Copy link
Contributor

Choose a reason for hiding this comment

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

@Enzo707 Any update on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang sorry for the delay, please take a look.

var cnbNode = new CodeBlockNodeSearchElement(cbnData, LibraryServices);
SearchModel?.Add(cnbNode);
AddNodeTypeToSearchIndex(cnbNode, iDoc);

var symbolSearchElement = new NodeModelSearchElement(symbolData)
{
Expand All @@ -1613,29 +1615,11 @@ private void InitializeIncludedNodes()
};

SearchModel?.Add(symbolSearchElement);
SearchModel?.Add(outputSearchElement);

//Adding this nodes are breaking the tests (due that we have two input and two output nodes):
//WhenHomeWorkspaceIsFocusedInputAndOutputNodesAreMissingFromSearch
//WhenStartingDynamoInputAndOutputNodesAreNolongerMissingFromSearch
// New index process from Lucene, adding missing nodes: Code Block, Input and Output
var ele = AddNodeTypeToSearch(outputData);
if (ele != null)
{
AddNodeTypeToSearchIndex(ele, iDoc);
}
AddNodeTypeToSearchIndex(symbolSearchElement, iDoc);
Copy link
Contributor

Choose a reason for hiding this comment

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

@RobertGlobant20 Didn't we add the check intentionally? Should we merge this only after we have the corresponding unit test added?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are refering to the check:
if (ele != null)
I think there is no need to check it due that we are not using the AddNodeTypeToSearch(outputData); method anymore (that could return null) and if you see the code some lines above you will see that those instances are created (see screenshot below).

The unit test only validates that the Input, Output and Code Block nodes are found with the Lucene Search results then it won't catch if they are duplicated visually in Library (As far as I remember Lucene Search doesn't return duplicated items), then I think this change can be merged

image


ele = AddNodeTypeToSearch(symbolData);
if (ele != null)
{
AddNodeTypeToSearchIndex(ele, iDoc);
}
SearchModel?.Add(outputSearchElement);
AddNodeTypeToSearchIndex(outputSearchElement, iDoc);

ele = AddNodeTypeToSearch(cbnData);
if (ele != null)
{
AddNodeTypeToSearchIndex(ele, iDoc);
}
}

internal static bool IsDisabledPath(string packagesDirectory, IPreferences preferences)
Expand Down