[pve-devel] [PATCH proxmox-widget-toolkit 1/3] fix #5588: notification: smtp: fix 'Authenticate' checkbox in Chromium

[pve-devel] [PATCH proxmox-widget-toolkit 1/3] fix #5588: notification: smtp: fix 'Authenticate' checkbox in Chromium

[Lukas Wagner]

When editing an SMTP endpoint that has no authentication settings, in
Chromium-based browsers, the 'Authenticate' checkbox was ticked because
of a race condition between the view-model default and the InputPanel
logic setting the existing values for the endpoint. Explicitly setting
the value in the view model in onSetValues seems to fix this reliably.

Tested on:
  - Firefox 140
  - Chromium 137
  - Chrome 138
  - Gnome Web (Webkit-based) 48

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/panel/SmtpEditPanel.js | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/panel/SmtpEditPanel.js b/src/panel/SmtpEditPanel.js
index df6e3b7..fd10b54 100644
--- a/src/panel/SmtpEditPanel.js
+++ b/src/panel/SmtpEditPanel.js
@@ -196,10 +196,17 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
     },
 
     onSetValues: function (values) {
+        let me = this;
+
         values.authentication = !!values.username;
         values.enable = !values.disable;
         delete values.disable;
 
+        // Fix race condition in chromium-based browsers. Without this, the
+        // 'Authenticate' remains ticked (the default value) if loading an
+        // SMTP target without authentication.
+        me.getViewModel().set('authentication', values.authentication);
+
         return values;
     },
 });
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-widget-toolkit 2/3] notification: smtp: only show 'Unchanged' in password field when it makes sense

[Lukas Wagner]

The formula used for deriving the emptyText for the password field did
not correctly pick up the 'isCreate' flag, so 'Unchanged' was also
visible when creating a new SMTP target that has no password yet.

Also took the opportunity to improve the behavior when modifying an
existing target. Now the original state of the 'authentication' flag is
saved. The 'Unchanged' emptyText is only shown if the loaded config
contains authentication parameters. When editing a target *without*
authentication, the password field does not have the 'Unchanged'
emptyText any more.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/panel/SmtpEditPanel.js | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/panel/SmtpEditPanel.js b/src/panel/SmtpEditPanel.js
index fd10b54..a7cfaff 100644
--- a/src/panel/SmtpEditPanel.js
+++ b/src/panel/SmtpEditPanel.js
@@ -8,12 +8,10 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
 
     viewModel: {
         xtype: 'viewmodel',
-        cbind: {
-            isCreate: '{isCreate}',
-        },
         data: {
             mode: 'tls',
             authentication: true,
+            originalAuthentication: true,
         },
         formulas: {
             portEmptyText: function (get) {
@@ -33,8 +31,13 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
                 return `${Proxmox.Utils.defaultText} (${port})`;
             },
             passwordEmptyText: function (get) {
-                let isCreate = this.isCreate;
-                return get('authentication') && !isCreate ? gettext('Unchanged') : '';
+                let isCreate = this.getView().isCreate;
+
+                let auth = get('authentication');
+                let origAuth = get('originalAuthentication');
+                let shouldShowUnchanged = !isCreate && auth && origAuth;
+
+                return shouldShowUnchanged ? gettext('Unchanged') : '';
             },
         },
     },
@@ -123,7 +126,9 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
             inputType: 'password',
             fieldLabel: gettext('Password'),
             name: 'password',
-            allowBlank: true,
+            cbind: {
+                allowBlank: '{!isCreate}',
+            },
             bind: {
                 disabled: '{!authentication}',
                 emptyText: '{passwordEmptyText}',
@@ -206,6 +211,7 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
         // 'Authenticate' remains ticked (the default value) if loading an
         // SMTP target without authentication.
         me.getViewModel().set('authentication', values.authentication);
+        me.getViewModel().set('originalAuthentication', values.authentication);
 
         return values;
     },
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-widget-toolkit 3/3] notification: smtp: don't split translatable string

[Lukas Wagner]

Splitting a string can make it harder or even impossible to provide an
accurate translation.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/panel/SmtpEditPanel.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/panel/SmtpEditPanel.js b/src/panel/SmtpEditPanel.js
index a7cfaff..37e4d51 100644
--- a/src/panel/SmtpEditPanel.js
+++ b/src/panel/SmtpEditPanel.js
@@ -76,7 +76,7 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
             fieldLabel: gettext('Encryption'),
             editable: false,
             comboItems: [
-                ['insecure', Proxmox.Utils.noneText + ' (' + gettext('insecure') + ')'],
+                ['insecure', gettext('None (insecure)')],
                 ['starttls', 'STARTTLS'],
                 ['tls', 'TLS'],
             ],
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-widget-toolkit 3/3] notification: smtp: don't split translatable string

[Maximiliano Sandoval]

