Skip to content

Commit 899b3c9

Browse files
vagishavagisha
committed
Address CR comments:
- Extract "Success" to KEY_SUCCESS constant - FlagRunAction: use getObject() with null check instead of getArray()[0] - ShowUserAction: pass form values via bean so user.jsp doesn't read from request
1 parent be02721 commit 899b3c9

3 files changed

Lines changed: 56 additions & 28 deletions

File tree

‎testresults/src/org/labkey/testresults/TestResultsController.java‎

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ public class TestResultsController extends SpringActionController
139139
private static final Logger _log = LogManager.getLogger(TestResultsController.class);
140140
private static final SimpleDateFormat MDYFormat = new SimpleDateFormat("MM/dd/yyyy");
141141

142+
private static final String KEY_SUCCESS = "Success";
143+
142144
private static final DefaultActionResolver _actionResolver = new DefaultActionResolver(TestResultsController.class);
143145

144146
// Tab name constants for menu highlighting
@@ -486,15 +488,15 @@ public Object execute(TrainRunForm form, BindException errors)
486488
{
487489
if (form.getRunId() == null)
488490
{
489-
return new ApiSimpleResponse(Map.of("Success", false, "error", "runId is required"));
491+
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "runId is required"));
490492
}
491493
int runId = form.getRunId();
492494
String trainString = form.getTrain();
493495
if (!Strings.CI.equals(trainString, "true") &&
494496
!Strings.CI.equals(trainString, "false") &&
495497
!Strings.CI.equals(trainString, "force"))
496498
{
497-
return new ApiSimpleResponse(Map.of("Success", false, "error", "train must be one of: true, false, force"));
499+
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "train must be one of: true, false, force"));
498500
}
499501
boolean train = Strings.CI.equals(trainString, "true"); // true = add to training set, false = remove
500502
boolean force = Strings.CI.equals(trainString, "force");
@@ -511,9 +513,9 @@ public Object execute(TrainRunForm form, BindException errors)
511513
if (!force)
512514
{
513515
if (details.length == 0)
514-
return new ApiSimpleResponse(Map.of("Success", false, "error", "run does not exist: " + runId));
516+
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId));
515517
else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty()))
516-
return new ApiSimpleResponse(Map.of("Success", false, "error", "no action necessary"));
518+
return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary"));
517519
}
518520
DbScope scope = TestResultsSchema.getSchema().getScope();
519521
try (DbScope.Transaction transaction = scope.ensureTransaction())
@@ -550,7 +552,7 @@ else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty()))
550552
new SqlExecutor(scope).execute(sqlFragmentUpdate);
551553
transaction.commit();
552554
}
553-
return new ApiSimpleResponse("Success", true);
555+
return new ApiSimpleResponse(KEY_SUCCESS, true);
554556
}
555557
}
556558

@@ -623,6 +625,10 @@ public ModelAndView getView(ShowUserForm form, BindException errors) throws Exce
623625
ensureRunDataCached(runs, false);
624626

625627
TestsDataBean bean = new TestsDataBean(runs, user == null ? new User[0] : new User[]{user});
628+
bean.setStartDate(startDate);
629+
bean.setEndDate(endDate);
630+
bean.setUsername(userName);
631+
bean.setDataInclude(dataInclude);
626632
JspView<TestsDataBean> view = new JspView<>("/org/labkey/testresults/view/user.jsp", bean);
627633
view.setTitle("User Results");
628634
return view;
@@ -956,7 +962,7 @@ public Object execute(RunIdForm form, BindException errors)
956962
ApiSimpleResponse response = new ApiSimpleResponse();
957963
if (form.getRunId() == null)
958964
{
959-
response.put("Success", false);
965+
response.put(KEY_SUCCESS, false);
960966
response.put("error", "runId is required");
961967
return response;
962968
}
@@ -972,11 +978,11 @@ public Object execute(RunIdForm form, BindException errors)
972978
Table.delete(TestResultsSchema.getTableInfoTestRuns(), rowId); // delete run last because of foreign key
973979
transaction.commit();
974980
} catch (Exception x) {
975-
response.put("Success", false);
981+
response.put(KEY_SUCCESS, false);
976982
response.put("error", x.getMessage());
977983
return response;
978984
}
979-
response.put("Success", true);
985+
response.put(KEY_SUCCESS, true);
980986
return response;
981987
}
982988
}
@@ -1003,7 +1009,7 @@ public Object execute(FlagRunForm form, BindException errors)
10031009
ApiSimpleResponse response = new ApiSimpleResponse();
10041010
if (form.getRunId() == null)
10051011
{
1006-
response.put("Success", false);
1012+
response.put(KEY_SUCCESS, false);
10071013
response.put("error", "runId is required");
10081014
return response;
10091015
}
@@ -1014,19 +1020,24 @@ public Object execute(FlagRunForm form, BindException errors)
10141020
SimpleFilter filter = new SimpleFilter();
10151021
filter.addCondition(FieldKey.fromParts("id"), rowId);
10161022
try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction()) {
1017-
RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class);
1018-
RunDetail detail = details[0];
1023+
RunDetail detail = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getObject(RunDetail.class);
1024+
if (detail == null)
1025+
{
1026+
response.put(KEY_SUCCESS, false);
1027+
response.put("error", "run not found: " + rowId);
1028+
return response;
1029+
}
10191030
if (form.getFlag() == null) // if not specified keep same
10201031
flag = detail.isFlagged();
10211032
detail.setFlagged(flag);
10221033
Table.update(null, TestResultsSchema.getTableInfoTestRuns(), detail, detail.getId());
10231034
transaction.commit();
10241035
} catch (Exception x) {
1025-
response.put("Success", false);
1036+
response.put(KEY_SUCCESS, false);
10261037
response.put("error", x.getMessage());
10271038
return response;
10281039
}
1029-
response.put("Success", true);
1040+
response.put(KEY_SUCCESS, true);
10301041
return response;
10311042
}
10321043
}
@@ -1591,7 +1602,7 @@ public Object execute(RetrainAllForm form, BindException errors)
15911602
transaction.commit();
15921603

