Re: [pbs-devel] [PATCH proxmox-backup v6 6/6] ui: add option to change the maintenance type

[Dominik Csapak <d.csapak@proxmox.com>]

comment inline:

On 2/2/22 16:49, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   www/Makefile                     |  1 +
>   www/Utils.js                     | 23 ++++++++++
>   www/datastore/OptionView.js      | 30 +++++++++++++
>   www/window/MaintenanceOptions.js | 72 ++++++++++++++++++++++++++++++++
>   4 files changed, 126 insertions(+)
>   create mode 100644 www/window/MaintenanceOptions.js
> 
> diff --git a/www/Makefile b/www/Makefile
> index 455fbeec..0952fb82 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -61,6 +61,7 @@ JSSRC=							\
>   	window/BackupGroupChangeOwner.js		\
>   	window/CreateDirectory.js			\
>   	window/DataStoreEdit.js				\
> +	window/MaintenanceOptions.js			\
>   	window/NotesEdit.js				\
>   	window/RemoteEdit.js				\
>   	window/TrafficControlEdit.js			\
> diff --git a/www/Utils.js b/www/Utils.js
> index 36a94211..db23645a 100644
> --- a/www/Utils.js
> +++ b/www/Utils.js
> @@ -640,4 +640,27 @@ Ext.define('PBS.Utils', {
>   	return `${icon} ${value}`;
>       },
>   
> +    renderMaintenance: function(type, activeTasks) {
> +	if (!type) return gettext('None');
> +	let at = 0;
> +	for (let x of ['read-only-', 'offline-']) {
> +	    if (type.startsWith(x)) {
> +	        at = x.length;
> +	    }
> +	}
> +
> +	const conflictingTasks = activeTasks.write + (type.startsWith('offline') ? activeTasks.read : 0);
> +	const checkmarkIcon = '<i class="fa fa-check"></i>';
> +	const spinnerIcon = '<i class="fa fa-spinner fa-pulse fa-fw"></i>';
> +	const conflictingTasksMessage = `<i>${conflictingTasks} conflicting tasks still active</i>`;
> +	const extra = conflictingTasks > 0 ? `| ${spinnerIcon} ${conflictingTasksMessage}` : checkmarkIcon;
> +
> +	const mode = type.substring(0, at-1);
> +	let msg = type.substring(at);
> +	if (msg.length > 0) {
> +	    msg = `"${msg}"`;
> +	}
> +	return Ext.String.capitalize(`${gettext(mode)} ${msg} ${extra}`) || gettext('None');

two small issues here:
i don't think using 'capitalize' is good here because
1. may not work in all translations like you intended
2. the gettext should already be capitalized

second issue is that you use a variable in gettext. while it'll "work"
the actual values you pass will never be picked up by our script in
proxmox-i18n.

instead something like this would work better (untested):

---
let modeText = Proxmox.Utils.unknownText;
switch (mode) {
     case 'read-only:
         modeText = gettext("Read-only");
         break;
     case 'offline':
         modeText = gettext("Offline");
         break;
}
---

with that, the gettexts are properly picked up and you don't need to use 'capitalize'


> +    },
> +
>   });
> 

[..snip..]