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

hotfix: check permission when closing issue #622

Merged
merged 7 commits into from Aug 17, 2023

Conversation

wannacfuture
Copy link
Contributor

@netlify
Copy link

netlify bot commented Aug 16, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit a8615c3
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/64de0efbce819a000888eac8
😎 Deploy Preview https://deploy-preview-622--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

0x4007
0x4007 previously approved these changes Aug 16, 2023
@0x4007 0x4007 enabled auto-merge August 16, 2023 14:31
@0x4007
Copy link
Member

0x4007 commented Aug 16, 2023

It's really annoying that GitHub won't let me merge because I am the last person to commit even as an admin lol @wannacfuture can you approve my commit so we can merge?

@wannacfuture
Copy link
Contributor Author

wannacfuture commented Aug 16, 2023

haha also Github is preventing approval from the pr's author about any commits on it. 😂
Guess we need other one. lol

src/helpers/issue.ts Outdated Show resolved Hide resolved
src/helpers/issue.ts Show resolved Hide resolved
src/helpers/issue.ts Outdated Show resolved Hide resolved
src/helpers/issue.ts Outdated Show resolved Hide resolved
Co-authored-by: whilefoo <139262667+whilefoo@users.noreply.github.com>
Co-authored-by: whilefoo <139262667+whilefoo@users.noreply.github.com>
src/helpers/issue.ts Outdated Show resolved Hide resolved
@wannacfuture
Copy link
Contributor Author

wannacfuture commented Aug 17, 2023

Ideally, we should check the response status and return true if status is 204.

@0xcodercrane , @whilefoo
Yeah, but currently octokit has a problem with checkMembershipForUser function.
repos.checkCollaborator returns 204 status and it works well.
But talking about checkMembershipForUser , It occurs type error when I try to check the status === 204.
and I guess octokit is in progress on fixing this issue.
octokit/rest.js#188
image

Or... should I use axios call instead of octokit?

@0x4007
Copy link
Member

0x4007 commented Aug 17, 2023

My intuition would say to typecast it. Otherwise we might need to axios it and heavily comment/explain the situation in the code which seems messy

@whilefoo
Copy link
Collaborator

Maybe you can make a comment like in octokit issue:

// @ts-expect-error This looks like a bug in octokit. (https://github.com/octokit/rest.js/issues/188)
return res.status === 204

@wannacfuture
Copy link
Contributor Author

done. ready for review.

src/helpers/issue.ts Outdated Show resolved Hide resolved
Co-authored-by: whilefoo <139262667+whilefoo@users.noreply.github.com>
@0x4007 0x4007 added this pull request to the merge queue Aug 17, 2023
@0xcodercrane 0xcodercrane removed this pull request from the merge queue due to a manual request Aug 17, 2023
@0xcodercrane 0xcodercrane merged commit 59825e4 into ubiquity:development Aug 17, 2023
9 checks passed
@@ -33,6 +34,10 @@ export const handleIssueClosed = async () => {

if (!issue) return;

const userHasPermission = await checkUserPermissionForRepoAndOrg(payload.sender.login, context);
Copy link
Member

@0x4007 0x4007 Aug 17, 2023

Choose a reason for hiding this comment

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

I found an issue testing it out.

This is not implemented correctly because I'm an admin of the ubiquibot organization @wannacfuture please review

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.

None yet

4 participants