feat/CUS-10637-Used consolidated plan counts in test plan results email#344
Conversation
📝 WalkthroughWalkthroughVersion bump and refactoring of test plan results email addon to consolidate count extraction logic into a helper method while updating API endpoints from standard to in-region domains. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
send_testplan_results_to_email/src/main/java/com/testsigma/addons/hook/SendTestPlanResultsToEmail.java (2)
47-48:⚠️ Potential issue | 🔴 CriticalHardcoded SMTP App Password will be exposed when addon is published publicly.
SMTP_PASSWORDcontains what appears to be a Google App Password hardcoded in source. The PR objective explicitly states this addon will be "published as public (IN Region)." Publishing a JAR or source with a hardcoded credential exposes the sending account to abuse, allows third parties to send mail asent_support@testsigma.com, and the secret cannot be rotated without a code change and redeployment.The credential should be moved to a
@TestData-annotated field (likeapiKey), injected at runtime, and the plaintext constant removed entirely.🔒 Proposed fix: convert to runtime-injected TestData
- private static final String SMTP_PASSWORD = "asla sdqa frwa uynz"; + `@TestData`(reference = "{SMTP PASSWORD}") + private com.testsigma.sdk.TestData smtpPassword;Then in
execute():- String smtpPass = SMTP_PASSWORD != null ? SMTP_PASSWORD.trim().replaceAll("\\s+", "") : ""; + String smtpPass = smtpPassword != null && smtpPassword.getValue() != null + ? smtpPassword.getValue().toString().trim().replaceAll("\\s+", "") : "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@send_testplan_results_to_email/src/main/java/com/testsigma/addons/hook/SendTestPlanResultsToEmail.java` around lines 47 - 48, Replace the hardcoded SMTP_PASSWORD constant in SendTestPlanResultsToEmail with a runtime-injected TestData field: remove the SMTP_PASSWORD static constant and add a non-static field annotated with `@TestData` (e.g. apiKey or smtpPassword) alongside or replacing SMTP_USER as needed; update the execute() method to read the injected TestData value at runtime and use it for SMTP authentication, validating null/empty and failing gracefully with a clear error if not provided, and ensure no plaintext secret remains in source or constants.
349-352:⚠️ Potential issue | 🟡 MinorStale Javadoc still references old field names.
The comment still lists
totalCount, passedCount, failedCountas the extracted fields, but after the refactor the code readsconsolidatedPlanTotalCount,consolidatedPlanPassedCount,consolidatedPlanFailedCount, etc. viagetCountsFromTestPlanResultMetric.📝 Proposed fix
- * Fetches test_case_results with executionResultId = lastRunId (one page), extracts Total, Passed, Failure, etc. - * from content[].environmentResult.executionResult.testPlanResultMetric (totalCount, passedCount, failedCount, ...). + * Fetches test_case_results with executionResultId = lastRunId (one page), extracts consolidated plan counts + * from content[].environmentResult.executionResult.testPlanResultMetric + * (consolidatedPlanTotalCount, consolidatedPlanPassedCount, consolidatedPlanFailedCount, ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@send_testplan_results_to_email/src/main/java/com/testsigma/addons/hook/SendTestPlanResultsToEmail.java` around lines 349 - 352, The Javadoc above the method that fetches test_case_results (referencing executionResultId = lastRunId and environmentResult.executionResult.testPlanResultMetric) is stale: update the comment in SendTestPlanResultsToEmail to list the current metric fields returned/used by getCountsFromTestPlanResultMetric (e.g., consolidatedPlanTotalCount, consolidatedPlanPassedCount, consolidatedPlanFailedCount, etc.) instead of the old totalCount/passedCount/failedCount names and ensure the description matches the actual extraction logic in that method.
🧹 Nitpick comments (1)
send_testplan_results_to_email/pom.xml (1)
9-9: Version skips 1.0.1.The version jumped from 1.0.0 directly to 1.0.2. If 1.0.1 was published for another region/environment, this is fine — otherwise consider whether 1.0.1 should be accounted for or the jump is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@send_testplan_results_to_email/pom.xml` at line 9, The project POM version tag currently jumps from 1.0.0 to <version>1.0.2</version>; confirm whether 1.0.1 was intentionally skipped or missing and either set the <version> element to the correct next version (e.g., 1.0.1) or document/justify the skip (add a CHANGELOG entry or release note) so the version history is consistent; update the <version> element in the pom.xml (or add the explanatory release note) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@send_testplan_results_to_email/src/main/java/com/testsigma/addons/hook/SendTestPlanResultsToEmail.java`:
- Around line 259-270: getCountsFromTestPlanResultMetric currently builds a map
of zeros when metric exists but contains none of the consolidatedPlan* keys,
which prevents the fallback chain in execute() from running; update
getCountsFromTestPlanResultMetric to first detect presence of any
consolidatedPlan* fields (e.g.,
"consolidatedPlanTotalCount","consolidatedPlanPassedCount","consolidatedPlanFailedCount","consolidatedPlanStoppedCount","consolidatedPlanNotExecutedCount","consolidatedPlanQueuedCount","consolidatedPlanRunningCount")
and return null if none are present, otherwise build and return the
LinkedHashMap as before; ensure callers getCountsFromRunDetails and
getPlanCountsFromTestCaseResultContent continue to rely on null to trigger
fallback paths referenced in execute().
---
Outside diff comments:
In
`@send_testplan_results_to_email/src/main/java/com/testsigma/addons/hook/SendTestPlanResultsToEmail.java`:
- Around line 47-48: Replace the hardcoded SMTP_PASSWORD constant in
SendTestPlanResultsToEmail with a runtime-injected TestData field: remove the
SMTP_PASSWORD static constant and add a non-static field annotated with
`@TestData` (e.g. apiKey or smtpPassword) alongside or replacing SMTP_USER as
needed; update the execute() method to read the injected TestData value at
runtime and use it for SMTP authentication, validating null/empty and failing
gracefully with a clear error if not provided, and ensure no plaintext secret
remains in source or constants.
- Around line 349-352: The Javadoc above the method that fetches
test_case_results (referencing executionResultId = lastRunId and
environmentResult.executionResult.testPlanResultMetric) is stale: update the
comment in SendTestPlanResultsToEmail to list the current metric fields
returned/used by getCountsFromTestPlanResultMetric (e.g.,
consolidatedPlanTotalCount, consolidatedPlanPassedCount,
consolidatedPlanFailedCount, etc.) instead of the old
totalCount/passedCount/failedCount names and ensure the description matches the
actual extraction logic in that method.
---
Nitpick comments:
In `@send_testplan_results_to_email/pom.xml`:
- Line 9: The project POM version tag currently jumps from 1.0.0 to
<version>1.0.2</version>; confirm whether 1.0.1 was intentionally skipped or
missing and either set the <version> element to the correct next version (e.g.,
1.0.1) or document/justify the skip (add a CHANGELOG entry or release note) so
the version history is consistent; update the <version> element in the pom.xml
(or add the explanatory release note) accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
send_testplan_results_to_email/pom.xmlsend_testplan_results_to_email/src/main/java/com/testsigma/addons/hook/SendTestPlanResultsToEmail.java
| private Map<String, Integer> getCountsFromTestPlanResultMetric(JsonObject metric) { | ||
| if (metric == null) return null; | ||
| Map<String, Integer> counts = new LinkedHashMap<>(); | ||
| counts.put("total", getInt(metric, "totalCount", null, 0)); | ||
| counts.put("passed", getInt(metric, "passedCount", null, 0)); | ||
| counts.put("failure", getInt(metric, "failedCount", null, 0)); | ||
| counts.put("stopped", getInt(metric, "stoppedCount", null, 0)); | ||
| counts.put("not_executed", getInt(metric, "notExecutedCount", null, 0)); | ||
| counts.put("queued", getInt(metric, "queuedCount", null, 0)); | ||
| counts.put("running", getInt(metric, "runningCount", null, 0)); | ||
| counts.put("total", getInt(metric, "consolidatedPlanTotalCount", null, 0)); | ||
| counts.put("passed", getInt(metric, "consolidatedPlanPassedCount", null, 0)); | ||
| counts.put("failure", getInt(metric, "consolidatedPlanFailedCount", null, 0)); | ||
| counts.put("stopped", getInt(metric, "consolidatedPlanStoppedCount", null, 0)); | ||
| counts.put("not_executed", getInt(metric, "consolidatedPlanNotExecutedCount", null, 0)); | ||
| counts.put("queued", getInt(metric, "consolidatedPlanQueuedCount", null, 0)); | ||
| counts.put("running", getInt(metric, "consolidatedPlanRunningCount", null, 0)); | ||
| return counts; | ||
| } |
There was a problem hiding this comment.
getCountsFromTestPlanResultMetric returns an all-zeros map instead of null when consolidated fields are absent — this silently breaks the fallback chain.
When metric is non-null but lacks consolidatedPlan* keys (e.g., older API response format), every getInt call falls through to its defaultValue of 0. The method returns a non-null map of all zeros. The fallback chain in execute() (lines 104–111) only activates when counts == null, so all three fallback paths (fetchPlanCountsFromTestCaseResultsByExecutionResultId, resultsData.planCountsFromApi, defaultCounts()) are bypassed. The resulting email silently reports zeros for every metric.
Add a presence guard before building the map:
🐛 Proposed fix
private Map<String, Integer> getCountsFromTestPlanResultMetric(JsonObject metric) {
if (metric == null) return null;
+ // Return null so callers can fall back when consolidated fields are absent (e.g., older API format).
+ if (!metric.has("consolidatedPlanTotalCount")
+ && !metric.has("consolidatedPlanPassedCount")
+ && !metric.has("consolidatedPlanFailedCount")) {
+ return null;
+ }
Map<String, Integer> counts = new LinkedHashMap<>();
counts.put("total", getInt(metric, "consolidatedPlanTotalCount", null, 0));
counts.put("passed", getInt(metric, "consolidatedPlanPassedCount", null, 0));
counts.put("failure", getInt(metric, "consolidatedPlanFailedCount", null, 0));
counts.put("stopped", getInt(metric, "consolidatedPlanStoppedCount", null, 0));
counts.put("not_executed", getInt(metric, "consolidatedPlanNotExecutedCount", null, 0));
counts.put("queued", getInt(metric, "consolidatedPlanQueuedCount", null, 0));
counts.put("running", getInt(metric, "consolidatedPlanRunningCount", null, 0));
return counts;
}This fix also benefits getCountsFromRunDetails (Line 423) and getPlanCountsFromTestCaseResultContent (Line 253), which both delegate here and rely on a null return to signal "not available."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@send_testplan_results_to_email/src/main/java/com/testsigma/addons/hook/SendTestPlanResultsToEmail.java`
around lines 259 - 270, getCountsFromTestPlanResultMetric currently builds a map
of zeros when metric exists but contains none of the consolidatedPlan* keys,
which prevents the fallback chain in execute() from running; update
getCountsFromTestPlanResultMetric to first detect presence of any
consolidatedPlan* fields (e.g.,
"consolidatedPlanTotalCount","consolidatedPlanPassedCount","consolidatedPlanFailedCount","consolidatedPlanStoppedCount","consolidatedPlanNotExecutedCount","consolidatedPlanQueuedCount","consolidatedPlanRunningCount")
and return null if none are present, otherwise build and return the
LinkedHashMap as before; ensure callers getCountsFromRunDetails and
getPlanCountsFromTestCaseResultContent continue to rely on null to trigger
fallback paths referenced in execute().
2 participants
Publish this addon as public (IN Region)
Addon Name: Send TestPlan Results To Email
Jarvis Link: https://jarvis-in.testsigma.com/ui/tenants/3/addons
Jira : https://testsigma.atlassian.net/browse/CUS-10637
Used consolidated plan counts in test plan results email
Summary by CodeRabbit