-
Notifications
You must be signed in to change notification settings - Fork 639
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-5709: Pm publish version patch request #15395
base: master
Are you sure you want to change the base?
Changes from 4 commits
8fc7e96
d1bbc5e
2c0eff3
09a2a0a
9fef12d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -180,7 +180,7 @@ public PackageViewModel(DynamoViewModel dynamoViewModel, Package model) | |
Model = model; | ||
|
||
PublishNewPackageVersionCommand = new DelegateCommand(() => ExecuteWithTou(PublishNewPackageVersion), IsOwner); | ||
PublishNewPackageCommand = new DelegateCommand(() => ExecuteWithTou(PublishNewPackage), IsOwner); | ||
PublishNewPackageCommand = new DelegateCommand(() => ExecuteWithTou(PublishNewPackage), CanPublishNewPackage); | ||
UninstallCommand = new DelegateCommand(Uninstall, CanUninstall); | ||
UnmarkForUninstallationCommand = new DelegateCommand(UnmarkForUninstallation, CanUnmarkForUninstallation); | ||
LoadCommand = new DelegateCommand(Load, CanLoad); | ||
|
@@ -410,6 +410,25 @@ private bool IsOwner() | |
return packageManagerClient.DoesCurrentUserOwnPackage(Model, dynamoModel.AuthenticationManager.Username); | ||
} | ||
|
||
private bool CanPublishNewPackage() | ||
{ | ||
if (!CanPublish) return false; | ||
|
||
return packageManagerClient.DoesCurrentUserOwnPackage(Model, dynamoModel.AuthenticationManager.Username) || | ||
PackageNotPublishedAndUserIsHolder(Model, dynamoModel.AuthenticationManager.Username); | ||
} | ||
|
||
|
||
// Utility function to assert if a local package can be published | ||
// If the current user is the package holder and a package with that name has not been published yet, return true | ||
private bool PackageNotPublishedAndUserIsHolder(Package package, string username) | ||
{ | ||
bool userIsHolder = package.CopyrightHolder != null && package.CopyrightHolder.Equals(username); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about renaming this function as |
||
bool packageHasNotBeenPublished = !this.dynamoViewModel.PackageManagerClientViewModel.CachedPackageList.Any(x => x.Name == package.Name); | ||
|
||
return userIsHolder && packageHasNotBeenPublished; | ||
} | ||
|
||
private bool CanDeprecate() | ||
{ | ||
var isDeprecated = IsPackageDeprecated(Model.Name); | ||
|
@@ -438,15 +457,17 @@ private void PublishNewPackageVersion() | |
Model.RefreshCustomNodesFromDirectory(dynamoModel.CustomNodeManager, DynamoModel.IsTestMode); | ||
var vm = PublishPackageViewModel.FromLocalPackage(dynamoViewModel, Model, true); | ||
vm.IsNewVersion = true; | ||
vm.IsPackageInstalled = true; | ||
|
||
dynamoViewModel.OnRequestPackagePublishDialog(vm); | ||
} | ||
|
||
private void PublishNewPackage() | ||
{ | ||
Model.RefreshCustomNodesFromDirectory(dynamoModel.CustomNodeManager, DynamoModel.IsTestMode); | ||
var vm = PublishPackageViewModel.FromLocalPackage(dynamoViewModel, Model, false); | ||
var vm = PublishPackageViewModel.FromLocalPackage(dynamoViewModel, Model, true); | ||
vm.IsNewVersion = false; | ||
vm.IsPackageInstalled = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this always be true? In the case of publishing a new package, the new package doesn't need to always be loaded, does it? |
||
|
||
dynamoViewModel.OnRequestPackagePublishDialog(vm); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -211,7 +211,25 @@ | |
} | ||
} | ||
} | ||
|
||
|
||
/// <summary> | ||
/// IsPackageInstalled property </summary> | ||
/// <value> | ||
/// Shows if the package is already installed </value> | ||
private bool _isPackageInstalled = false; | ||
public bool IsPackageInstalled | ||
{ | ||
get { return _isPackageInstalled; } | ||
Check failure on line 222 in src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs
|
||
set | ||
Check failure on line 223 in src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs
|
||
{ | ||
if (_isPackageInstalled != value) | ||
{ | ||
_isPackageInstalled = value; | ||
RaisePropertyChanged(nameof(IsPackageInstalled)); | ||
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// CanEditName property </summary> | ||
/// <value> | ||
|
@@ -956,8 +974,8 @@ | |
customNodeDefinitions = new List<CustomNodeDefinition>(); | ||
SubmitCommand = new DelegateCommand(Submit, CanSubmit); | ||
PublishLocallyCommand = new DelegateCommand(PublishLocally, CanPublishLocally); | ||
ShowAddFileDialogAndAddCommand = new DelegateCommand(ShowAddFileDialogAndAdd, CanShowAddFileDialogAndAdd); | ||
SelectDirectoryAndAddFilesRecursivelyCommand = new DelegateCommand(SelectDirectoryAndAddFilesRecursively); | ||
ShowAddFileDialogAndAddCommand = new DelegateCommand(ShowAddFileDialogAndAdd, CanAddFiles); | ||
SelectDirectoryAndAddFilesRecursivelyCommand = new DelegateCommand(SelectDirectoryAndAddFilesRecursively, CanAddFiles); | ||
SelectMarkdownDirectoryCommand = new DelegateCommand(SelectMarkdownDirectory); | ||
ClearMarkdownDirectoryCommand = new DelegateCommand(ClearMarkdownDirectory); | ||
CancelCommand = new DelegateCommand(Cancel); | ||
|
@@ -1364,7 +1382,7 @@ | |
{ | ||
if (e.PropertyName == "PackageContents") | ||
{ | ||
CanSubmit(); | ||
CanSubmit(); | ||
SubmitCommand.RaiseCanExecuteChanged(); | ||
PublishLocallyCommand.RaiseCanExecuteChanged(); | ||
} | ||
|
@@ -1493,6 +1511,7 @@ | |
/// <summary> | ||
/// The method is used to create a PublishPackageViewModel from a Package object. | ||
/// If retainFolderStructure is set to true, the folder structure of the package will be retained. Else, the default folder structure will be imposed. | ||
/// Investigating if both options (publish and publish new version) should not use 'retainFolderStructure' with disabled package files and folders editing | ||
/// </summary> | ||
/// <param name="dynamoViewModel"></param> | ||
/// <param name="pkg">The package to be loaded</param> | ||
|
@@ -2117,9 +2136,9 @@ | |
} | ||
} | ||
|
||
private bool CanShowAddFileDialogAndAdd() | ||
private bool CanAddFiles() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just remove this redundant property (it's just the inverse of |
||
{ | ||
return true; | ||
return !IsPackageInstalled; | ||
} | ||
|
||
internal void AddFile(string filename) | ||
|
@@ -2267,7 +2286,15 @@ | |
{ | ||
// begin submission | ||
var pmExtension = dynamoViewModel.Model.GetPackageManagerExtension(); | ||
var handle = pmExtension.PackageManagerClient.PublishAsync(Package, RetainFolderStructureOverride ? updatedFiles : contentFiles, MarkdownFiles, IsNewVersion, CurrentPackageRootDirectories, RetainFolderStructureOverride); | ||
PackageUploadHandle handle; | ||
if (IsPackageInstalled) | ||
{ | ||
handle = pmExtension.PackageManagerClient.PublishInstalledPackageAsync(Package, IsNewVersion); | ||
} | ||
else | ||
{ | ||
handle = pmExtension.PackageManagerClient.PublishAsync(Package, RetainFolderStructureOverride ? updatedFiles : contentFiles, MarkdownFiles, IsNewVersion, CurrentPackageRootDirectories, RetainFolderStructureOverride); | ||
} | ||
|
||
// start upload | ||
Uploading = true; | ||
|
@@ -2601,10 +2628,12 @@ | |
} | ||
|
||
/// <summary> | ||
/// Delegate used to publish the element locally </summary> | ||
/// Delegate used to publish the element locally | ||
/// If the package is already installed (and therefore loaded), we shouldn't be able to publish it locally again? | ||
/// </summary> | ||
private bool CanPublishLocally() | ||
{ | ||
return CheckPackageValidity(); | ||
return !IsPackageInstalled && CheckPackageValidity(); | ||
} | ||
|
||
private bool CheckPackageValidity() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does theDoesCurrentUserOwnPackage
check mean that the package cannot be republished by someone who hasn't published it before?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like only an existing package maintainer or package copyright holder can republish (publish a new version) the package? The code would be clearer if you said something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait..Copyright Holder should not be able to publish a version or a package, we do not validate copyright holder on server side, any actions related to package publishing either republishing from local copy or publishing a version should be limited to its current maintainers only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood.