Skip to content

Conversation

@carolynvs
Copy link
Contributor

When someone tries to use /lgtm or /approve, but they are not authorized to perform that action, log the error but also comment back on the pull request so that they know that their slash command didn't work.

Screenshot below shows what the comments look like when you aren't in the OWNERS file, or if there isn't an OWNERS file, for /lgtm and /approve.

Screen Shot 2022-05-14 at 9 10 59 PM

When someone tries to use /lgtm or /approve, but they are not authorized
to perform that action, log the error but also comment back on the pull
request so that they know that their slash command didn't work.

Signed-off-by: Carolyn Van Slyck <[email protected]>
carolynvs added 2 commits May 15, 2022 17:34
When I first wrote these tests, I found it easier to assert that the
handlers threw an error, even though the rest of the tests were doing
the asserts through the mocks.

Now that the failure case works better with that pattern, because we are
calling the comment api when we throw an error, it's easier to use the
same type of tests, calling the issue comment handler instead of lgtm
directly. So I've updated the tests to work like the rest.

Signed-off-by: Carolyn Van Slyck <[email protected]>
Signed-off-by: Carolyn Van Slyck <[email protected]>
Copy link
Owner

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Wow - this is incredible and actually something I've wanted to do for a long time.

I'm away and off work for the next few weeks but just wanted to drop a note that this is so cool, get eyes on it, and I'm planning to take a deep dive on this once I'm back 👏🏼

@carolynvs
Copy link
Contributor Author

Enjoy your break! 🏖️

Copy link
Owner

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Awesome! Looks great!! I think main is pretty close to being able to cut a release; we can get that out soon.

Thank you so much for this contribution!!

@jpmcb jpmcb merged commit 2ac4434 into jpmcb:main Jun 10, 2022
@carolynvs carolynvs deleted the talk-back branch July 7, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants