On 10/20/2010 07:33 AM, Jan Provaznik wrote: > On 10/19/2010 10:23 PM, Mohammed Morsi wrote: >> Currently when a user adds a package to a new template, a temporary >> template gets created for use until the form is submitted. If the >> user >> cancels the form, the template still exists when it should not. This >> patch simply changes the create template logic to only create the >> template when the form is submitted >> >> This patch now works both when javascript is enabled and disabled. >> >
OK thank you for reviewing / acking this. I tested and integrated the patch you attached, rebased mine to apply against the current aggregator HEAD and pushed. A couple comments below. > > Mostly ACK, this patch looks good and makes code cleaner. There are > still some bugs in this code: > - template id in controller actions is not checked properly and w/o JS > throws exceptions on some places (see attachment) Thanks for this, I verified it works and included it > - group selection doesn't work Ah yes you are correct, though only when js is disabled. I added a small addition to the patch fixing this. > - "show all" link doesn't work I'm assuming this is the "show unsorted" link at the bottom of the package selection page. In this case you are right, I had mistakingly removed the js which was wired up to this link and didn't update the link itself to work in the non-js case. In any case, it should be working now (pushed a small follow up patch fixing this). Good catch. > - this patch expects that all selected packages are submited for each > "add select" action (user can't add packages incrementally), it's not > problem now, but will be problem with new selection model. This one I'm not so sure about. It seems to work locally in both the JS and non-js cases. I can incrementally add and remove packages successfully (even many times in a single form submission). How exactly can I reproduce this issue? Again thanks for the review / ACK. -Mo _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
