Skip to content
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.

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

move logic to action file #14

Merged
merged 2 commits into from
Feb 7, 2024
Merged

move logic to action file #14

merged 2 commits into from
Feb 7, 2024

Conversation

JisanAR03
Copy link
Contributor

@JisanAR03 JisanAR03 commented Feb 6, 2024

work : before run the assign function, we just check the comment body and behalf of this, run the assign function , so it's fixed issue #10 and #11

Note: To apply these changes in BLT, we have to update the workflow file in BLT, and for this, please approve PR: OWASP-BLT/BLT#1753

Copy link
Contributor

@fredfalcon fredfalcon left a comment

Choose a reason for hiding this comment

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

I would like to see some kind of automated tests

@OWASP-BLT OWASP-BLT deleted a comment from JisanAR03 Feb 7, 2024
@DonnieBLT
Copy link
Collaborator

@JisanAR03 can you use this project as the test project? So it is self contained?

@JisanAR03
Copy link
Contributor Author

@JisanAR03 can you use this project as the test project? So it is self contained?
@DonnieBLT sir, sorry for asking again. Can you explain a little bit more detailly?

@DonnieBLT
Copy link
Collaborator

@JisanAR03 you mentioned having a test project so can you use this repository to have the test project? Ideally we can have a github action in this repository that can test creating an issue adding "/ assign" to it and see if it gets assigned to the user? or if there was a way to make the code more modular and test individual functions that might work too. My main concern is that we have some automated test to make sure it always works.

@JisanAR03
Copy link
Contributor Author

@JisanAR03 you mentioned having a test project so can you use this repository to have the test project? Ideally we can have a github action in this repository that can test creating an issue adding "/ assign" to it and see if it gets assigned to the user? or if there was a way to make the code more modular and test individual functions that might work too. My main concern is that we have some automated test to make sure it always works.

@DonnieBLT sir, then I think I should update this PR with the workflow file. and then you can check here. Is it ok, sir?

@JisanAR03
Copy link
Contributor Author

JisanAR03 commented Feb 7, 2024

@DonnieBLT , actually, sir I only added few lines of code but for the "if (issue && proceedWithIssueProcessing) {" line, github shows I edited all of the code 🙂

I just added the below code :
``if (eventName === 'issue_comment' && issue && comment) {

    console.log('processing issue comment');
    const commentBody = comment.body.toLowerCase(); // Convert to lower case for case-insensitive comparison
    const assignKeywords = ['/assign', 'assign to me', 'assign this to me', 'please assign me this', 'i can try fixing this', 'i am interested in doing this', 'i am interested in contributing'];
    const unassignKeywords = ['/unassign'];

    const shouldAssign = assignKeywords.some(keyword => commentBody.includes(keyword));
    const shouldUnassign = unassignKeywords.some(keyword => commentBody.startsWith(keyword));

    if (shouldAssign) {
        proceedWithIssueProcessing = true; // Set flag to true
    } else if (shouldUnassign) {
        console.log(`Unassigning issue #${issue.number} from ${comment.user.login}`);
        await octokit.issues.removeAssignees({
            owner,
            repo,
            issue_number: issue.number,
            assignees: [comment.user.login]
        });
    }
}``

@DonnieBLT
Copy link
Collaborator

ok let's go with it, thanks. hopefully everything continues to work. Can you please test after it's merged?

@DonnieBLT DonnieBLT merged commit 00e94ab into OWASP-BLT:main Feb 7, 2024
@JisanAR03
Copy link
Contributor Author

@DonnieBLT , thank you so much sir ☺

@JisanAR03
Copy link
Contributor Author

ok let's go with it, thanks. hopefully everything continues to work. Can you please test after it's merged?

yeah sir sure .. I will test this after merge the "BLT" pr

@JisanAR03
Copy link
Contributor Author

screenshot after test:
image
image
image

looks fine sir ☺

@DonnieBLT
Copy link
Collaborator

awesome, nice work, thanks!

@DonnieBLT
Copy link
Collaborator

Can you please also update the readme

@JisanAR03
Copy link
Contributor Author

Can you please also update the readme

yeah sir sure , I will make one more PR after update the Readme

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants