fixed unassign issues

[SukhvirKooner]

Summary

Fixes #12837

All the reported issues have now been resolved:

✅ 1. Unexpected Unassignments

• The action now retains assignments for linked PRs, regardless of variations in the link format.
• The action also properly distinguishes between issues and pull requests, ensuring it doesn't act on PRs.

✅ 2. Organization Member Unassignment

• The action now reliably differentiates external accounts from organization members, ensuring organization members are never unintentionally unassigned.

✅ 3. Slack Notification Format Improvement

• The Slack notification format has been optimized to minimize space usage and improve readability.

Below are some Screenshots of the Testing i have done:

…

References

…

Reviewer guidance

…

[MisRob]

Hi @SukhvirKooner, thank you for these updates. I have just finished testing another action and will move toward reviewing and re-testing this one.

[MisRob]

Hi @SukhvirKooner, thank you for these updates. It's shaping up nicely. I finished code review and left a few clarification questions and possibly requests. Let me know your thoughts. After we wrap up code review, I will merge and re-test.

[MisRob]

Hi @SukhvirKooner thanks a lot for all the clarifications. I will get back to this next week to read everything in detail and we can then decide on next steps.

[MisRob]

Hi @SukhvirKooner, I left few last notes. After addressing them, would you be able to test as many scenarios as possible one more time in your environment? And if it works, we can merge, and I will give it another try in our repos. Thanks a lot for patient work in the area that has many nuances to address, seems we're getting close.

[MisRob]

@SukhvirKooner oh and it seems there's a conflict, could you merge main and resolve it?

[SukhvirKooner]

Hi @SukhvirKooner, I left few last notes. After addressing them, would you be able to test as many scenarios as possible one more time in your environment? And if it works, we can merge, and I will give it another try in our repos. Thanks a lot for patient work in the area that has many nuances to address, seems we're getting close.

Sounds good! I'll address the notes and run a thorough round of testing. Thanks for the feedback!

[SukhvirKooner]

Hi @MisRob,
I’ve added the regex in Method 2 and tested it. Also, I’ve removed Method 3 along with some unnecessary comments. I’m almost done testing all the scenarios.

If you have any additional scenarios or edge cases that you think should be covered, please let me know.

[MisRob]

Thanks @SukhvirKooner, nothing comes to my mind, except all the situations we've talked about. Let me know when you think this is ready.

[SukhvirKooner]

Hi @MisRob ,
I've tested all the scenarios we discussed. Everything looks good on my end, and you can go ahead and merge this.

[MisRob]

Perfect @SukhvirKooner, thank you. The newest commits make sense to me. I will merge now and re-test. As you may have noticed it may take a week or two, but I will follow-up and let you know how things look like.

[MisRob]

Hi @SukhvirKooner, I did first re-testing and we still have some trouble with recognizing core team members. On these two issues, both MisRob and MisRob2 got unassigned, whereas only MisRob2 should be unassigned. MisRob is core team account.

• Testing issue 1 test-actions#49 (comment)
• Testing issue 2 test-actions#63 (comment)

Would taking inspiration from this action be helpful? I tested it some time ago and it worked well in this regard. It uses this endpoint https://docs.github.com/en/rest/collaborators/collaborators

[SukhvirKooner]

Hi @MisRob,

Thanks for the detailed update! I had tested it earlier and just did a retest on my end—it’s working fine for me. I’ve attached screenshots below for reference.

• Testing issue

That said, I’ll definitely look into this further to see if there’s anything we might be missing in your case.

[MisRob]

Hi @SukhvirKooner, yes I believe you tested it all, and thanks for double-checking now. I just think there's differences between your test environment and ours that we will need to fine-tune. Few things we can chat about:

• Do you know what's the difference between the methods you use and the collaborators API?
• Is there some additional logging we can add so I can provide you more information?
• Could there be some token permissions missing on our end?

[MisRob]