Lukas Wagner <l.wagner@proxmox.com> writes:

> Splitting a string can make it harder or even impossible to provide an
> accurate translation.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/panel/SmtpEditPanel.js | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/panel/SmtpEditPanel.js b/src/panel/SmtpEditPanel.js
> index a7cfaff..37e4d51 100644
> --- a/src/panel/SmtpEditPanel.js
> +++ b/src/panel/SmtpEditPanel.js
> @@ -76,7 +76,7 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
>              fieldLabel: gettext('Encryption'),
>              editable: false,
>              comboItems: [
> -                ['insecure', Proxmox.Utils.noneText + ' (' + gettext('insecure') + ')'],
> +                ['insecure', gettext('None (insecure)')],
>                  ['starttls', 'STARTTLS'],
>                  ['tls', 'TLS'],
>              ],


lgtm.

Reviewed-by: Maximiliano Sandoval <m.sandoval@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-widget-toolkit 1/3] fix #5588: notification: smtp: fix 'Authenticate' checkbox in Chromium

[Michael Köppl]

Tested this on the latest master with both Chromium 137.0.7151.68 and
Firefox 139.0.1. Was able to reproduce the original issue #5588 on PBS
in Chromium before applying the patches.

Ensured that:
- the "Authenticate" checkbox stays unchecked in Chromium
- "Unchanged" is only shown as the empty text *with* authentication
activated
- the password field is actually empty upon creation of a new SMTP
target, instead of showing "Unchanged"

Did not notice anything off. The implementation looks good to me. I
spent a few minutes tinkering around with an alternative approach of
detecting whether the user had previously entered a password, but the
approach in 2/3 is works as expected and doesn't add unnecessary complexity.

Consider this:
Tested-by: Michael Köppl <m.koeppl@proxmox.com>
Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>

On 6/27/25 14:13, Lukas Wagner wrote:
> When editing an SMTP endpoint that has no authentication settings, in
> Chromium-based browsers, the 'Authenticate' checkbox was ticked because
> of a race condition between the view-model default and the InputPanel
> logic setting the existing values for the endpoint. Explicitly setting
> the value in the view model in onSetValues seems to fix this reliably.
> 
> Tested on:
>   - Firefox 140
>   - Chromium 137
>   - Chrome 138
>   - Gnome Web (Webkit-based) 48
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/panel/SmtpEditPanel.js | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/panel/SmtpEditPanel.js b/src/panel/SmtpEditPanel.js
> index df6e3b7..fd10b54 100644
> --- a/src/panel/SmtpEditPanel.js
> +++ b/src/panel/SmtpEditPanel.js
> @@ -196,10 +196,17 @@ Ext.define('Proxmox.panel.SmtpEditPanel', {
>      },
>  
>      onSetValues: function (values) {
> +        let me = this;
> +
>          values.authentication = !!values.username;
>          values.enable = !values.disable;
>          delete values.disable;
>  
> +        // Fix race condition in chromium-based browsers. Without this, the
> +        // 'Authenticate' remains ticked (the default value) if loading an
> +        // SMTP target without authentication.
> +        me.getViewModel().set('authentication', values.authentication);
> +
>          return values;
>      },
>  });



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH proxmox-widget-toolkit 1/3] fix #5588: notification: smtp: fix 'Authenticate' checkbox in Chromium

[Thomas Lamprecht]

On Fri, 27 Jun 2025 14:13:37 +0200, Lukas Wagner wrote:
> When editing an SMTP endpoint that has no authentication settings, in
> Chromium-based browsers, the 'Authenticate' checkbox was ticked because
> of a race condition between the view-model default and the InputPanel
> logic setting the existing values for the endpoint. Explicitly setting
> the value in the view model in onSetValues seems to fix this reliably.
> 
> Tested on:
>   - Firefox 140
>   - Chromium 137
>   - Chrome 138
>   - Gnome Web (Webkit-based) 48
> 
> [...]

Applied, thanks!

btw. adding a cover-letter to any series with a few patches might be nice in
any case, as that allows replying with R-b/T-b on that and make it clarify
that they should be applied to the whole series to both devs and b4, the
cover-letter itself can be very basic.
Here I applied Michael's trailer to all patches and Maximiliano just to the
one he replied too, as it would be fiting for him to review translation
stuff, so I figured it's really just for that part.

[1/3] fix #5588: notification: smtp: fix 'Authenticate' checkbox in Chromium
      commit: ca138ff8d0dc8687ddfe9e4e448342e1fe1fb8a0
[2/3] notification: smtp: only show 'Unchanged' in password field when it makes sense
      commit: f47eec10122e5e8a25f5d76411cbef9d18ebc022
[3/3] notification: smtp: don't split translatable string
      commit: dc5d616d72e41b805155165aeb1c81c7fe7d058c


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel