----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3078/#review3782 -----------------------------------------------------------
Found a few small items that I think we need to decide how to handle as a group -- but other than that looks good to me. https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/resources/messages.properties <https://reviews.apache.org/r/3078/#comment8555> We need to be sure to open a JIRA ticket to add these to the other messages files -- or maybe we should just try Google Translate for a first pass and enter them ourselves. Anyone have a preference? My vote is to try Google Translate as a first pass. https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/admin/widgetdetail.jsp <https://reviews.apache.org/r/3078/#comment8553> I think we need to be careful about scripting attacks here -- if administrators are the only ones who can ever disable a widget and set the message its probably fine, but if end users can ever do this we need to watch out for the potential for them to add malicious script to the message. I'm not sure if this is an issue currently or not (I'm hoping others more familiar with this part of the codebase can say one way or another), but if it is I wouldn't hold up the patch to fix it -- I'd just make sure a JIRA ticket is created to address it. https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/script/rave.js <https://reviews.apache.org/r/3078/#comment8554> I added a comment about the potential for malicious script earlier -- thinking more about it now maybe we should just say no markup is allowed in the message (since it shouldn't really be needed) and ensure that we add the content safely here. I think just using .text(...) instead of .html(...) might do it. Can anyone else confirm? https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/script/rave.js <https://reviews.apache.org/r/3078/#comment8552> If we append something to the title then I think we need to internationalize it. I think I'd prefer to leave the title alone though and just rely on the message that the administrator can set to convey to the user that the widget is disabled. So I guess I'd either pull this out or find some way to internationalize the appended DISABLED string. My vote is to pull it out. - Jesse On 2011-12-08 21:10:12, Venkat Mahadevan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3078/ > ----------------------------------------------------------- > > (Updated 2011-12-08 21:10:12) > > > Review request for rave. > > > Summary > ------- > > RAVE-210 Administrators should be able to disable widgets > > To disable a gadget, login as Admin and go to "Admin Interface" and to the > Widgets tab and click on a widget to get its metadata. Here you will find two > new columns, "Disable Gadget" and "Disable Gadget Message". One can disable > the gadget from rendering if you check he disable gadget checkbox and > whatever you put in the message will be displayed on the gadget when the > gadget is rendered. If you want to enable it, uncheck the checkbox. The > gadget will be displayed as usual (even if the disable message is still there > in the metadata, since the checkbox overrides it). > > > Diffs > ----- > > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/admin/widgetdetail.jsp > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/resources/messages.properties > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/tag/RegionWidgetTag.java > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/Widget.java > 1211932 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/tag/AbstractContextAwareSingletonBeanDependentTag.java > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/script/rave.js > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/test/javascript/raveSpec.js > 1211897 > > Diff: https://reviews.apache.org/r/3078/diff > > > Testing > ------- > > > Thanks, > > Venkat > >
