[pve-devel] [[PATCH pve-manager] Button Delete Subscription key 1/1] [pve-manager] Add button to delete Subscription key. Delete Subcription key from "/etc/subscription"

[Thomas Lamprecht]

The subject looks broken, both in for mail and git purpose.
It got nested tags, and the rest is way to long - we try to stay under 70
characters per line for git commits, and the commit subject should be a
very short summary, the rest has to move in the commit messages body.

Your subject:
> [[PATCH pve-manager] Button Delete Subscription key 1/1] [pve-manager] Add button to delete Subscription key. Delete Subcription key from "/etc/subscription"

Better one:
> [PATCH pve-manager] api, ui: allow to remove subscription

Here "api, ui: allow to remove subscription" is the real commit message
subject and "[PATCH pve-manager]" is done by git send-email or format-patch
--subject-prefix "PATCH pve-manager"

See the "for patch formatting convenience" helper in:
http://intranet.proxmox.com/index.php/Useful_Tips#copy.26paste_shortcut_to_fetch_most_repositories

to set this as default in the configuration.

On 4/20/20 2:00 PM, Moayad Almalat wrote:
> Signed-off-by: Moayad Almalat <m.almalat at proxmox.com>
> ---
>  PVE/API2/Subscription.pm          | 22 ++++++++++++++++++++++
>  PVE/CLI/pvesubscription.pm        |  1 +
>  www/manager6/node/Subscription.js |  8 ++++++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/PVE/API2/Subscription.pm b/PVE/API2/Subscription.pm
> index 6657c00d..46895997 100644
> --- a/PVE/API2/Subscription.pm
> +++ b/PVE/API2/Subscription.pm
> @@ -245,4 +245,26 @@ __PACKAGE__->register_method ({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'delete',
> +    path => '',
> +    method => 'DELETE',
> +    permissions => {
> +        check => ['perm', '/nodes/{node}', [ 'Sys.Modify' ]],
> +    },
> +    description => "Delete subscription key.",
> +    proxyto => 'node',
> +    protected => 1,
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => {
> +            node => get_standard_option('pve-node'),
> +
> +        },
> +    },
> +    code => sub {
> +        unlink ("/etc/subscription");

unlink can fail, which needs to be checked. Also, please no space between unlink
and the parenthesis.

unlink("/etc/subscription") or die "could not delete subscription key: $!";

Above uses the $! perl variable. This is for C library functions and would be
the ernno (and it's derived string from that error number).

Note that unlink does not fails if the file is not present before trying to
unlink, that behavior is OK here.

> +        return undef;
> +    }});
> +
>  1;
> diff --git a/PVE/CLI/pvesubscription.pm b/PVE/CLI/pvesubscription.pm
> index cd81c415..751dde58 100755
> --- a/PVE/CLI/pvesubscription.pm
> +++ b/PVE/CLI/pvesubscription.pm
> @@ -28,6 +28,7 @@ our $cmddef = {
>  		 }
>  	     }],
>      set => [ 'PVE::API2::Subscription', 'set', ['key'], { node => $nodename } ],
> +    delete => [ 'PVE::API2::Subscription', 'delete', undef, { node => $nodename } ],
>  };
>  
>  1;
> diff --git a/www/manager6/node/Subscription.js b/www/manager6/node/Subscription.js
> index e4a35874..7f93cd80 100644
> --- a/www/manager6/node/Subscription.js
> +++ b/www/manager6/node/Subscription.js
> @@ -163,6 +163,14 @@ Ext.define('PVE.node.Subscription', {
>  			win.on('destroy', reload);
>  		    }
>  		},
> +                {
> +                    text: gettext('Delete Subscription Key'),

This is a to long text, buttons should have rather shorter ones, e.g.:
gettext('Remove Subscription')

> +                    xtype: 'proxmoxStdRemoveButton',
> +                    confirmMsg: 'Are you sure to delete Subscription Key?',

misses the gettext, as is it cannot be translated.. Also grammar error, misses the
article for subscription key, maybe use:

gettext('Are you sure to remove the subscription key?')

> +                    getUrl: () => '/api2/extjs' + baseurl,

This misses the "selModel: false" and you can use baseurl now instead of
overwriting  getUrl.

I do not want the behavior that I have to select a bogus row of the subscription
view first for the delete button to get disabled, as said offlist ;-)


> +                    dangerous: true,
> +                    callback: reload,> +                },
>  		{
>  		    text: gettext('Check'),
>  		    handler: function() {
>