Skip to content

Fix: Remove incorrect 'Closed' label for merged PRs in reviewed section #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix: Remove incorrect 'Closed' label for merged PRs in reviewed section #177

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions src/scripts/scrumHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,21 +972,34 @@ ${userReason}`;
});
}
li += `</li>`;
} else if (item.state === 'closed') {
let merged = null;
if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
let repoParts = repository_url.split('/');
let owner = repoParts[repoParts.length - 2];
let repo = repoParts[repoParts.length - 1];
merged = mergedStatusResults[`${owner}/${repo}#${number}`];
}
if (merged === true) {
li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_merged_button}</li>`;
} else {
// Always show closed label for merged === false or merged === null/undefined
li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_closed_button}</li>`;
}
}
}//// Replace the existing logic around line where item.state === 'closed' is handled

} else if (item.state === 'closed') {
let merged = null;
if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
let repoParts = repository_url.split('/');
let owner = repoParts[repoParts.length - 2];
Comment on lines +977 to +981
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Potential edge case if repository_url is malformed.

Accessing repository_url by index may fail if its structure is inconsistent. Please add validation to ensure repository_url is well-formed before splitting and accessing its parts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you resend the suggestion to merge this pr beacuse me bymistakely clicked resolve option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'm generating a new review now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @manhuu14, I've posted a new review for you!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else if (item.state === 'closed') {
const merged = getMergedStatus(item, repository_url, item.number, githubToken, useMergedStatus, fallbackToSimple, mergedStatusResults);

if (merged === true) {
    // Confirmed merged
    li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_merged_button}</li>`;
} else if (merged === false) {
    // Closed but not merged
    li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_closed_button}</li>`;
} else {
    // Unknown merge status: show no state label to avoid misleading users
    li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a></li>`;
}

}
and function getMergedStatus(item, repository_url, prNumber, githubToken, useMergedStatus, fallbackToSimple, mergedStatusResults) {
let merged = null;

if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
    try {
        const parsedUrl = new URL(repository_url);
        const pathParts = parsedUrl.pathname.split('/').filter(Boolean); // remove empty strings

        if (pathParts.length >= 2) {
            const owner = pathParts[pathParts.length - 2];
            const repo = pathParts[pathParts.length - 1];
            const prKey = `${owner}/${repo}#${prNumber}`;

            if (mergedStatusResults.hasOwnProperty(prKey)) {
                merged = mergedStatusResults[prKey]; // true, false, or undefined
            }
        } else {
            console.warn('[scrum_helper] URL path too short to parse owner/repo:', parsedUrl.pathname);
        }
    } catch (err) {
        console.warn('[scrum_helper] Invalid repository_url:', repository_url, err);
    }
}

return merged;

}
add this and try

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your latest changes and the review discussion, your PR is ready to be merged! You have addressed the feedback by making the merged status logic more robust and handling edge cases for malformed URLs. If your tests are passing and everything works as expected, you can go ahead and merge the PR.

If you need a final approval from a maintainer, just request it�otherwise, you're good to go!

Let me know if you need help with the merge process or anything else.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i made another pr and there is no isuue ai saing all good how to request maintainer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To request a review from a maintainer on your new PR, follow these steps:

  1. Go to your pull request page on GitHub.
  2. On the right sidebar, look for the "Reviewers" section.
  3. Click the gear icon or the "Request" button next to "Reviewers."
  4. Search for the maintainer's GitHub username (or select from the list) and click their name to add them as a reviewer.
  5. They will be notified and can review your PR.

If you don't see the "Reviewers" section, you may not have permission to request reviewers directly. In that case, you can leave a comment on the PR mentioning the maintainer (e.g., @maintainer-username) and politely ask them to review your PR.

Let me know if you need more help!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i leave this one automatically merge or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, your pull request will not merge automatically if you leave it (unless your repository has an auto-merge feature enabled, which is uncommon). Normally, after your PR is approved and all checks pass, someone (either you or a maintainer) needs to manually click the "Merge" button on GitHub to complete the process.

If you want your PR to be merged, make sure to:

  • Get all required approvals
  • Pass all required checks (like CI tests)
  • Then, either merge it yourself (if you have permission) or ask a maintainer to merge it for you

Let me know if you need help with the merge process or have any other questions!

let repo = repoParts[repoParts.length - 1];
merged = mergedStatusResults[`${owner}/${repo}#${number}`];
}

if (merged === true) {
// PR is confirmed merged
li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_merged_button}</li>`;
} else if (merged === false) {
// PR is confirmed closed but not merged
li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_closed_button}</li>`;
} else {
// merged === null/undefined - status unknown
// For reviewed PRs section, we should be more conservative about showing "closed"
// since these aren't the user's PRs. Default to showing no specific state label
// or a neutral label instead of assuming "closed"
li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_open_button}</li>`;

// Alternative approach: Don't show any state label for unknown status
// li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a></li>`;
}

lastWeekArray.push(li);
continue;
} else {
Expand Down