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

Jump to bottom

fix dyn and image loading from package docs folders. (and some other things) #14554

Merged
merged 11 commits into from
Nov 6, 2023
Merged

fix dyn and image loading from package docs folders. (and some other things) #14554

merged 11 commits into from
Nov 6, 2023

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Nov 1, 2023 •
edited
Loading

Purpose

  1. This PR re-enables the Documentation browser tests
  2. They were all failing because the test runner was crashing - We were using WPF DoEevnts in user code - a dangerous thing to do, I removed that, and used the dispatcher with two separate tasks to fix the timing issue.
  3. After doing this I found that these two prs - DYN-6010 Enable Node Help Docs Sharing DYN files #14441 and DYN-6010 Enable Node Help Docs Sharing Images #14429 broke image and dyn loading from individual package /doc folders.
  4. I fixed that by attempting to determine if the node which is having docs requested is a package node or not and setting the virtual host name to folder mapping depending on that.
  5. Added a test for loading .dyns from a package.
  6. PackageManagerConflictsUnloadedWithBltInPackage test - I have no idea how this was passing before, I think the mocked method was getting invoked after the test ended, I was able to see it fail even in 2.18 before the test was marked failure - fixed it.

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

@mjkkirschner mjkkirschner requested review from RobertGlobant20, pinzart90 and dnenov and removed request for RobertGlobant20 November 1, 2023 23:04
@mjkkirschner
Copy link
Member Author

looking into the test failures, of course they pass locally.

@QilongTang QilongTang added this to the 3.0 milestone Nov 2, 2023
@mjkkirschner
Copy link
Member Author

There are failures on this branch from my latest changes that I need to fix, please do not merge.

@mjkkirschner mjkkirschner added the DNM Do not merge. For PRs. label Nov 3, 2023
RobertGlobant20
RobertGlobant20 approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@RobertGlobant20 RobertGlobant20 left a comment

Choose a reason for hiding this comment

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

LGTM with some comments

src/DocumentationBrowserViewExtension/DocumentationBrowserView.xaml.cs Show resolved Hide resolved
src/DocumentationBrowserViewExtension/DocumentationBrowserView.xaml.cs Show resolved Hide resolved
src/DocumentationBrowserViewExtension/DocumentationBrowserViewModel.cs
@@ -264,13 +273,13 @@ private void HandleLocalResource(OpenDocumentationLinkEventArgs e)
this.shouldLoadDefaultContent = false;
}

private string GetGraphLinkFromMDLocation(Uri link)
private string GetGraphLinkFromMDLocation(Uri link,bool isOwnedByPackage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for curiosity you already defined the property IsOwnedByPackage then why passing it as a parameter to a method?

Copy link
Member Author

@mjkkirschner mjkkirschner Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good eye, and this is very important because unfortunately the singleton nature of the Docs browser model makes it very easy to access data which has not yet been set correctly.

For example before I made this change it was possible for me to reference isOwnedByPackage before it had even been calculated for the node docs request we were parsing. By using the param as a dependency it's clear to callers that the method will be using it and it should be up to date.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth making it a class member in that case?

@mjkkirschner
Copy link
Member Author

Tests are passing now, any objections to merging this @pinzart90 @dnenov ?

@mjkkirschner mjkkirschner removed the DNM Do not merge. For PRs. label Nov 6, 2023
@mjkkirschner mjkkirschner merged commit 81a68f6 into DynamoDS:master Nov 6, 2023
18 checks passed
@mjkkirschner mjkkirschner deleted the pkgtest branch November 6, 2023 15:46
aparajit-pratap
aparajit-pratap reviewed Nov 6, 2023
View reviewed changes
test/DynamoCoreWpfTests/ViewExtensions/DocumentationBrowserViewExtensionTests.cs
//Validates that we have 5 nodes the CurrentWorkspace (after the graph was added)
Assert.AreEqual(ViewModel.Model.CurrentWorkspace.Nodes.Count(), 5);
//Assert.AreEqual(ViewModel.Model.CurrentWorkspace.Nodes.Count(), 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

??

aparajit-pratap
aparajit-pratap reviewed Nov 6, 2023
View reviewed changes
test/DynamoCoreWpfTests/ViewExtensions/DocumentationBrowserViewExtensionTests.cs

var graphPathValue = docsViewModel.GraphPath;

var dynFileName = Path.GetFileNameWithoutExtension(docsViewModel.Link.AbsolutePath) + ".dyn";
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to add an assertion for dynFileName?

@mjkkirschner mjkkirschner mentioned this pull request Nov 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aparajit-pratap aparajit-pratap aparajit-pratap left review comments

@dnenov dnenov dnenov left review comments

@RobertGlobant20 RobertGlobant20 RobertGlobant20 approved these changes

@pinzart90 pinzart90 pinzart90 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@mjkkirschner @dnenov @aparajit-pratap @pinzart90 @RobertGlobant20 @QilongTang