raise notification at startup if installed package targets a different host. #12990

mjkkirschner · 2022-06-09T22:02:34Z

Purpose

https://jira.autodesk.com/browse/DYN-4884
Follow up for the work @sm6srw started: https://jira.autodesk.com/browse/DYN-4880

PR does the following:

refactors host conflict check into package manager extension and out of package manager client view model.
updates some tests
At startup, raises notifications if the check fails for any packages that installed already. I went with notifications over message boxes to avoid irritating/overwhelming users because these notifications will show up at each startup.

notification after install.

notification when adding package path.

In addition to these cases, startup also generates a notification of course.

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

Release Notes

Add notifications at startup for packages that target a different host than current Dynamo context.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

refactor where check is fix existing tests

src/DynamoPackages/PackageManagerExtension.cs

src/DynamoCore/Extensions/ReadyParams.cs

src/DynamoPackages/PackageManagerExtension.cs

src/DynamoCoreWpf/Properties/Resources.resx

… into customnodefail

aparajit-pratap · 2022-06-13T20:12:00Z

Last question, can we perform these checks for case (2) When a new package directory is added pointed out by Jorgen here https://jira.autodesk.com/browse/DYN-4884?

mjkkirschner · 2022-06-13T20:22:38Z

Last question, can we perform these checks for case (2) When a new package directory is added pointed out by Jorgen here https://jira.autodesk.com/browse/DYN-4884?

yes, I think theres no reason we can't perform these same checks then, I would likely do it at late package load instead of directly tying it to path updates, so that PackageManagerViewExtension doesn't need to know about the preferences dialog, or package path updates - it just does these checks whenever a package is loaded, this has one consequences I can think of.

we'll get some notifications added even if a user already got a message box telling them about the potential issue, I guess this isn't so bad, because now there will be a logged record of the package for us to find the dynamo log.

I'll give this a shot.

add test

mjkkirschner · 2022-06-13T21:57:03Z

@aparajit-pratap PTAL - implemented notifications when adding package paths.

aparajit-pratap · 2022-06-14T15:12:09Z

src/DynamoCoreWpf/Properties/Resources.en-US.resx

@@ -3229,4 +3229,13 @@ You can manage this in Preferences -&gt; Security.</value>
  <data name="TrustLocationSkippedNotification" xml:space="preserve">
    <value>No trusted location has been added.</value>
  </data>
+  <data name="MessagePackageTargetOtherHosts2" xml:space="preserve">
+    <value>The package or one of its dependencies targets a different environment, such as Revit, Civil 3D, Advance Steel, Alias or FormIt. This can cause instability and unexpected problems.</value>


Can we be specific about which host specifically the package is targeting rather than saying it could be one of the many?

That will require refactoring the functions which check what the targets are - I think it gets a bit complicated for two reasons.

In some instances we are checking a package and all its dependencies - I guess it's unlikely they depend on different hosts. Probably a strange edge case.

The current implementation just bails when it finds any issue, so we can return that first reason for failure I think fairly easily, but building up the complete list of different targets could possibly make the search quite slow in some cases (lots of dependencies) - and potentially confusing.

Is it okay to just return something like:

The package or one of its dependencies targets {X} which is different from the current Dynamo environment.This can cause instability and unexpected problems.

Or do we need to ensure the list is complete to include all targets which we reasoned were a failure?

Don't we have that information already? Isn't it how we are calculating stuff in CheckIfPackagesTargetOtherHosts?

Yes, I think I meant just the first host.

aparajit-pratap · 2022-06-14T15:14:41Z

src/DynamoPackagesWPF/PackageManagerViewExtension.cs

