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

[Wen Wang]

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.
>
>>
>>>              $('input[name="disks"]', 
>>> templateEditForm).removeAttr('disabled');
>>>          }
>>>      });
>>>
>>>
>>>
>>>
>>
>

-- 

=======================================================
Best Regards
Wang Wen