[Kimchi-devel] [PATCH] Bugfix:#424 Edit Template, "Disk (GB)" is changing with storage pool

[Aline Manera]

On 09/19/2014 06:43 AM, Wen Wang wrote:
>
> On 09/19/2014 10:43 AM, Aline Manera wrote:
>>
>> On 09/18/2014 04:33 AM, Wen Wang wrote:
>>> From: Wen Wang <wenwang at linux.vnet.ibm.com>
>>>
>>> This patch fix the bug that value of "Disk(GB)" input box is changing
>>> with the item of "Storage Pool" in "Templates" --> "Actions" --> "Edit"
>>> -->"Edit Templates" dialogue. This might be comfusing to user when
>>> changing the "Storage Pool", "Disk(GB)" is changed with it without even
>>> notice. This might confuse the users and is not necessary. Solved by
>>> removing the change.
>>>
>>> Signed-off-by: Wen Wang <wenwang at linux.vnet.ibm.com>
>>> ---
>>>   ui/js/src/kimchi.template_edit_main.js |   25 
>>> -------------------------
>>>   1 files changed, 0 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/ui/js/src/kimchi.template_edit_main.js 
>>> b/ui/js/src/kimchi.template_edit_main.js
>>> index cb43091..61d23b4 100644
>>> --- a/ui/js/src/kimchi.template_edit_main.js
>>> +++ b/ui/js/src/kimchi.template_edit_main.js
>>> @@ -117,31 +117,6 @@ kimchi.template_edit_main = function() {
>>>           });
>>>       });
>>
>>
>>> - $('#template-edit-storagePool').change(function() {
>>> -        storagepool = $(this).val();
>>> -        var storageArray = storagepool.split("/");
>>> -        if (storageArray.length > 3) {
>>> -            volumeName = storageArray.pop();
>>> -            poolName = storageArray.pop();
>>> -            kimchi.getStoragePoolVolume(poolName, volumeName, 
>>> function(result) {
>>> -                $('input[name="disks"]', 
>>> templateEditForm).val(result.capacity / Math.pow(1024,3));
>>> -                $('input[name="disks"]', 
>>> templateEditForm).attr('disabled','disabled');
>>> -                return false;
>>> -            }, function (err) {
>>> - kimchi.message.error(err.responseJSON.reason);
>>> -            });
>>> -        } else {
>>> -            if (origPool == storagepool) {
>>> -                // Previous disk size value
>>> -                $('input[name="disks"]', 
>>> templateEditForm).val(origDisks[0].size);
>>> -            } else {
>>> -                // Default disk size value
>>> -                $('input[name="disks"]', templateEditForm).val(10);
>>> -            }
>>> -            $('input[name="disks"]', 
>>> templateEditForm).removeAttr('disabled');
>>> -        }
>>> -    });
>>> -
>>>       $('#tmpl-edit-button-save').on('click', function() {
>>>           var editableFields = [ 'name', 'cpus', 'memory', 
>>> 'storagepool', 'disks', 'graphics'];
>>>           var data = {};
>>
>> We can't remove this code.
>> It is used when a iSCSI or SCSI storage pool is selected. In this 
>> case, the disk size comes from the volumes selected and can not be 
>> changed.
>>
>> iSCSI and SCSI pools are what we said as read-only pools which means 
>> we can't create new volumes in it as the volumes are pre-configured 
>> by system.
>> So when user selects one of these pools, he/she needs also to point 
>> the volume because that we display <pool>/<volumes> in these cases.
>>
>> What we can do is always display the previous disk size when changing 
>> from pools that are not iSCSI or SCSI.
>> To do that we need to change the "else" statement like below:
>>
>>
>>          } else {
>> -            if (origPool == storagepool) {
>> -                // Previous disk size value
>> -                $('input[name="disks"]', 
>> templateEditForm).val(origDisks[0].size);
>> -            } else {
>> -                // Default disk size value
>> -                $('input[name="disks"]', templateEditForm).val(10);
>> -            }
>
>> +            // Previous disk size value
>> +            $('input[name="disks"]', 
>> templateEditForm).val(origDisks[0].size);
>
> I don't think these two line is necessary for this. I mean if we 
> detect no iSCSI nor SCSI device, The only thing we need to do is to 
> enable the input box. Having it changed without any notificaton is not 
> that good user experience. This input box has value when it initiated 
> and if changing to iSCSI or SCSI device, we just need to enable the 
> input box, if changing back, we disable the input box again and 
> refresh the value.

If we don't update the disk size value when a no iSCSI nor SCSI device 
is selected, the input box will contain the previous value which can be 
not valid.
Example.

1. Select a dir pool
2. Disk size is set to 10Gb
3. Select a iSCSI volume
4. Disk size is set to 4GB
5. Select back the dir pool

6.a Disk size is still set to 4GB (*without* those 2 lines)
6.b Disk size turns back to 4GB (*with* the 2 lines)

 From my view, the correct behavior is 6.b

>
>>              $('input[name="disks"]', 
>> templateEditForm).removeAttr('disabled');
>>          }
>>      });
>>
>>
>>
>>
>