[pve-devel] [PATCH manager] fix #4531: Fix ACME plugin edit form not detecting changes

[pve-devel] [PATCH manager] fix #4531: Fix ACME plugin edit form not detecting changes

[Filip Schauer]

Fix the ACME plugin edit form not detecting dirtychanges to the values
of textfields properly.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/window/ACMEPluginEdit.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/window/ACMEPluginEdit.js b/src/window/ACMEPluginEdit.js
index 0f21527..f6d1bcd 100644
--- a/src/window/ACMEPluginEdit.js
+++ b/src/window/ACMEPluginEdit.js
@@ -126,7 +126,7 @@ Ext.define('Proxmox.window.ACMEPluginEdit', {
 		for (const [key, value] of Object.entries(data)) {
 		    if (me.createdFields[key]) {
 			me.createdFields[key].setValue(value);
-			me.createdFields[key].originalValue = me.originalValues[key];
+			me.createdFields[key].resetOriginalValue();
 		    } else {
 			extradata.push(`${key}=${value}`);
 		    }
-- 
2.39.2

Re: [pve-devel] [PATCH manager] fix #4531: Fix ACME plugin edit form not detecting changes

[Thomas Lamprecht]

meta: your repo tag is wrong, this is for widget-toolkit, not manager.

off topic, while checking that I saw that we still have a (FWICT unused)
copy of the old pveACMEPluginEditor module checked in in pve-manager, we
should probably clean that up (no need for patches, this is more efficiently
deleted directly in git, but some confirmation that this is unused, and if
there are others that might be dangling around too, would be good).


Am 29/08/2023 um 13:16 schrieb Filip Schauer:
> Fix the ACME plugin edit form not detecting dirtychanges to the values
> of textfields properly.

a bit more information about would be great here, like why this solution
was choosen over adding the missing call directly, like:

me.createdFields[key].checkDirty();

As the previous code and resetOriginalValue() minus the checkDirty() call
aren't exactly the same.

> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/window/ACMEPluginEdit.js | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/window/ACMEPluginEdit.js b/src/window/ACMEPluginEdit.js
> index 0f21527..f6d1bcd 100644
> --- a/src/window/ACMEPluginEdit.js
> +++ b/src/window/ACMEPluginEdit.js
> @@ -126,7 +126,7 @@ Ext.define('Proxmox.window.ACMEPluginEdit', {
>  		for (const [key, value] of Object.entries(data)) {
>  		    if (me.createdFields[key]) {
>  			me.createdFields[key].setValue(value);
> -			me.createdFields[key].originalValue = me.originalValues[key];

me.originalValues is now unused, but still set – this seems off; I find it a
bit weird that Dominik added this handling and explicitly wrote
"we have to properly track the original values" in the original commit
45708891 ("ui: add ACMEPluginEdit window") from pve-manager repo..

Your code might be even all right, I did not reviewed this in depth, but if
it would, then it should also remove the "originalValues" field with a good
reason in the commit message why that was wrong, or why it's now unnecessary.


> +			me.createdFields[key].resetOriginalValue();
>  		    } else {
>  			extradata.push(`${key}=${value}`);
>  		    }

Re: [pve-devel] [PATCH manager] fix #4531: Fix ACME plugin edit form not detecting changes

[Dominik Csapak]

On 8/30/23 11:07, Thomas Lamprecht wrote:
> meta: your repo tag is wrong, this is for widget-toolkit, not manager.
> 
> off topic, while checking that I saw that we still have a (FWICT unused)
> copy of the old pveACMEPluginEditor module checked in in pve-manager, we
> should probably clean that up (no need for patches, this is more efficiently
> deleted directly in git, but some confirmation that this is unused, and if
> there are others that might be dangling around too, would be good).

we actually use that still, but with a few tweaks to the pve code
it would be redundant (iow. we have to replace the window
in dc/ACMEClusterView.js)

> 
> 
> Am 29/08/2023 um 13:16 schrieb Filip Schauer:
>> Fix the ACME plugin edit form not detecting dirtychanges to the values
>> of textfields properly.
> 
> a bit more information about would be great here, like why this solution
> was choosen over adding the missing call directly, like:
> 
> me.createdFields[key].checkDirty();
> 
> As the previous code and resetOriginalValue() minus the checkDirty() call
> aren't exactly the same.
> 
>>
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
>> ---
>>   src/window/ACMEPluginEdit.js | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/window/ACMEPluginEdit.js b/src/window/ACMEPluginEdit.js
>> index 0f21527..f6d1bcd 100644
>> --- a/src/window/ACMEPluginEdit.js
>> +++ b/src/window/ACMEPluginEdit.js
>> @@ -126,7 +126,7 @@ Ext.define('Proxmox.window.ACMEPluginEdit', {
>>   		for (const [key, value] of Object.entries(data)) {
>>   		    if (me.createdFields[key]) {
>>   			me.createdFields[key].setValue(value);
>> -			me.createdFields[key].originalValue = me.originalValues[key];
> 
> me.originalValues is now unused, but still set – this seems off; I find it a
> bit weird that Dominik added this handling and explicitly wrote
> "we have to properly track the original values" in the original commit
> 45708891 ("ui: add ACMEPluginEdit window") from pve-manager repo..
> 
> Your code might be even all right, I did not reviewed this in depth, but if
> it would, then it should also remove the "originalValues" field with a good
> reason in the commit message why that was wrong, or why it's now unnecessary.

i was the one to recommend that, and it worked, in some situations.
after looking at the code i now remember why i did not do that.


if you edit a value when selecting one plugin, then selecting a different
one with e.g. no schema, and then switching back, the new (dirty) value
is registered as the original one...

so to fix that we have to something like doing the check manually
( like Thomas mentioned above )

> 
> 
>> +			me.createdFields[key].resetOriginalValue();
>>   		    } else {
>>   			extradata.push(`${key}=${value}`);
>>   		    }
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel