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

[Aline Manera]

On 09/23/2014 04:23 AM, Wen Wang wrote:
>
> On 09/19/2014 09:20 PM, Aline Manera wrote:
>>
>> 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
> I have tried your proposal, And there occurs one more problem just 
> according to this change. Think about the scenario below:
> Current template is
>
> "Storage Pool: default
>   Disk(GB): 15"
>
> And user change the "Disk(GB)" value to 30( without saving)
> then user choose the "Storage Pool" and decided to use "ISO", by 
> choosing that, the value of "Disk(GB)" changed to 15 which is the 
> value set for "default" and that value makes no sense to "ISO" so as 
> the change of this value.
>
> So I recommend we just leave the user with the value that they edited 
> before and it will have a default value either from the server stored 
> or set by iSCSI & SCSI automatic settings.

Good point. So we should also save the inputed value

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