Re: [Kimchi-devel] [PATCH V4 4/5] Storagepool SCSI/FC: Modifies UI flow to select a LUN to new VM

On 02/10/2014 12:20 PM, Aline Manera wrote: > > When I select the text associated to the radio box it does not select > the option. Need to fix it This is working in my tests

> More comments below > > On 02/05/2014 12:18 PM, Rodrigo Trujillo wrote: >> This patch implements the UI functions and API calls to show to user >> the list of volumes (LUNs) of a SCSI FC storagepools. The user can then >> select the LUN when creating a new VM. >> >> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo(a)linux.vnet.ibm.com&gt; >> --- >> ui/js/src/kimchi.api.js | 13 +++++++ >> ui/js/src/kimchi.guest_add_main.js | 73 >> +++++++++++++++++++++++++++++++++++--- >> ui/pages/i18n.html.tmpl | 1 + >> 3 files changed, 83 insertions(+), 4 deletions(-) >> >> diff --git a/ui/js/src/kimchi.api.js b/ui/js/src/kimchi.api.js >> index 66fc41e..4597c5d 100644 >> --- a/ui/js/src/kimchi.api.js >> +++ b/ui/js/src/kimchi.api.js >> @@ -155,6 +155,19 @@ var kimchi = { >> }); >> }, >> >> + /* >> + * Retrieve the information of a storage pool by the given name. >> + */ >> + retrieveStoragePool : function(storagePoolName, suc, err) { >> + kimchi.requestJSON({ >> + url : kimchi.url + "storagepools/" + >> + encodeURIComponent(storagePoolName), >> + type : 'GET', >> + contentType : 'application/json', >> + dataType : 'json' >> + }).done(suc); >> + }, >> + >> /** >> * Retrieve the information of a template by the given name. >> */ >> diff --git a/ui/js/src/kimchi.guest_add_main.js >> b/ui/js/src/kimchi.guest_add_main.js >> index 2085562..6b8fc38 100644 >> --- a/ui/js/src/kimchi.guest_add_main.js >> +++ b/ui/js/src/kimchi.guest_add_main.js >> @@ -62,9 +62,7 @@ kimchi.guest_add_main = function() { >> } >> }); >> >> - var addGuest = function(event) { >> - var formData = $('#form-vm-add').serializeObject(); >> - >> + var addGuest = function(formData) { >> kimchi.createVM(formData, function() { >> kimchi.listVmsAuto(); >> kimchi.window.close(); >> @@ -79,8 +77,75 @@ kimchi.guest_add_main = function() { >> return false; >> }; >> >> + // This function is used to select a lun for new vm disk if >> template has >> + // a SCSI storagepool associated. >> + function getLun() { >> + var formData = $('#form-vm-add').serializeObject(); >> + var templateName = formData.template.substring(11); > >> + kimchi.retrieveTemplate(templateName, > >> function(templateInfo) { >> + var poolName = templateInfo.storagepool.substring(14); >> + kimchi.retrieveStoragePool(poolName, function(poolInfo){ >> + if (poolInfo.type === "scsi") { >> + kimchi.listStorageVolumes(poolInfo.name, >> function(lunsList) { >> + if (lunsList.length == 0) { >> + kimchi.message.error('There are not >> volumes for this pool'); > > You need to add the message to i18n.html.tmpl and use it here, > otherwise this message will not be translated. Done > >> + return false; >> + } >> + var popUpList = '<section >> class="form-section">' + >> + '<h2>1. Storage Pool: ' >> + poolInfo.name + '</h2>' + >> + '<div class="lun_radios">'; > > We need to improve the message to the user. > > "You select a Template associated to a SCSI Storage Pool so you need > to select which LUN you wan to use as primary disk to guest." > > Or something like it Done > >> + $.each(lunsList, function(index, value) { >> + popUpList += '<div class="field">' + >> + '<input type="radio" >> id="lun-' + value.name + '" name="lun" value="' + value.name + '">' + >> + '<label for="lun-' + >> value.name + '">' + value.name + '</label></div>'; >> + }); >> + popUpList += '</div></section>'; >> + console.log(popUpList) >> + var popup = $(popUpList); >> + popup.dialog({ >> + autoOpen : true, >> + modal : true, >> + width : 400, >> + draggable : false, >> + resizable : false, >> + closeText: "X", >> + dialogClass : "network-config", >> + title: "Please, select a LUN", >> + close: function( event, ui ) { >> $('input[name=lun]').attr('checked',false); }, >> + buttons : [ { >> + text : i18n.action_select_lun, >> + class: "ui-button-primary", >> + click : function() { >> + var lunName = >> $('input:radio[name=lun]:checked').val(); >> + if (lunName === undefined) { >> + kimchi.message.error('You must select a LUN'); >> + } else { >> + formData.volumes = new >> Array(lunName); >> + addGuest(formData); >> + } >> + $( this ).dialog( "close" ); >> + } >> + }] >> + }); > > What about split this big function into smaller ones? > It is hard to read it that way > > Example: > > 1) function to get template > 2) function to get storage associate to template > 3) function to get type of storage pool > > get template; > get storage pool > get storage pool type > > if pool type != scsi: > return > > 4) function to display new dialog I am not 100% sure how to do this. JS functions are still complicated for me, so it will take a while to code/test. As the code is working, I would rather ask some UI specialist to do this refactoring in next days. Or I can try to do in future, when I have less things in my backlog.

> >> + },function() { >> + // listStorageVolumes error handler >> + kimchi.message.error(i18n['msg.kimchi.list.volume.fail']); >> + }); >> + } >> + else { addGuest(formData); } >> + }, function() { >> + // retrieveStoragePool error handler >> + kimchi.message.error(i18n['msg.kimchi.retrieve.pool.fail']); >> + }); >> + }, function() { >> + // retrieveStoragePool error handler >> + kimchi.message.error(i18n['msg.fail.template.retrieve']); >> + }); >> + return false; >> + } >> + >> $('#form-vm-add').on('submit', addGuest); >> - $('#vm-doAdd').on('click', addGuest); >> + $('#vm-doAdd').on('click', getLun); >> >> showTemplates(); >> }; >> diff --git a/ui/pages/i18n.html.tmpl b/ui/pages/i18n.html.tmpl >> index a4c3ccb..f635fdf 100644 >> --- a/ui/pages/i18n.html.tmpl >> +++ b/ui/pages/i18n.html.tmpl >> @@ -123,6 +123,7 @@ var i18n = { >> 'network_dialog_ok': "$_("OK")", >> 'network_dialog_cancel': "$_("Cancel")", >> 'action_create': "$_("Create")", >> + 'action_select_lun': "$_("Select")", >> 'msg_warning': "$_("Warning")", >> 'msg.logicalpool.confirm.delete': "$_("It will format your >> disk and you will loose any data in" >> " there, are you sure to >> continue? ")", > > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel