Conversation
|
Sorry, No meaningful review files are found. So, all good. |
|
/review |
|
Sorry, No meaningful review files are found. So, all good. |
|
/review |
|
PR Summary: Summary: |
|
Reviewed up to commit:99c9f20239dbbf446ed3962c5cca742be05cb88d Looks good to me! 👍 Additional Suggestionfunctions/poll_service_java/ResponseData.java, line:57-68The method validateDuration does not return any value if a ParseException or Exception is caught. This can lead to unexpected behavior at runtime. Ensure that a proper Boolean value is returned in the catch blocks.private static Boolean validateDuration(String duration) {
DateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
Date endTime;
try {
endTime = dateFormat.parse(duration);
return endTime.before(getCurrentISTTime());
} catch (ParseException e) {
return false;
} catch (Exception e) {
e.printStackTrace();
return false;
}
} |
|
/review |
![pantoaibot[bot]](https://avatars.githubusercontent.com/in/934356?s=60&v=4){kind=small}
| responseData.remove("pollMap"); | ||
|
|
||
| } else if (url.equals("/MyPolls") && method.equals(POST)) { | ||
| } else if (url.equals("/MyPoll") && method.equals(POST)) { |
There was a problem hiding this comment.
[CRITICAL_BUG] The endpoint change from '/MyPolls' to '/MyPoll' could break backward compatibility. Verify that this update is intentional and that all clients are updated accordingly.
|
Reviewed up to commit:16cf933912d3fad14d141050e24bb9b6c159e3c0 Additional Suggestionfunctions/poll_service_java/ResponseData.java, line:57-69In validateDuration, not all execution paths return a boolean value. The empty catch blocks may lead to a missing return value; consider returning a default (e.g., false) and logging the error.private static Boolean validateDuration(String duration) {
DateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
Date endTime;
try {
endTime = dateFormat.parse(duration);
return endTime.before(getCurrentISTTime());
} catch (ParseException e) {
// Optionally log the error
return false;
} catch (Exception e) {
e.printStackTrace();
return false;
}
}functions/poll_service_java/ResponseData.java, line:32Typo in method name 'getMaxximumPolled' - consider renaming it to 'getMaximumPolled' to improve code readability.private HashMap<String, Object> getMaximumPolled(String rowId) {
// ...implementation...
}
// And update the call site:
endedData.putAll(getMaximumPolled(zcRowObject.get("ROWID").toString())); |
|
/review check it clearly |
| responseData.remove("pollMap"); | ||
|
|
||
| } else if (url.equals("/MyPolls") && method.equals(POST)) { | ||
| } else if (url.equals("/MyPoll") && method.equals(POST)) { |
There was a problem hiding this comment.
[CRITICAL_BUG] The endpoint change from '/MyPolls' to '/MyPoll' could break backward compatibility. Verify that this update is intentional and that all clients are updated accordingly.
|
Reviewed up to commit:16cf933912d3fad14d141050e24bb9b6c159e3c0 Additional Suggestionfunctions/poll_service_java/ResponseData.java, line:57-69In validateDuration, not all execution paths return a boolean value. The empty catch blocks may lead to a missing return value; consider returning a default (e.g., false) and logging the error.private static Boolean validateDuration(String duration) {
DateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
Date endTime;
try {
endTime = dateFormat.parse(duration);
return endTime.before(getCurrentISTTime());
} catch (ParseException e) {
// Optionally log the error
return false;
} catch (Exception e) {
e.printStackTrace();
return false;
}
}functions/poll_service_java/ResponseData.java, line:32Typo in method name 'getMaxximumPolled' - consider renaming it to 'getMaximumPolled' to improve code readability.private HashMap<String, Object> getMaximumPolled(String rowId) {
// ...implementation...
}
// And update the call site:
endedData.putAll(getMaximumPolled(zcRowObject.get("ROWID").toString())); |
|
code review - @coderabbitai |
|
@aravind-dd-11556 I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
WalkthroughTwo changes: removed the Boolean edited parameter from ResponseData’s constructor and related result field; updated Service routing to use POST "/MyPoll" instead of "/MyPolls" while invoking the same getMyPolls handler. No other logic paths were altered. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Service
participant Handler as getMyPolls
Client->>Service: POST /MyPoll
Note over Service: Updated route path
Service->>Handler: invoke getMyPolls(request)
Handler-->>Service: response payload
Service-->>Client: 200 OK / error
sequenceDiagram
autonumber
participant Caller
participant ResponseData
Caller->>ResponseData: new ResponseData(zcRowObject, votedData)
Note over ResponseData: 'edited' flag no longer accepted/stored
ResponseData-->>Caller: initialized instance (no edited field in result)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Docstrings generation was requested by @aravind-dd-11556. * #2 (comment) The following files were modified: * `functions/poll_service_java/ResponseData.java` * `functions/poll_service_java/Service.java`
|
Note Generated docstrings for this pull request at #3 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/poll_service_java/Service.java (1)
246-246: Fix ResponseData constructor usagesResponseData now expects two parameters—remove the third boolean argument at functions/poll_service_java/Service.java:246, 318, 367, 439, 542, replacing
new ResponseData(rowData, votedData, …)withnew ResponseData(rowData, votedData).
🧹 Nitpick comments (2)
functions/poll_service_java/ResponseData.java (2)
20-41: Constructor change removes 'edited' from API payload; verify downstream impact.Dropping the edited flag changes the response schema. Ensure UI/consumers don’t rely on it (e.g., to show edit controls for “My Polls”).
If needed for a smoother migration, add a temporary overload delegating to this constructor:
+ ResponseData(ZCRowObject zcRowObject, HashMap<String, Object> votedData, Boolean edited) throws Exception { + this(zcRowObject, votedData); // edited intentionally ignored after deprecation + }
82-105: Minor: method name typo and resilience.Consider renaming getMaxximumPolled to getMaximumPolled and handling empty results from both queries defensively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
functions/poll_service_java/ResponseData.java(1 hunks)functions/poll_service_java/Service.java(1 hunks)
🔇 Additional comments (1)
functions/poll_service_java/Service.java (1)
72-72: Route rename may break clients; support both paths during transition.Changing '/MyPolls' to '/MyPoll' is a breaking API change. Either keep both temporarily or provide a redirect/deprecation period.
Apply this to preserve backward compatibility:
- } else if (url.equals("/MyPoll") && method.equals(POST)) { + } else if ((url.equals("/MyPolls") || url.equals("/MyPoll")) && method.equals(POST)) {Please confirm all client apps and docs are updated if you intend to fully deprecate '/MyPolls'.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/review |
| responseData.remove("pollMap"); | ||
|
|
||
| } else if (url.equals("/MyPolls") && method.equals(POST)) { | ||
| } else if (url.equals("/MyPoll") && method.equals(POST)) { |
There was a problem hiding this comment.
[VALIDATION] Validate that the request parameters (e.g., 'user_id' and 'page') exist and are non-null before calling toString() or parsing them to prevent potential NullPointerExceptions.
String user_id = requestParameters.get("user_id") != null ? requestParameters.get("user_id").toString() : null;
if (user_id == null) {
// handle missing user_id, e.g., throw exception or return error response
}
Object pageObj = requestParameters.get("page");
Integer page = null;
if (pageObj != null) {
page = Integer.parseInt(pageObj.toString());
} else {
// handle missing page, e.g., throw exception or return error response
}
|
Reviewed up to commit:81bc3b0434f206a13f81d41ca2ce2dbc71f10bda Additional SuggestionOthers- Multiple branches contain duplicated code for reading the request body and parsing JSON. Consider extracting this logic into a helper method to reduce duplication and improve maintainability.private JSONObject parseRequestBody(HttpServletRequest request) throws IOException, ParseException {
ServletInputStream requestBody = request.getInputStream();
JSONParser jsonParser = new JSONParser();
return (JSONObject) jsonParser.parse(new InputStreamReader(requestBody, "UTF-8"));
}
// Usage in each branch:
JSONObject requestParameters = parseRequestBody(request); |
|
/review force |
| responseData.remove("pollMap"); | ||
|
|
||
| } else if (url.equals("/MyPolls") && method.equals(POST)) { | ||
| } else if (url.equals("/MyPoll") && method.equals(POST)) { |
There was a problem hiding this comment.
[REFACTORING] The endpoint URL has been changed from '/MyPolls' to '/MyPoll' (line 72). Ensure that this update is consistently reflected in client integrations and documentation.
|
Reviewed up to commit:81bc3b0434f206a13f81d41ca2ce2dbc71f10bda |
2 participants
Summary by CodeRabbit