incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pranav Saxena <pranav.sax...@citrix.com>
Subject RE: Review Request: CloudStack-965: When a detailview action is prohibited, the operation dialog box should not show up in the mean time
Date Fri, 18 Jan 2013 12:44:19 GMT
Isaac, 
I really appreciate for all your efforts . Your patch looks good just that the refactoring
of the code seemed to be a point of concern . I tested your patch and it seemed to be working
fine . Perhaps you could send an updated patch by removing the whitespace errors and I can
push it to asf/master. 

Thanks,
Pranav

-----Original Message-----
From: Isaac Chiang [mailto:noreply@reviews.apache.org] On Behalf Of Isaac Chiang
Sent: Thursday, January 17, 2013 7:05 PM
To: Pranav Saxena
Cc: cloudstack; Isaac Chiang
Subject: Re: Review Request: CloudStack-965: When a detailview action is prohibited, the operation
dialog box should not show up in the mean time



> On Jan. 17, 2013, 10:15 a.m., Pranav Saxena wrote:
> > Your code changes look good . However , I have few concerns - 
> > 
> > 1) The patch doesn't apply cleanly because of trailing whitespace errors . Hence
could you take care of these - 
> > 
> > home/pranav/Downloads/fix-cloudstack-965.patch:25: trailing whitespace.
> >         // Setup form validation			
> > /home/pranav/Downloads/fix-cloudstack-965.patch:27: trailing whitespace.
> >         $formContainer.find('input, select').each(function() {			  
> > /home/pranav/Downloads/fix-cloudstack-965.patch:31: space before tab in indent.
> > 			  	else {
> > /home/pranav/Downloads/fix-cloudstack-965.patch:32: space before tab in indent.
> > 			  	  $(this).rules('add', {});
> > /home/pranav/Downloads/fix-cloudstack-965.patch:33: space before tab in indent.
> > 			  	}
> > 
> > 2) Is there any specific reason for moving/reorganizing a section of code in dialog.js
from one point to other , though I know it won't affect anything. I believe , you could implement
the same without moving the chunks of code here and there , just to maintain the code consistency
and ordering. Please let me know if you have any concerns.
> > 
> > Thanks !
> >

Hi Pranav:
             The root cause of the issue, as I saw, is when the font-end is trying to make
a dialog form, it takes fields from from the caller (  detailview->createForm->fields
). Once the field block contains an ajax call, e.g., migrate in  instance.js : 867, It emits
it. But it still do the rest of the code in createForm and shows the dialog form. I figured
out few ways to solve the issue, including refactor entire workflow for creating the dialog
form, but those are more than I can do right now. The patch isn't good but merely a tradeoff
in my situation, just let you know. No matter what, hope the piece of code will help to identify
the issue. I'll close the review request if it achieves its purpose, thanks :)


- Isaac


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8992/#review15452
-----------------------------------------------------------


On Jan. 17, 2013, 8:09 a.m., Isaac Chiang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8992/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2013, 8:09 a.m.)
> 
> 
> Review request for cloudstack and Pranav Saxena.
> 
> 
> Description
> -------
> 
> The patch is to resolve CLOUDSTACK-965 issue which prevents from showing the error message
and dialog form in the mean time.
> 
> 
> This addresses bug CLOUDSTACK-965.
> 
> 
> Diffs
> -----
> 
>   ui/scripts/ui/dialog.js 5236bb6 
> 
> Diff: https://reviews.apache.org/r/8992/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Isaac Chiang
> 
>

Mime
View raw message