#2693 Expansion of the Event search function #3074
Conversation
Expanded search function for events
There was a problem hiding this comment.
It is worth improving the appearance of this search engine, a proposal of what it could look like is in the issue, it may also be in a different direction, so that it matches the style schemes used on the website and in other forms.
- Id is missing
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces a new event search feature with a redesigned flexbox-based UI, enhanced date range filtering, and comprehensive backend support. Changes include a modernized JSP search interface with jQuery UI datepickers, a new DWR API endpoint, service and DAO layer methods, dynamic SQL query building in QueryUtils, comprehensive test coverage, and localization keys across 10 language files. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant JSP as events.jsp
participant DWR as EventsDwr.searchNew()
participant Svc as EventService
participant DAO as EventDAO.searchNew()
participant DB as Database
participant Comments as IUserCommentDAO
User->>JSP: Submit search form<br/>(dates, filters, keywords)
JSP->>DWR: Call searchNew(eventSourceTypes,<br/>statuses, alarmLevels,<br/>startDate, endDate, keywords, eventId)
activate DWR
DWR->>DWR: Parse date strings (dd-MM-yyyy)
DWR->>Svc: searchNew(...filters, userId)
activate Svc
Svc->>DAO: searchNew(...same params)
activate DAO
DAO->>DAO: QueryUtils.buildSearchSql(...)<br/>generates parameterized SQL
DAO->>DB: Execute SQL query
activate DB
DB-->>DAO: Result set (EventInstances)
deactivate DB
DAO->>Comments: Augment each event<br/>with comments
DAO-->>Svc: List<EventInstance>
deactivate DAO
Svc-->>DWR: List<EventInstance>
deactivate Svc
DWR->>DWR: Prepare JSP model<br/>(eventList, result count,<br/>control states)
DWR-->>JSP: DwrResponseI18n<br/>(HTML + localized message)
deactivate DWR
JSP->>JSP: Render results &<br/>update searchResults container
JSP-->>User: Display events & controls
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @src/com/serotonin/mango/web/dwr/EventsDwr.java:
- Around line 41-42: The server is using a 4-digit year pattern while the UI
datepicker sends 2-digit years; update the DateTimeFormat patterns used in
EventsDwr (notably in searchNew and the related parsing code in the 283-307
area) to match the UI by using DateTimeFormat.forPattern("dd-MM-yy") (two-digit
year, uppercase M for month) wherever dates from the UI are parsed so
DateTime.parse succeeds.
In @src/org/scada_lts/dao/event/EventDAO.java:
- Around line 566-568: The method getStatusActiveCondition() returns
STATUS_ASSIGNEE_CONDITION_SQL which is a name/behavior mismatch; either rename
the method to getStatusAssigneeCondition() to match the returned constant (and
update all call sites such as QueryUtils to use the new method name) or change
the return to STATUS_ACTIVE_CONDITION_SQL if the intent was to return the
active-status SQL constant; ensure you update any imports/usages and related
javadoc/comments to reflect the chosen semantics.
- Around line 1604-1640: searchNew currently executes an unbounded query and
calls userCommentDAO.getEventComments(e) for each EventInstance (N+1 problem);
modify searchNew to enforce a server-side max result limit or accept/return
pagination (e.g., add parameters like maxResults/offset or enforce a hard cap
before executing the query) and change comment enrichment to batch-load comments
by collecting eventIds from the result set and invoking a new or existing DAO
method (e.g., userCommentDAO.getCommentsForEventIds(List<Integer> eventIds)) to
map comments back onto EventInstance objects (or make enrichment optional via a
flag) instead of calling getEventComments per row; keep UserEventRowMapper usage
for row mapping but populate comments after batch query.
In @src/org/scada_lts/utils/QueryUtils.java:
- Around line 153-155: The SQL fragment built from orKeywords is missing spaces
around the OR token, producing invalid SQL; in QueryUtils.java update the
where.add call that uses String.join("OR", orKeywords) to use String.join(" OR
", orKeywords) so the clause becomes "(" + String.join(" OR ", orKeywords) + ")"
(refer to the orKeywords variable and the where.add(...) line in the QueryUtils
class).
- Around line 89-91: The loops in addEventSourceTypesCondition and
addAlarmLevelsCondition call Integer.valueOf(src) on user-supplied strings which
can throw NumberFormatException; wrap the conversion in a try-catch (catch
NumberFormatException) and skip or collect invalid entries instead of letting
the exception propagate, and optionally log a warning including the offending
value and the method name (addEventSourceTypesCondition /
addAlarmLevelsCondition) so malformed inputs are ignored safely and valid
integers are still added to params.
In @webapp-resources/messages_pl.properties:
- Around line 3513-3514: The two Polish locale entries are still in English:
replace the values for events.search.startDate and events.search.endDate with
Polish translations; update events.search.startDate to "Data rozpoczęcia" and
events.search.endDate to "Data zakończenia" so the PL UI shows localized labels.
In @WebContent/WEB-INF/jsp/events.jsp:
- Around line 157-158: The code sets
jQuery('#searchMessage').text("Searching...") with a hardcoded English string;
replace that literal with the localized message key used by doSearchOld() and
doSearch(), i.e. use the Spring message for events.search.searching instead of
the hardcoded "Searching..." so the UI respects localization (for example by
emitting the localized text into the JS from JSP via <spring:message
code="events.search.searching"/> and using that value in the
jQuery('#searchMessage').text(...) call).
🧹 Nitpick comments (14)
webapp-resources/messages_nl.properties (1)
3491-3492: Localization values are in English rather than Dutch.The newly added
events.search.startDateandevents.search.endDatekeys use English text ("Start date", "End date") rather than Dutch translations. Existing related keys in this section show mixed usage—some are Dutch (e.g.,events.search.search=Zoek), while others remain English (e.g.,events.search.keywords=Keywords).Per learnings, SCADA-LTS does not actively maintain non-English/Polish translations, and AI translation may be applied in the future, so this is not a blocker. However, if a Dutch translation is desired now, consider using "Startdatum" and "Einddatum" (or "Begindatum"/"Einddatum") instead.
webapp-resources/messages_ru.properties (1)
3524-3525: Russian bundle contains English labels for new date-range fields.
Given repo practice this may be fine, but ifruis user-facing, consider translating (or reusing existingcommon.dateRangeFrom/Tosemantics) so the UI isn’t mixed-language. Based on learnings, this shouldn’t block the PR.Proposed tweak (optional)
-events.search.startDate=Start date -events.search.endDate=End date +events.search.startDate=Дата начала +events.search.endDate=Дата окончанияwebapp-resources/messages_de.properties (1)
3373-3374: Optional: translate these labels for German UI consistency.If you want DE UI to be fully localized, consider:
Optional tweak
-events.search.startDate=Start date -events.search.endDate=End date +events.search.startDate=Startdatum +events.search.endDate=EnddatumBased on learnings, non-maintained locales shouldn’t block this PR.
webapp-resources/messages_pt.properties (1)
3528-3529: Consider providing Portuguese translations for the new event search date labels.Lines 3528-3529 introduce new UI labels in English ("Start date", "End date") in a Portuguese localization file. Nearby keys (e.g.,
common.dateRangeFrom=Deat line 31) use Portuguese. For consistency and proper localization, consider translating these to Portuguese (e.g., "Data de início" and "Data de término" or similar variants used elsewhere in the file).However, per learnings, SCADA-LTS does not actively maintain non-English locales like Portuguese currently, so English text here is acceptable as a temporary solution pending future AI-assisted translation efforts.
webapp-resources/messages_es.properties (1)
3416-3417: Spanish bundle values are English; OK to ship, but consider translating or reusing existing “Desde/Hasta” keys.
Given current practice, I wouldn’t block onmessages_es.properties, but if you want proper ES: “Fecha de inicio” / “Fecha de fin” (or reusecommon.dateRangeFrom/common.dateRangeTo). Based on learnings, non-maintained locales should not block changes.webapp-resources/messages_en.properties (1)
3376-3378: LGTM — labels are clear; optional: consider reusing existingcommon.dateRangeFrom/Toto reduce key duplication.src/org/scada_lts/mango/service/EventService.java (1)
718-734: Simplify delegation + consider@Transactional(readOnly = true)for search.This method can be reduced to
return eventDAO.searchNew(...);and (optionally) annotated as read-only for consistency/perf depending on your Spring transaction strategy.Proposed simplification
public List<EventInstance> searchNew(String[] eventSourceTypes, String[] statuses, String[] alarmLevels, Date startDate, Date endDate, String[] keywordArr, int userId, ResourceBundle bundle, int eventId) { - List<EventInstance> results = eventDAO.searchNew( - eventSourceTypes, - statuses, - alarmLevels, - startDate, - endDate, - keywordArr, - userId, - bundle, - eventId - ); - - return results; + return eventDAO.searchNew( + eventSourceTypes, statuses, alarmLevels, + startDate, endDate, keywordArr, + userId, bundle, eventId + ); }test/org/scada_lts/utils/QueryUtilsTest.java (1)
1-257: Good coverage for SQL + parameter ordering; consider reducing whitespace brittleness.These tests provide solid safety for param ordering/count. If QueryUtils formatting changes, some
contains(...)assertions may become flaky—consider normalizing whitespace or using regex for the keyword/status blocks.WebContent/WEB-INF/jsp/events.jsp (5)
46-50: Redundant datepicker initialization.The datepickers are initialized twice - once on line 46 without options and again on lines 49-50 with
dateFormat. The first initialization is unnecessary and can cause unexpected behavior.♻️ Proposed fix
jQuery(function() { - jQuery("#startDate, #endDate").datepicker(); - - // Initialize the datepickers on each input jQuery("#startDate").datepicker({ dateFormat: 'dd-mm-yy' }); jQuery("#endDate").datepicker({ dateFormat: 'dd-mm-yy' });
172-175: Error handling provides no useful context.When
response.hasMessagesis true, the user sees only "Error or messages found" with no details about what went wrong. Consider extracting and displaying the actual messages.♻️ Proposed improvement
if (response.hasMessages) { - - jQuery('#searchMessage').text("Error or messages found"); + var errorText = ""; + if (response.messages && response.messages.length > 0) { + for (var i = 0; i < response.messages.length; i++) { + errorText += response.messages[i].contextualMessage + " "; + } + } else { + errorText = "<spring:message code='common.error'/>"; + } + jQuery('#searchMessage').text(errorText); }
154-156: Missing input validation for eventId.
parseIntis applied to the eventId value, but if the user enters a non-numeric string,parseIntreturnsNaN, which is then passed to the backend. Consider validating before the DWR call.♻️ Proposed fix
var eventIdStr = jQuery('#eventId').val(); - var eventId = eventIdStr ? parseInt(eventIdStr, 10) : 0; + var eventId = 0; + if (eventIdStr) { + eventId = parseInt(eventIdStr, 10); + if (isNaN(eventId)) { + jQuery('#searchMessage').text("<spring:message code='badIntegerFormat'/>"); + return; + } + }
268-280: Date inputs lack validation for format and logical order.Users can enter invalid date formats or specify an end date before the start date. Consider adding client-side validation before submitting the search.
256-261: Accessibility: form inputs lack proper association.The labels use
forattributes, but the association relies onidmatching. Consider addingaria-labeloraria-describedbyfor screen readers, especially for the date fields with calendar icons.Also applies to: 264-294
src/org/scada_lts/utils/QueryUtils.java (1)
51-81: Consider adding a LIMIT clause to prevent unbounded result sets.The query has no row limit, which could cause performance issues or memory exhaustion with large event tables. The old
doSearchOldfunction used amaxResultsparameter.♻️ Proposed enhancement
Update the method signature and SQL:
public static EventSearchQuery buildSearchSql( int userId, String[] eventSourceTypes, String[] statuses, String[] alarmLevels, Date startDate, Date endDate, String[] keywordArr, - int eventId + int eventId, + int maxResults ) { // ... existing code ... - sql.append(" ORDER BY e.activeTs DESC"); + sql.append(" ORDER BY e.activeTs DESC"); + if (maxResults > 0) { + sql.append(" LIMIT ?"); + params.add(maxResults); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
WebContent/WEB-INF/jsp/events.jspbuild.gradlesrc/com/serotonin/mango/web/dwr/EventsDwr.javasrc/org/scada_lts/dao/event/EventDAO.javasrc/org/scada_lts/mango/service/EventService.javasrc/org/scada_lts/utils/QueryUtils.javatest/org/scada_lts/utils/QueryUtilsTest.javawebapp-resources/messages_de.propertieswebapp-resources/messages_en.propertieswebapp-resources/messages_es.propertieswebapp-resources/messages_fi.propertieswebapp-resources/messages_fr.propertieswebapp-resources/messages_lu.propertieswebapp-resources/messages_nl.propertieswebapp-resources/messages_pl.propertieswebapp-resources/messages_pt.propertieswebapp-resources/messages_ru.propertieswebapp-resources/messages_zh.properties
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T14:39:51.606Z
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: webapp-resources/messages_fi.properties:3565-3565
Timestamp: 2026-01-05T14:39:51.606Z
Learning: SCADA-LTS maintains translations only for Polish and English. Do not invest in or expect active maintenance for other languages (e.g., messages_fi.properties, messages_de.properties). If a messages_*.properties file contains English text and is not maintained, it can be left as-is for now. AI translation may be used in the future. During reviews, prioritize Polish and English keys/text; non-maintained locales should not block changes.
Applied to files:
webapp-resources/messages_fi.propertieswebapp-resources/messages_lu.propertieswebapp-resources/messages_en.propertieswebapp-resources/messages_pt.propertieswebapp-resources/messages_nl.propertieswebapp-resources/messages_fr.propertieswebapp-resources/messages_es.propertieswebapp-resources/messages_pl.propertieswebapp-resources/messages_de.propertieswebapp-resources/messages_ru.propertieswebapp-resources/messages_zh.properties
🧬 Code graph analysis (2)
src/org/scada_lts/dao/event/EventDAO.java (2)
src/org/scada_lts/utils/QueryUtils.java (1)
QueryUtils(10-169)src/org/scada_lts/web/beans/ApplicationBeans.java (1)
ApplicationBeans(31-206)
test/org/scada_lts/utils/QueryUtilsTest.java (1)
src/org/scada_lts/utils/QueryUtils.java (1)
QueryUtils(10-169)
🔇 Additional comments (9)
webapp-resources/messages_fi.properties (1)
3501-3502: Additions look correct; OK to leave English here.Keys are clear and consistent with the event search UI labeling pattern. Since non-EN/PL locales aren’t actively maintained, leaving the values in English is fine and shouldn’t block the PR. (Based on learnings, SCADA-LTS maintains translations only for Polish and English.)
webapp-resources/messages_lu.properties (1)
3389-3390: LGTM — consistent new keys.The added
events.search.startDate/events.search.endDatekeys match the intended UI labels; keeping English values is acceptable for non-maintained locales. (Based on learnings, SCADA-LTS maintains translations only for Polish and English.)webapp-resources/messages_fr.properties (1)
3370-3371: English text in French locale file is acceptable per project policy.The new localization keys
events.search.startDateandevents.search.endDatecontain English text ("Start date" and "End date") rather than French translations. This is acceptable since French is not an actively maintained locale in SCADA-LTS.Based on learnings, only Polish and English locales are actively maintained. Non-maintained locale files like
messages_fr.propertiescan contain English text as a placeholder until AI translation or community contributions are provided in the future.build.gradle (1)
248-248: LGTM! Test filter correctly added.The new test filter for
QueryUtilsTestis properly added following the existing pattern. This enables execution of the test suite that validates the SQL query building functionality for the event search feature introduced in this PR.webapp-resources/messages_zh.properties (1)
3476-3477: New i18n keys added; zh values are English (non-blocking).Given SCADA-LTS translation maintenance focus (EN/PL), adding the keys is good; leaving English text in
messages_zh.propertiesis acceptable for now. Based on learnings, non-maintained locales shouldn’t block changes.src/org/scada_lts/utils/QueryUtils.java (4)
23-39: LGTM!The
EventSearchQueryinner class is well-structured with immutable fields and clean accessors.
41-49: LGTM!The
buildInClausehelper correctly builds parameterized IN clauses, avoiding SQL injection.
130-142: End date filter may exclude events on the selected day.Using
e.activeTs <= ?withendDate.getTime()only includes events up to the start of the end date (midnight). To include the entire end day, consider adding 24 hours minus 1 millisecond, or document this as expected behavior.Verify if the frontend sends end-of-day timestamps or if users expect inclusive end dates. If users select "09-01-2026" as the end date, they likely expect events from that entire day to be included.
95-116: LGTM!The status condition builder correctly handles multiple status types with proper OR logic and uses predefined constants from
EventsDwr.
| import org.joda.time.format.DateTimeFormat; | ||
| import org.joda.time.format.DateTimeFormatter; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the UI code that calls EventsDwr.searchNew and the datepicker/date formatting configuration.
find . -type f \( -name "*.jsp" -o -name "*.js" -o -name "*.html" \) | head -20
rg -n "searchNew\(" --max-count=10 || true
rg -n "datepicker|dateFormat|startDateStr|endDateStr" --max-count=20 || true
rg -n "dd-MM-yyyy" --max-count=10 || trueRepository: SCADA-LTS/Scada-LTS
Length of output: 998
🏁 Script executed:
cat -n src/com/serotonin/mango/web/dwr/EventsDwr.java | sed -n '35,50p'
cat -n src/com/serotonin/mango/web/dwr/EventsDwr.java | sed -n '275,315p'Repository: SCADA-LTS/Scada-LTS
Length of output: 2254
🏁 Script executed:
# Search for searchNew calls in web files
rg -n "searchNew" -A 5 -B 5 --type-list | head -20 || true
rg -n "searchNew" WebContent -A 3 || true
# Search for datepicker and date format configuration in JS files
find WebContent -name "*.js" | xargs grep -l "datepicker\|dateFormat" 2>/dev/null | head -10 || true
# Look for events.jsp or similar
find WebContent -name "*events*" -type f | head -20 || trueRepository: SCADA-LTS/Scada-LTS
Length of output: 1421
🏁 Script executed:
# Search for how dates are formatted when sent to the server
rg -n "startDate|endDate" WebContent -A 2 -B 2 --type-list | head -40 || true
# Look for masking or formatting patterns in JS
rg -n "dd-MM-yyyy|yy-mm-dd|yyyy-mm-dd|MM/dd/yyyy" WebContent || trueRepository: SCADA-LTS/Scada-LTS
Length of output: 50783
🏁 Script executed:
# Find and read events.jsp
find WebContent -name "events.jsp" -type f
# Get the context around the searchNew call
cat -n WebContent/WEB-INF/jsp/events.jsp | sed -n '155,175p'
# Search for datepicker configuration in events.jsp
cat -n WebContent/WEB-INF/jsp/events.jsp | grep -A 5 -B 5 "datepicker\|dateFormat" | head -60Repository: SCADA-LTS/Scada-LTS
Length of output: 3857
🏁 Script executed:
# Also check for any date formatting when sending to server
rg -n "startDate|endDate" WebContent/WEB-INF/jsp/events.jsp -A 3 -B 3 | head -80Repository: SCADA-LTS/Scada-LTS
Length of output: 2563
Date format mismatch between UI and server will cause parsing failures.
UI datepicker (events.jsp, lines 49-50) sends dates as dd-mm-yy format (2-digit year: "25-12-24"), but EventsDwr.searchNew (line 297) expects dd-MM-yyyy format (4-digit year: "25-12-2024"). The year pattern difference will cause DateTime.parse() to fail. Update EventsDwr to use DateTimeFormat.forPattern("dd-mm-yy") to match the UI, or reconfigure the UI datepicker to 'dd-mm-yyyy'.
Also applies to: 283-307
🤖 Prompt for AI Agents
In @src/com/serotonin/mango/web/dwr/EventsDwr.java around lines 41 - 42, The
server is using a 4-digit year pattern while the UI datepicker sends 2-digit
years; update the DateTimeFormat patterns used in EventsDwr (notably in
searchNew and the related parsing code in the 283-307 area) to match the UI by
using DateTimeFormat.forPattern("dd-MM-yy") (two-digit year, uppercase M for
month) wherever dates from the UI are parsed so DateTime.parse succeeds.
| public DwrResponseI18n searchNew( | ||
| String[] eventSourceTypes, | ||
| String[] statuses, | ||
| String[] alarmLevels, | ||
| String startDateStr, | ||
| String endDateStr, | ||
| String keywords, | ||
| int eventId) | ||
| { | ||
| DwrResponseI18n response = new DwrResponseI18n(); | ||
| HttpServletRequest request = WebContextFactory.get().getHttpServletRequest(); | ||
| User user = Common.getUser(request); | ||
|
|
||
| // Parse the date strings | ||
| DateTimeFormatter formatter = DateTimeFormat.forPattern("dd-MM-yyyy"); | ||
| Date startDate = null; | ||
| Date endDate = null; | ||
|
|
||
| if (startDateStr != null && !startDateStr.trim().isEmpty()) { | ||
| startDate = DateTime.parse(startDateStr.trim(), formatter).toDate(); | ||
| } | ||
| if (endDateStr != null && !endDateStr.trim().isEmpty()) { | ||
| endDate = DateTime.parse(endDateStr.trim(), formatter).toDate(); | ||
| } | ||
|
|
||
|
|
||
| // Split keywords | ||
| String[] keywordArr = getKeywords(keywords); | ||
|
|
||
| // Use event service class to perform the search. | ||
| EventService eventService = new EventService(); | ||
| List<EventInstance> results = eventService.searchNew( | ||
| eventSourceTypes, | ||
| statuses, | ||
| alarmLevels, | ||
| startDate, | ||
| endDate, | ||
| keywordArr, | ||
| user.getId(), | ||
| getResourceBundle(), | ||
| eventId | ||
| ); | ||
|
|
||
| // Prepare the JSP model | ||
| Map<String, Object> model = new HashMap<>(); | ||
| model.put("events", results); | ||
| model.put("showControls", false); | ||
|
|
||
| SystemSettingsService systemSettingsService = new SystemSettingsService(); | ||
| model.put("isEventAssignEnabled", systemSettingsService.isEventAssignEnabled()); | ||
|
|
||
| response.addData("content", generateContent(request, "eventList.jsp", model)); | ||
| response.addData("resultCount", new LocalizableMessage("events.search.resultCount", results.size())); | ||
|
|
||
| return response; | ||
| } |
There was a problem hiding this comment.
Fix end-date inclusivity, null keywords, and date-parse error handling.
Current behavior likely excludes the entire endDate day (parsed at midnight) and can throw NPE/parse exceptions (keywords == null and invalid date strings).
Proposed fix (inclusive end-of-day + null-safe keywords + parse guard)
public DwrResponseI18n searchNew(
String[] eventSourceTypes,
String[] statuses,
String[] alarmLevels,
String startDateStr,
String endDateStr,
String keywords,
int eventId)
{
DwrResponseI18n response = new DwrResponseI18n();
HttpServletRequest request = WebContextFactory.get().getHttpServletRequest();
User user = Common.getUser(request);
// Parse the date strings
DateTimeFormatter formatter = DateTimeFormat.forPattern("dd-MM-yyyy");
Date startDate = null;
Date endDate = null;
- if (startDateStr != null && !startDateStr.trim().isEmpty()) {
- startDate = DateTime.parse(startDateStr.trim(), formatter).toDate();
- }
- if (endDateStr != null && !endDateStr.trim().isEmpty()) {
- endDate = DateTime.parse(endDateStr.trim(), formatter).toDate();
- }
+ try {
+ if (startDateStr != null && !startDateStr.trim().isEmpty()) {
+ // start of day
+ startDate = DateTime.parse(startDateStr.trim(), formatter).toDate();
+ }
+ if (endDateStr != null && !endDateStr.trim().isEmpty()) {
+ // inclusive end of day
+ endDate = DateTime.parse(endDateStr.trim(), formatter).plusDays(1).minusMillis(1).toDate();
+ }
+ } catch (IllegalArgumentException ex) {
+ // TODO: wire this into your standard DWR error mechanism/message keys
+ response.addData("resultCount", new LocalizableMessage("badDateFormat"));
+ response.addData("content", "");
+ return response;
+ }
// Split keywords
- String[] keywordArr = getKeywords(keywords);
+ String[] keywordArr = (keywords == null || keywords.trim().isEmpty()) ? null : getKeywords(keywords);
// Use event service class to perform the search.
EventService eventService = new EventService();
List<EventInstance> results = eventService.searchNew(
eventSourceTypes,
statuses,
alarmLevels,
startDate,
endDate,
keywordArr,
user.getId(),
getResourceBundle(),
eventId
);| public static String getStatusActiveCondition() { | ||
| return STATUS_ASSIGNEE_CONDITION_SQL; | ||
| } |
There was a problem hiding this comment.
getStatusActiveCondition() name/return mismatch (returns assignee condition).
The method name suggests “active status”, but it returns STATUS_ASSIGNEE_CONDITION_SQL; this is easy to misuse and already reads oddly in QueryUtils. Consider renaming to getStatusAssigneeCondition() (or returning STATUS_ACTIVE_CONDITION_SQL if that was intended).
🤖 Prompt for AI Agents
In @src/org/scada_lts/dao/event/EventDAO.java around lines 566 - 568, The method
getStatusActiveCondition() returns STATUS_ASSIGNEE_CONDITION_SQL which is a
name/behavior mismatch; either rename the method to getStatusAssigneeCondition()
to match the returned constant (and update all call sites such as QueryUtils to
use the new method name) or change the return to STATUS_ACTIVE_CONDITION_SQL if
the intent was to return the active-status SQL constant; ensure you update any
imports/usages and related javadoc/comments to reflect the chosen semantics.
| public List<EventInstance> searchNew( | ||
| String[] eventSourceTypes, | ||
| String[] statuses, | ||
| String[] alarmLevels, | ||
| Date startDate, | ||
| Date endDate, | ||
| String[] keywordArr, | ||
| int userId, | ||
| ResourceBundle bundle, | ||
| int eventId | ||
| ) { | ||
| QueryUtils.EventSearchQuery eq = QueryUtils.buildSearchSql( | ||
| userId, | ||
| eventSourceTypes, | ||
| statuses, | ||
| alarmLevels, | ||
| startDate, | ||
| endDate, | ||
| keywordArr, | ||
| eventId | ||
| ); | ||
|
|
||
| final List<EventInstance> results = new ArrayList<>(); | ||
| final UserEventRowMapper rowMapper = new UserEventRowMapper(); | ||
|
|
||
| DAO.getInstance().getJdbcTemp().query(eq.getSql(), eq.getParamsArray(), rs -> { | ||
| IUserCommentDAO userCommentDAO = ApplicationBeans.getUserCommentDaoBean(); | ||
| while (rs.next()) { | ||
| EventInstance e = rowMapper.mapRow(rs, 0); | ||
| e.setEventComments(userCommentDAO.getEventComments(e)); | ||
| results.add(e); | ||
| } | ||
| return null; | ||
| }); | ||
|
|
||
| return results; | ||
| } |
There was a problem hiding this comment.
Guard against unbounded result sets + avoid N+1 comment queries.
searchNew can return an unbounded number of events and then performs userCommentDAO.getEventComments(e) per row, which can get expensive quickly (especially via the new UI search). Consider:
- adding a hard server-side cap / pagination (even if UI sends “maxResults”), and
- batch-loading comments (single query for all eventIds) or making comment enrichment optional.
🤖 Prompt for AI Agents
In @src/org/scada_lts/dao/event/EventDAO.java around lines 1604 - 1640,
searchNew currently executes an unbounded query and calls
userCommentDAO.getEventComments(e) for each EventInstance (N+1 problem); modify
searchNew to enforce a server-side max result limit or accept/return pagination
(e.g., add parameters like maxResults/offset or enforce a hard cap before
executing the query) and change comment enrichment to batch-load comments by
collecting eventIds from the result set and invoking a new or existing DAO
method (e.g., userCommentDAO.getCommentsForEventIds(List<Integer> eventIds)) to
map comments back onto EventInstance objects (or make enrichment optional via a
flag) instead of calling getEventComments per row; keep UserEventRowMapper usage
for row mapping but populate comments after batch query.
| for (String src : eventSourceTypes) { | ||
| params.add(Integer.valueOf(src)); | ||
| } |
There was a problem hiding this comment.
Integer.valueOf may throw NumberFormatException.
If the frontend sends malformed data (non-numeric strings), Integer.valueOf(src) will throw an uncaught exception. Consider adding validation or wrapping in try-catch.
🐛 Proposed fix for addEventSourceTypesCondition
for (String src : eventSourceTypes) {
- params.add(Integer.valueOf(src));
+ try {
+ params.add(Integer.valueOf(src));
+ } catch (NumberFormatException e) {
+ // Skip invalid values or log warning
+ }
}Apply the same pattern to addAlarmLevelsCondition at lines 124-126.
Also applies to: 124-126
🤖 Prompt for AI Agents
In @src/org/scada_lts/utils/QueryUtils.java around lines 89 - 91, The loops in
addEventSourceTypesCondition and addAlarmLevelsCondition call
Integer.valueOf(src) on user-supplied strings which can throw
NumberFormatException; wrap the conversion in a try-catch (catch
NumberFormatException) and skip or collect invalid entries instead of letting
the exception propagate, and optionally log a warning including the offending
value and the method name (addEventSourceTypesCondition /
addAlarmLevelsCondition) so malformed inputs are ignored safely and valid
integers are still added to params.
| if (!orKeywords.isEmpty()) { | ||
| where.add("(" + String.join("OR", orKeywords) + ")"); | ||
| } |
There was a problem hiding this comment.
Missing spaces around OR keyword produces invalid SQL.
String.join("OR", orKeywords) generates e.message LIKE ?ORe.message LIKE ? instead of e.message LIKE ? OR e.message LIKE ?.
🐛 Proposed fix
if (!orKeywords.isEmpty()) {
- where.add("(" + String.join("OR", orKeywords) + ")");
+ where.add("(" + String.join(" OR ", orKeywords) + ")");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!orKeywords.isEmpty()) { | |
| where.add("(" + String.join("OR", orKeywords) + ")"); | |
| } | |
| if (!orKeywords.isEmpty()) { | |
| where.add("(" + String.join(" OR ", orKeywords) + ")"); | |
| } |
🤖 Prompt for AI Agents
In @src/org/scada_lts/utils/QueryUtils.java around lines 153 - 155, The SQL
fragment built from orKeywords is missing spaces around the OR token, producing
invalid SQL; in QueryUtils.java update the where.add call that uses
String.join("OR", orKeywords) to use String.join(" OR ", orKeywords) so the
clause becomes "(" + String.join(" OR ", orKeywords) + ")" (refer to the
orKeywords variable and the where.add(...) line in the QueryUtils class).
| events.search.startDate=Start date | ||
| events.search.endDate=End date No newline at end of file |
There was a problem hiding this comment.
Polish locale strings are in English; translate these two labels.
These keys will show up in the PL UI as English currently.
Proposed fix
-events.search.startDate=Start date
-events.search.endDate=End date
+events.search.startDate=Data początkowa
+events.search.endDate=Data końcowa📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| events.search.startDate=Start date | |
| events.search.endDate=End date | |
| events.search.startDate=Data początkowa | |
| events.search.endDate=Data końcowa |
🤖 Prompt for AI Agents
In @webapp-resources/messages_pl.properties around lines 3513 - 3514, The two
Polish locale entries are still in English: replace the values for
events.search.startDate and events.search.endDate with Polish translations;
update events.search.startDate to "Data rozpoczęcia" and events.search.endDate
to "Data zakończenia" so the PL UI shows localized labels.
| jQuery('#searchBtn').prop('disabled', true); | ||
| jQuery('#searchMessage').text("Searching..."); |
There was a problem hiding this comment.
Hardcoded string bypasses localization.
"Searching..." is hardcoded instead of using the existing <spring:message code="events.search.searching"/> key that's used in doSearchOld() and doSearch().
🌐 Proposed fix
jQuery('#searchBtn').prop('disabled', true);
- jQuery('#searchMessage').text("Searching...");
+ $set("searchMessage", "<spring:message code='events.search.searching'/>");Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @WebContent/WEB-INF/jsp/events.jsp around lines 157 - 158, The code sets
jQuery('#searchMessage').text("Searching...") with a hardcoded English string;
replace that literal with the localized message key used by doSearchOld() and
doSearch(), i.e. use the Spring message for events.search.searching instead of
the hardcoded "Searching..." so the UI respects localization (for example by
emitting the localized text into the JS from JSP via <spring:message
code="events.search.searching"/> and using that value in the
jQuery('#searchMessage').text(...) call).
Expanded search function for events
Summary by CodeRabbit
New Features
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.