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