User feedback survey workflow#5658
Conversation
|
Hi @sasha-gitg , can you please review this. |
sasha-gitg
left a comment
There was a problem hiding this comment.
I have performed a thorough, line-by-line code review of the changes in this PR, and successfully verified the workflow logic and ADK API compatibility using local testing and mock execution.
Here is a summary of the findings, ordered by severity.
Major Findings
1. Duplicate Comments Posted When Multiple Labels Match
- File:
.github/scripts/csat.js - Severity: Major (must fix)
- Description:
The script loops overcontext.payload.issue.labelsand evaluates the matching logic inside the loop. If an issue is closed with multiple matching labels (e.g.['bug', 'core', 'mcp']), the script will post the CSAT comment multiple times (once for each matching label). This creates unnecessary duplicate comment noise on issues. - Proposed Fix:
UseArray.prototype.some()to check if any label matches, and perform the comment posting logic exactly once outside the loop.
2. Case Sensitivity in Label Matching
- File:
.github/scripts/csat.js - Severity: Major (should fix)
- Description:
The matching check useslabel.name.includes(...). Since the constants inconstant.jsare lowercase, any capitalized or custom-cased label (e.g.,Bug,CORE) will fail to match becauseincludes()is case-sensitive in JavaScript. - Proposed Fix:
Convertlabel.nameto lowercase first using.toLowerCase()before performing the matching.
Minor Findings & Cleanups
3. Redundant Workflow Permissions
- File:
.github/workflows/csat.yml - Severity: Minor (nice to fix)
- Description:
The workflow only triggers onissues: closed. It does not run on pull requests. The permissionpull-requests: writeis redundant and should be removed to strictly adhere to the principle of least privilege.
4. Typos in Comments & Missing Trailing Newlines
- File:
.github/scripts/csat.jsand.github/scripts/constant.js - Severity: Suggestion
- Description:
- Line 19 of
csat.js: Mention ofstale_csat.jsandcsat.yamlis outdated (the workflow file iscsat.yml, andstale_csat.jsis not part of this PR). - Line 28 of
csat.js: Typothsinstead ofthe. .github/scripts/constant.js,.github/scripts/csat.js, and.github/workflows/csat.ymlall have\ No newline at end of file. Please add a trailing newline to satisfy standard style checkers.
- Line 19 of
Proposed Code Refactor for csat.js
Here is a complete, refactored, and fully-tested replacement for csat.js that is cleaner, case-insensitive, DRY, and entirely avoids the duplicate comment bug:
const CONSTANT_VALUES = require('./constant');
/**
* Invoked from csat.yml workflow file to post survey link
* in closed issues.
* @param {!Object.<string,!Object>} github contains pre defined functions.
* context Information about the workflow run.
* @return {null}
*/
module.exports = async ({ github, context }) => {
const issue = context.payload.issue.html_url;
// Check if any label matches (case-insensitive) the supported CSAT labels.
const supportedLabels = Object.values(CONSTANT_VALUES.GLOBALS.LABELS);
const hasMatchingLabel = context.payload.issue.labels.some(label => {
const name = label.name.toLowerCase();
return supportedLabels.some(supportedLabel => name.includes(supportedLabel));
});
if (hasMatchingLabel) {
console.log(`Posting CSAT survey for issue =${issue}`);
const baseUrl = CONSTANT_VALUES.MODULE.CSAT.BASE_URL;
const yesCsat = `<a href="${baseUrl + CONSTANT_VALUES.MODULE.CSAT.SATISFACTION_PARAM +
CONSTANT_VALUES.MODULE.CSAT.YES +
CONSTANT_VALUES.MODULE.CSAT.ISSUEID_PARAM + encodeURIComponent(issue)}"> ${CONSTANT_VALUES.MODULE.CSAT.YES}</a>`;
const noCsat = `<a href="${baseUrl + CONSTANT_VALUES.MODULE.CSAT.SATISFACTION_PARAM +
CONSTANT_VALUES.MODULE.CSAT.NO +
CONSTANT_VALUES.MODULE.CSAT.ISSUEID_PARAM + encodeURIComponent(issue)}"> ${CONSTANT_VALUES.MODULE.CSAT.NO}</a>`;
const comment = CONSTANT_VALUES.MODULE.CSAT.MSG + '\n' + yesCsat + '\n' +
noCsat + '\n';
const issueNumber = context.issue.number ?? context.payload.issue.number;
await github.rest.issues.createComment({
issue_number: issueNumber,
owner: context.repo.owner,
repo: context.repo.repo,
body: comment
});
}
};|
Hi @rshashank17 , can you please address the review comments. |
|
Hi @rohityan and @sasha-gitg , I’ve addressed all the review comments, including the duplicate comment logic and case sensitivity fixes. The latest commits are ready for a final look! |
b2ecca7 to
cbd7885
Compare
|
Response from ADK Triaging Agent Hello @rshashank17, thank you for creating this PR! It looks like the Thank you! |
cbd7885 to
399c3e8
Compare
Problem
The repository currently lacks an automated mechanism to collect user feedbackwhen issues are resolved and closed.
Solution
This PR implements a user feedback survey workflow for the ADK Python repository. When an issue is closed, a GitHub Actions workflow is triggered to post a comment with a survey link if the issue has specific relevant labels.
Changes
.github/workflows/csat.yml: Workflow definition file that triggers onissues: closedand runs the script..github/scripts/csat.js: JavaScript script that checks issue labels and posts the survey feedback comment with generated links..github/scripts/constant.js: Constants file containing labels, base URL, and message strings for the survey.Labels Checked
The script will post the survey if the closed issue contains any of the following labels:
bugcoretoolsservicesmodelsmcpauthlivedocumentationgood first issueagent enginebqevaltracingwebworkflowTesting Plan
Manual End-to-End (E2E) Tests:
Checklist
Additional context
The survey links to a Google Form with pre-filled parameters for the issue URL and satisfaction level (Yes/No).