+        {
+            add
+            {
+             notificationLogged+=value;


src/DynamoPackagesWPF/PackageManagerViewExtension.cs

aparajit-pratap

@mjkkirschner thanks for responding to my questions. As far as the message is concerned about the non-matching host, feel free to disregard it if it's too difficult a refactor. It's a minor point as I thought it might be nice to have that additional info for the user as to what host their downloaded package is actually targeting.

mjkkirschner · 2022-06-15T14:59:17Z

@aparajit-pratap @QilongTang given the timing, I will merge this, and cherry pick to 2.15 given this task is scoped for release in jira.

I can attempt to make improvements to the message in master.

mjkkirschner · 2022-06-15T16:46:09Z

pass here:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/6375/

I ran it a few more times and sometimes it looks like another one of those issues with checking ContextMenuPopUp.IsOpen being flaky... it does not fail consistently, fairly certain we've seen this failure before.
I don't think it's related to these changes.

…t host. (DynamoDS#12990) * add notifications for packages with other host at startup time refactor where check is fix existing tests * fix and better notification * add test * fix tests * comments * Update PackageManagerExtension.cs * Update PackageManagerViewExtensionTests.cs * typo * also check hosts at package load add test

…t host. (#12990) (#13010) * add notifications for packages with other host at startup time refactor where check is fix existing tests * fix and better notification * add test * fix tests * comments * Update PackageManagerExtension.cs * Update PackageManagerViewExtensionTests.cs * typo * also check hosts at package load add test

mzjensen · 2022-12-01T16:43:29Z

Hi @mjkkirschner,

I noticed this new notification now that Dynamo Core 2.15 is shipped with the latest Civil 3D update (2023.1). It's convenient that you were doing your testing with my package as well! I find the message a little confusing because it says that my package targets a different host, which would be true if it was installed for Sandbox. But why does this message show in Dynamo for Civil 3D? Is it a bug, or do I need to change something in my pkg.json file? My ultimate goal is for this notification to not appear for Civil 3D users so they don't wonder if they did something wrong.

mjkkirschner · 2022-12-01T16:51:10Z

Hi @mzjensen that sounds like a bug because it looks like your package already marks a host dependency on Civil3d. I will file a bug to investigate.

mzjensen · 2022-12-01T17:03:31Z

@mjkkirschner - sounds good, thanks. FWIW, I also see the same message when installing the IronPython 2.7 package, which doesn't seem right.

mjkkirschner · 2022-12-01T17:05:33Z

yes, I think if you were to debug you would see that Civil 3D is providing an unexpected host name...

mjkkirschner · 2022-12-01T17:13:34Z

at the moment I'm not clear if this will work - but as a workaround you could try not setting the host dependencies to anything, and publishing locally to see if that works to avoid the notification.

mjkkirschner added 2 commits June 9, 2022 17:05

add notifications for packages with other host at startup time

eeca697

refactor where check is fix existing tests

fix and better notification

23c070e

mjkkirschner changed the title ~~raise notification at startup if installed package targets a different host.~~ WIP raise notification at startup if installed package targets a different host. Jun 9, 2022

mjkkirschner added 2 commits June 10, 2022 11:58

add test

74507bb

fix tests

8e917e1

mjkkirschner requested review from QilongTang and aparajit-pratap June 10, 2022 18:24

mjkkirschner changed the title ~~WIP raise notification at startup if installed package targets a different host.~~ raise notification at startup if installed package targets a different host. Jun 10, 2022

zeusongit reviewed Jun 13, 2022

View reviewed changes

src/DynamoPackages/PackageManagerExtension.cs Outdated Show resolved Hide resolved

QilongTang reviewed Jun 13, 2022

View reviewed changes

src/DynamoCore/Extensions/ReadyParams.cs Show resolved Hide resolved

comments

12163bd

aparajit-pratap reviewed Jun 13, 2022

View reviewed changes

src/DynamoPackages/PackageManagerExtension.cs Show resolved Hide resolved

mjkkirschner added 2 commits June 13, 2022 15:47

Update PackageManagerExtension.cs

c5a6853

Update PackageManagerViewExtensionTests.cs

e4537a0

aparajit-pratap reviewed Jun 13, 2022

View reviewed changes

src/DynamoCoreWpf/Properties/Resources.resx Show resolved Hide resolved

aparajit-pratap approved these changes Jun 13, 2022

View reviewed changes

mjkkirschner added 2 commits June 13, 2022 16:10

typo

a46c213

Merge branch 'customnodefail' of https://github.com/mjkkirschner/Dynamo…

8a9947f

… into customnodefail

also check hosts at package load

1a38fca

add test

aparajit-pratap reviewed Jun 14, 2022

View reviewed changes

src/DynamoPackagesWPF/PackageManagerViewExtension.cs

{

add

{

notificationLogged+=value;

Copy link

Contributor

aparajit-pratap Jun 14, 2022

Choose a reason for hiding this comment

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

formatting

aparajit-pratap reviewed Jun 14, 2022

View reviewed changes

src/DynamoPackagesWPF/PackageManagerViewExtension.cs Show resolved Hide resolved

aparajit-pratap approved these changes Jun 14, 2022

View reviewed changes

mjkkirschner merged commit f928a6f into DynamoDS:master Jun 15, 2022

mjkkirschner deleted the customnodefail branch June 15, 2022 17:23

mjkkirschner mentioned this pull request Jun 15, 2022

cherry pick raise notification at startup if installed package targets a different host #13010

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raise notification at startup if installed package targets a different host. #12990

raise notification at startup if installed package targets a different host. #12990

mjkkirschner commented Jun 9, 2022 •

edited

Loading

aparajit-pratap commented Jun 13, 2022 •

edited

Loading

mjkkirschner commented Jun 13, 2022 •

edited

Loading

mjkkirschner commented Jun 13, 2022

aparajit-pratap Jun 14, 2022

mjkkirschner Jun 14, 2022 •

edited

Loading

aparajit-pratap Jun 14, 2022

aparajit-pratap Jun 14, 2022

aparajit-pratap Jun 14, 2022

aparajit-pratap left a comment

mjkkirschner commented Jun 15, 2022

mjkkirschner commented Jun 15, 2022 •

edited

Loading

mzjensen commented Dec 1, 2022

mjkkirschner commented Dec 1, 2022 •

edited

Loading

mzjensen commented Dec 1, 2022

mjkkirschner commented Dec 1, 2022

mjkkirschner commented Dec 1, 2022

raise notification at startup if installed package targets a different host. #12990

raise notification at startup if installed package targets a different host. #12990

Conversation

mjkkirschner commented Jun 9, 2022 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

aparajit-pratap commented Jun 13, 2022 • edited Loading

mjkkirschner commented Jun 13, 2022 • edited Loading

mjkkirschner commented Jun 13, 2022

aparajit-pratap Jun 14, 2022

Choose a reason for hiding this comment

mjkkirschner Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Jun 14, 2022

Choose a reason for hiding this comment

aparajit-pratap Jun 14, 2022

Choose a reason for hiding this comment

aparajit-pratap Jun 14, 2022

Choose a reason for hiding this comment

aparajit-pratap left a comment

Choose a reason for hiding this comment

mjkkirschner commented Jun 15, 2022

mjkkirschner commented Jun 15, 2022 • edited Loading

mzjensen commented Dec 1, 2022

mjkkirschner commented Dec 1, 2022 • edited Loading

mzjensen commented Dec 1, 2022

mjkkirschner commented Dec 1, 2022

mjkkirschner commented Dec 1, 2022

mjkkirschner commented Jun 9, 2022 •

edited

Loading

aparajit-pratap commented Jun 13, 2022 •

edited

Loading

mjkkirschner commented Jun 13, 2022 •

edited

Loading

mjkkirschner Jun 14, 2022 •

edited

Loading

mjkkirschner commented Jun 15, 2022 •

edited

Loading

mjkkirschner commented Dec 1, 2022 •

edited

Loading