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

One way to fix bot_not_found due to SlackBot #520

Merged
merged 5 commits into from
Aug 2, 2018

Conversation

Eibwen
Copy link
Contributor

@Eibwen Eibwen commented Aug 1, 2018

fixes #519
Specifically when /remind is setup to trigger a hubot event

Summary

Prevent bot_not_found when looking up SlackBot using bots.info with id:B01 from causing an error that stops execution of listeners

Requirements (place an x in each [ ])

Specifically when /remind is setup to trigger a hubot event
@CLAassistant
Copy link

CLAassistant commented Aug 1, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 1, 2018

Codecov Report

Merging #520 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #520   +/-   ##
=======================================
  Coverage   85.55%   85.55%           
=======================================
  Files           6        6           
  Lines         353      353           
  Branches       79       79           
=======================================
  Hits          302      302           
  Misses         29       29           
  Partials       22       22
Impacted Files Coverage Δ
src/client.coffee 95.09% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c84f7f7...1a726c5. Read the comment docs.

@aoberoi
Copy link
Contributor

aoberoi commented Aug 1, 2018

Thanks for taking the time to offer a solution!

I'll have to confirm this, but I think we know that slackbot always has bot ID B01 and user ID USLACKBOT. If that's true, the solution I'd rather merge would be changing this line to this:

      if event.bot_id?
        fetches.bot = if event.bot_id == "B01" then { user_id: "USLACKBOT" } else @fetchBotUser event.bot_id

Then, when the code reaches this line, the fetch for the user representation would be made and resolved to a real user object (instead of just false).

@Eibwen
Copy link
Contributor Author

Eibwen commented Aug 1, 2018

Ah that is likely a better solution.

I didn't look too closely into how @botUserIdMap worked, but saw that some of the values were just false, so went with that. Another possible option would be to insert a B01 entry into that lookup upon creation or something

It seems like slackbot from the users.list endpoint on a small slack group I'm in at least is currently including "is_bot": false, which is quite surprising to me. So that was also part of why I just went with false (but really didn't put much thought into it :) )

@Eibwen
Copy link
Contributor Author

Eibwen commented Aug 1, 2018

Also happen to know if the CLAassistant is based on username, or commit email, or both? (I was extra lazy and edited each file in the browser, so I'm not even sure what email these commits were created with, but wondering if that is the mis-match) Assuming that is the mismatch, shall I just sign the CLA again with whatever probably the eibwen@github email that these were likely created with?

Or does it just take a while to update?

@aoberoi
Copy link
Contributor

aoberoi commented Aug 1, 2018

Another possible option would be to insert a B01 entry into that lookup upon creation or something

I like that even better! We can just hardcode one entry into the map when its initialized here.

It seems like slackbot from the users.list endpoint on a small slack group I'm in at least is currently including "is_bot": false, which is quite surprising to me.

Slackbot is... a special snowflake ❄️. Ideally, yes, it makes more sense for slackbot to have is_bot: "true", but practically I don't think there should be much of a difference. I don't think its worth patching in this Hubot SDK and then supporting that patch for the conceivable future, unless we can think of a reason this would make a script behave in unintended ways.

@aoberoi
Copy link
Contributor

aoberoi commented Aug 1, 2018

Also happen to know if the CLAassistant is based on username, or commit email, or both?

I believe its just be Github username, or at least that seems to be the main key in the list of signatures I have access to. I could be wrong, or there could be something wrong with CLAassistant. Just to be sure, you clicked the yellow badge above and then signed into Github to agree, right? If you click that badge again, what do you see?

Or does it just take a while to update?

There's a link in the CLAassistant message to recheck. I clicked and nothing changed, so I assume its all caught up.

@Eibwen
Copy link
Contributor Author

Eibwen commented Aug 1, 2018

Ah, I didn't click the badge the first time. Whatever I clicked just took me to the google form without requiring the GitHub App authorization, maybe from https://github.com/slackapi/hubot-slack/blob/master/.github/contributing.md ? Probably the "Details" link in the "Some checks were not successful" section

Idk worked great when I clicked the badge! Did also change the email to the email the commits have, but with the App authorization I imagine it has access to all the emails on my github account

@aoberoi
Copy link
Contributor

aoberoi commented Aug 2, 2018

Whoohoo! This is great. Thank you so much.

@aoberoi aoberoi merged commit 67d82ee into slackapi:master Aug 2, 2018
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.

Scripts triggered by SlackBot's /remind no longer working
3 participants