15931604
ApiSimpleResponse response = new ApiSimpleResponse();
1594-
response.put("Success", true);
1605+
response.put(KEY_SUCCESS, true);
15951606
response.put("usersRetrained", usersRetrained);
15961607
response.put("totalTrainRuns", totalTrainRuns);
15971608
response.put("mode", form.getMode());
@@ -1601,7 +1612,7 @@ public Object execute(RetrainAllForm form, BindException errors)
16011612
{
16021613
_log.error("Error in RetrainAllAction", e);
16031614
ApiSimpleResponse response = new ApiSimpleResponse();
1604-
response.put("Success", false);
1615+
response.put(KEY_SUCCESS, false);
16051616
response.put("error", e.getMessage());
16061617
return response;
16071618
}
@@ -1654,13 +1665,13 @@ else if (xml.isEmpty())
16541665
} catch (Exception e) {
16551666
_log.info("XML failed to parse/store");
16561667
_log.info("Attempting to save file for a future post attempt");
1657-
res.put("Success", false);
1668+
res.put(KEY_SUCCESS, false);
16581669
res.put("Message", "Error Parsing XML attempting to save the XML file... " + NIGHTLY_POSTER.SaveXML(file, getContainer()));
16591670
res.put("Exception", e + NIGHTLY_POSTER.getStackTraceText(e));
16601671
return new ApiSimpleResponse(res);
16611672
}
16621673

1663-
return new ApiSimpleResponse("Success", true);
1674+
return new ApiSimpleResponse(KEY_SUCCESS, true);
16641675
}
16651676

16661677
private void DebugRequest(HttpServletRequest hsRequest)

‎testresults/src/org/labkey/testresults/view/TestsDataBean.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ public class TestsDataBean
5050
private String viewType;
5151
private Date startDate;
5252
private Date endDate;
53+
private String username;
54+
private String dataInclude;
5355
private Integer boundaryWarning;
5456
private Integer boundaryError;
5557

@@ -99,6 +101,26 @@ public void setViewType(String viewType)
99101
this.viewType = viewType;
100102
}
101103

104+
public String getUsername()
105+
{
106+
return username;
107+
}
108+
109+
public void setUsername(String username)
110+
{
111+
this.username = username;
112+
}
113+
114+
public String getDataInclude()
115+
{
116+
return dataInclude;
117+
}
118+
119+
public void setDataInclude(String dataInclude)
120+
{
121+
this.dataInclude = dataInclude;
122+
}
123+
102124
// Getters and Setters for fields
103125
public RunDetail[] getRuns() {
104126
RunDetail[] r = runs.values().toArray(new RunDetail[0]);

‎testresults/src/org/labkey/testresults/view/user.jsp‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,18 +48,13 @@
4848
4949
User userObj = data.getUsers().length == 1 ? data.getUsers()[0] : null;
5050
51-
HttpServletRequest req = getViewContext().getRequest();
52-
String startDate = req.getParameter("start");
53-
String endDate = req.getParameter("end");
54-
String user = req.getParameter("username");
55-
boolean showSingleUser = user != null && !user.isEmpty();
5651
DateFormat df = new SimpleDateFormat("MM/dd/yyyy");
5752
Date today = new Date();
58-
if (startDate == null)
59-
startDate = df.format(today);
60-
if (endDate == null)
61-
endDate = df.format(today);
62-
String dataInclude = req.getParameter("datainclude");
53+
String startDate = data.getStartDate() != null ? df.format(data.getStartDate()) : df.format(today);
54+
String endDate = data.getEndDate() != null ? df.format(data.getEndDate()) : df.format(today);
55+
String user = data.getUsername();
56+
boolean showSingleUser = user != null && !user.isEmpty();
57+
String dataInclude = data.getDataInclude();
6358
if (dataInclude == null ||
6459
(!dataInclude.equalsIgnoreCase("date") && !dataInclude.equalsIgnoreCase("train") && !dataInclude.equalsIgnoreCase("both")))
6560
dataInclude = "date";
@@ -132,7 +127,7 @@
132127
<h2><%=h(user)%></h2>
133128
<%
134129
String headerDate = startDate;
135-
if (getViewContext().getRequest().getParameter("end") != null)
130+
if (!startDate.equals(endDate))
136131
headerDate += " - " + endDate;
137132
%>
138133
<p><%=h(headerDate)%></p>

0 commit comments

Comments
 (0)