-----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
