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

DYN-4117: Delete Button missing from Package Manager preferences panel #12249

Merged
merged 8 commits into from
Nov 11, 2021

Conversation

sm6srw
Copy link
Contributor

@sm6srw sm6srw commented Nov 4, 2021

Purpose

This pull request does:

  • stop hiding the package delete command for packages with no scheduled operation.

DYN-4117

  • add a new tooltip message for the case when delete is disabled because custom nodes from the package are in use.

Declaration

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

(FILL ME IN) Brief description of the fix / enhacement. Mandatory section

@sm6srw sm6srw changed the title DYN-4117: DYN-4117: Delete Button missing from Package Manager preferences panel Nov 4, 2021
@QilongTang
Copy link
Contributor

Thank you for restoring it, I love this function!

@sm6srw sm6srw requested a review from mjkkirschner November 10, 2021 13:21
&& Model.LoadState.ScheduledState != PackageLoadState.ScheduledTypes.ScheduledForDeletion
&& !CanUninstall())
{
return Resources.PackageContextMenuDeletePackageCustomNodesInUseTooltip;
Copy link
Contributor

Choose a reason for hiding this comment

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

@sm6srw By custom nodes ...do you mean non-built-in ? or custom node definitions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Custom Node definitions

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm..I do not see any specific checks for custom node defs...is that the only possibility?...what about packages with loaded assemblies...

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 check is in CanUnInstall(). It is a little strange because we say that a package can always be uninstalled if the package has a loaded assembly. But it can not be uninstalled if it has custom nodes that are in use. We missed that case. What I think is missing is to do the same thing for all packages (allow or not allow). But that is future work. For now we just try to clarify this a bit.

Comment on lines +41 to +50
public bool LoadedWithNoScheduledOperation
{
get
{
return Model.BuiltInPackage ?
Model.LoadState.State != PackageLoadState.StateTypes.Unloaded &&
Model.LoadState.ScheduledState != PackageLoadState.ScheduledTypes.ScheduledForUnload :
Model.LoadState.ScheduledState != PackageLoadState.ScheduledTypes.ScheduledForDeletion;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this condition different from simply checking Model.LoadState.State == PackageLoadState.StateTypes.Loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not 100% sure why we can't check for Loaded only but I think it has to do with packages in an error state. I just followed the existing pattern and made a property of it so I can use it here and for viability control in the xaml file.

Comment on lines 125 to 126
if (Model.LoadState.State != PackageLoadState.StateTypes.Unloaded
&& Model.LoadState.ScheduledState != PackageLoadState.ScheduledTypes.ScheduledForDeletion
Copy link
Contributor

Choose a reason for hiding this comment

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

How about just checking for if(!CanUininstall())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that should work I think. The control will be hidden in those cases when this matter. I will give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works fine.

@sm6srw sm6srw merged commit 42b75bb into DynamoDS:master Nov 11, 2021
@sm6srw sm6srw deleted the DYN-4117 branch November 11, 2021 13:19
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.

4 participants