@SukhvirKooner I found the log of one of those troublesome unassignments

Notice the problematic "MisRob is not an organization member".

Also I am seeing a deprecation notice that would be good to deal with if we can, but that's lesser priority.

[MisRob]

@SukhvirKooner let me know any ideas about how I can assist you here.

Other than that I haven't noticed any problems, and Slack works too! So hopefully this is a last hiccup.

[SukhvirKooner]

1. Difference Between Methods:

   • Current Method:
     Uses the organization membership check (and repository ownership) to determine if a user is "internal".
     This method may miss external collaborators who have been added as collaborators but are not members of the organization.

   • Collaborators API Method:
     Directly queries the /repos/{owner}/{repo}/collaborators/{username} endpoint.
     A 204 status indicates the user is a collaborator ("internal"), while a 404 indicates they are not (i.e., "external").

   ==> As a member of the core team (internal collaborator), you should not have been unassigned.

2. Additional Logging Suggestions (to add in your code):
   To help diagnose the issue, consider adding logs like these:

const checkUserMembership = async (owner, repo, username, github) => {
if (!username) {
  console.error("Invalid username provided to checkUserMembership");
  return false;
}

try {
  console.log(`Fetching repository details for ${owner}/${repo} to check if ${username} is the owner...`);
  const repoDetails = await github.rest.repos.get({
    owner,
    repo
  });
  console.log(`Repository owner is ${repoDetails.data.owner.login}; comparing with ${username}`);
  
  if (repoDetails.data.owner.login === username) {
    console.log(`${username} is the repository owner.`);
    return true;
  }
  
  console.log(`Checking if ${username} is a member of organization ${owner}...`);
  try {
    const membershipResponse = await github.rest.orgs.getMembershipForUser({
      org: owner,
      username: username
    });
    console.log(`${username} is confirmed as an organization member. Membership response:`, membershipResponse.data);
    return true;
  } catch (orgError) {
    console.error(`${username} is not an organization member. Org membership error:`, orgError.response?.status, orgError.response?.data);
    return false;
  }
} catch (error) {
  console.error(`Error checking user membership for ${username}:`, error.response?.status, error.response?.data);
  return false;
}
};

In this updated version, I’ve added logging for:

• The repository details retrieval:
• To verify the owner and check that the correct username is being compared.

The organization membership check:
To log the membership response if the check is successful, and to log detailed error information if the check fails.

3. Necessary Token Permissions:
   • "repo" scope: Needed for accessing repository details (including collaborators) and updating issues.
   • "read:org" scope: Required for checking organization membership using endpoints like orgs.getMembershipForUser.

Ensure your token in all environments has these permissions to allow the API calls to work correctly.

[SukhvirKooner]

@MisRob Please let me know if you'd like to implement the membership check method that you suggested. I can make those changes and raise a PR. Additionally, I'll look into the deprecation issue.

[MisRob]

Thanks @SukhvirKooner, this is great. No you don't need to change that implementation yet, but feel free to add this logging, or I can do it some time later. Then we will know more and can decide what to do next.

I think those permissions should be fine but let me double check meanwhile.

[MisRob]

Okay @SukhvirKooner so it really was the permission. It works now as expected learningequality/test-actions#49 (comment). Apologies for the hassle, it caught me by surprise because the other action for detecting membership doesn't need so many permissions like this one. So no changes needed in this area. Let me know how things look like with the deprecation issue and I can check then one last time.

[SukhvirKooner]

Hey @MisRob,

I've fixed the deprecation issue. Should I open a new issue first and then submit a PR?

[MisRob]

Hi @SukhvirKooner, that's perfect, thank you. No need for issue since we confirmed together - you're welcome to submit a PR.

[SukhvirKooner]

Hi @MisRob ,
I’ve opened a PR #20 for this! I’ve tested the action after making the changes, and it’s working as expected. Looking forward to getting it merged.