-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hey Tomas, some comments on this commit:
Checkstyle is currently failing, you can see the results by running "ant checkstyle" in java/ directory: checkstyle: [mkdir] Created dir: /home/dgoodwin/src/spacewalk/java/build/reports [checkstyle] log4j:WARN No appenders could be found for logger (org.apache.commons.beanutils.BeanUtils). [checkstyle] log4j:WARN Please initialize the log4j system properly. [checkstyle] Running Checkstyle 4.1 on 1802 files [checkstyle] /home/dgoodwin/src/spacewalk/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java:1273:1: Line contains a tab character. [checkstyle] /home/dgoodwin/src/spacewalk/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java:1274:1: Line contains a tab character. [checkstyle] /home/dgoodwin/src/spacewalk/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java:1277:1: Line contains a tab character. [checkstyle] /home/dgoodwin/src/spacewalk/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java:1278:1: Line contains a tab character. [checkstyle] /home/dgoodwin/src/spacewalk/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java:1279:1: Line contains a tab character. [checkstyle] /home/dgoodwin/src/spacewalk/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java:1282:17: '}' should be alone on a line. Obviously this is relatively minor but the check is run before our Hudson build and the automated tests won't run if the checkstyle fails. (Hudson's running internally so I can send you the link on IRC) As for the commit itself, this piece could be problematic: if (next.getClass() == StateChangeData.class) { As it adds a dependency on a Monitoring class to the ListDisplayTag, which is a generic component and technically shouldn't need to know anything about monitoring classes. Class comparison in general is usually a sign that there is a better way. Looking at the issue you're working on solving, there might be a cleaner way to accomplish this if we go straight to the action(s) that query and provide this StateChangeData. Am I correct in assuming this is for ProbeDetailsAction.java around line 163, which is used by java/code/webapp/WEB-INF/pages/systems/probes/details.jsp? Looking over this, could you modify StateChangeData to store a copy of the htmlifiedData when data is set, and add getHtmlifiedMessage and getHtmlifiedState, then modify details.jsp to display these two properties instead. This should leave the original data intact and the CSV should continue to generate properly. Attaching a *very* quick and completely untested patch to demonstrate what I mean. This should keep the monitoring specific logic tucked away in monitoring specific code. Cheers, Devan On Thu, Jun 11, 2009 at 7:00 AM, Tomas Lestach<tlest...@fedoraproject.org> wrote: > java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java > | 13 ++++++++ > java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java > | 15 +++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) > > New commits: > commit ddb349526220dd58132b0108792eb0afbd8e853f > Author: Tomas Lestach <tlest...@redhat.com> > Date: Thu Jun 11 11:52:17 2009 +0200 > > 498650 - html escape of monitoring data before displaying on the > WEBUI > > only a copy of monitoring data will be escaped > we do not want to modify the real data, > because they're used later on (f.e. for CSV generation) > > diff --git > a/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java > b/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java > index 9d78f8b..a5bddc0 100644 --- > a/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java > +++ > b/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java > @@ -15,6 +15,7 @@ package com.redhat.rhn.frontend.dto.monitoring; > > import com.redhat.rhn.common.localization.LocalizationService; > +import com.redhat.rhn.common.util.StringUtil; > import com.redhat.rhn.frontend.dto.BaseDto; > > import java.sql.Timestamp; > @@ -28,7 +29,17 @@ public class StateChangeData extends BaseDto { > private String oId; > private String data; > private Long entryTime; > - > + > + /** copies StateChangeData object and html escapes its data > + * @param scd StateChangeData object to be copied > + */ > + public void createHtmlEscapedCopy(StateChangeData scd) { > + oId = scd.oId; > + entryTime = scd.entryTime; > + data = StringUtil.htmlifyText(scd.data); > + setSelected(scd.isSelected()); > + } > + > /** > * @return Returns the oId. > */ > diff --git > a/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java > b/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java > index 5a6d03e..ff3a2a3 100644 --- > a/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java > +++ > b/java/code/src/com/redhat/rhn/frontend/taglibs/ListDisplayTag.java > @@ -44,6 +44,7 @@ import > com.redhat.rhn.common.util.ServletExportHandler; import > com.redhat.rhn.common.util.StringUtil; import > com.redhat.rhn.domain.rhnset.RhnSet; import > com.redhat.rhn.frontend.dto.UserOverview; +import > com.redhat.rhn.frontend.dto.monitoring.StateChangeData; import > com.redhat.rhn.frontend.html.HtmlTag; import > com.redhat.rhn.frontend.listview.AlphaBar; import > com.redhat.rhn.frontend.listview.PaginationUtil; @@ -1268,7 +1269,19 > @@ public class ListDisplayTag extends BodyTagSupport { columnCount = > 0; Object next = iterator.next(); out.println(getTrElement(next)); > - pageContext.setAttribute("current", next); > + > + /* escape monitoring data (can include html) > + * just before displaying */ > + if (next.getClass() == StateChangeData.class) { > + StateChangeData scd = new StateChangeData(); > + /* we do not want to have real data escaped > + * (further used f.e. for CSV generation) > + * that's why we escape copy of data only */ > + scd.createHtmlEscapedCopy((StateChangeData)next); > + pageContext.setAttribute("current", scd); > + } else { > + pageContext.setAttribute("current", next); > + } > return EVAL_BODY_AGAIN; > } > > > > _______________________________________________ > spacewalk-commits mailing list > spacewalk-comm...@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/spacewalk-commits > - -- Devan Goodwin <dgood...@redhat.com> Software Engineer Spacewalk / RHN Satellite Halifax, Canada 650.567.9039x79267 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.10 (GNU/Linux) iEYEARECAAYFAkoxEtUACgkQAyHWaPV9my5clwCgpcnij8mSl/sBJDASX5/yi3yd 03MAn1c4BJH5QTR+l1amA1BB7IxBsrCp =jsjE -----END PGP SIGNATURE-----
diff --git a/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java b/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java index a5bddc0..761877b 100644 --- a/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java +++ b/java/code/src/com/redhat/rhn/frontend/dto/monitoring/StateChangeData.java @@ -28,18 +28,9 @@ import java.util.StringTokenizer; public class StateChangeData extends BaseDto { private String oId; private String data; + private String htmlifiedData; private Long entryTime; - /** copies StateChangeData object and html escapes its data - * @param scd StateChangeData object to be copied - */ - public void createHtmlEscapedCopy(StateChangeData scd) { - oId = scd.oId; - entryTime = scd.entryTime; - data = StringUtil.htmlifyText(scd.data); - setSelected(scd.isSelected()); - } - /** * @return Returns the oId. */ @@ -64,6 +55,7 @@ public class StateChangeData extends BaseDto { */ public void setData(String dataIn) { this.data = dataIn; + this.htmlifiedData = StringUtil.htmlifyText(dataIn); } /** @@ -104,6 +96,14 @@ public class StateChangeData extends BaseDto { String message = data.substring(data.indexOf(" ") + 1); return message; } + + public String getHtmlifiedMessage() { + if (htmlifiedData == null) { + return null; + } + String message = htmlifiedData.substring(htmlifiedData.indexOf(" ") + 1); + return message; + } /** * The Message is the latter half of the "DATA" field: @@ -118,6 +118,14 @@ public class StateChangeData extends BaseDto { StringTokenizer st = new StringTokenizer(data, " "); return st.nextToken(); } + + public String getHtmlifiedState() { + if (htmlifiedData == null) { + return null; + } + StringTokenizer st = new StringTokenizer(htmlifiedData, " "); + return st.nextToken(); + } /** {...@inheritdoc} */ public Long getId() { diff --git a/java/code/webapp/WEB-INF/pages/systems/probes/details.jsp b/java/code/webapp/WEB-INF/pages/systems/probes/details.jsp index f5842d9..327b75a 100644 --- a/java/code/webapp/WEB-INF/pages/systems/probes/details.jsp +++ b/java/code/webapp/WEB-INF/pages/systems/probes/details.jsp @@ -119,10 +119,10 @@ ${current.entryDate} </rhn:column> <rhn:column header="probedetails.jsp.state"> - ${current.state} + ${current.htmlifiedState} </rhn:column> <rhn:column header="probedetails.jsp.message"> - ${current.message} + ${current.htmlifiedMessage} </rhn:column> </rhn:listdisplay> </rhn:list>
_______________________________________________ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel