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.

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

Conversation

Enzo707
Copy link
Contributor

@Enzo707 Enzo707 commented Sep 5, 2023

Screen Recording 2023-09-08 at 14 57 54

Purpose

This PR is for removing the duplicated "code block" node in Library.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

There was a double indexing statement of the code block node within DynamoModel

Reviewers

@QilongTang
@RobertGlobant20

FYIs

@avidit

@Enzo707 Enzo707 changed the title bugfix[library]: remove duplicated code block node bugfix[DYN-6189]: remove duplicated code block node Sep 5, 2023
@@ -1594,7 +1594,6 @@ 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.

{
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

@QilongTang QilongTang merged commit 93ffb7e into DynamoDS:master Sep 11, 2023
@QilongTang QilongTang added this to the 3.0 milestone Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants