Load users to robot.brain.users #381

ysakasin · 2016-12-13T16:43:37Z

I've read and understood the Contributing guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
I've written tests to cover the new code and functionality included in this PR.
I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

Store users in the robot.brain.users. PR is written with reference to old code.

Related Issues

Fix #352

Test strategy

~~Not yet.~~

Call userChange and loadUsers, after compare input data with data which in brain.

codecov-io · 2016-12-13T16:46:35Z

Current coverage is 77.59% (diff: 78.94%)

Merging #381 into master will increase coverage by 0.15%

@@             master       #381   diff @@
==========================================
  Files             4          4          
  Lines           164        183    +19   
  Methods           0          0          
  Messages          0          0          
  Branches         37         42     +5   
==========================================
+ Hits            127        142    +15   
- Misses           26         30     +4   
  Partials         11         11

Powered by Codecov. Last update 855b3e0...902b00f

DEGoodmanWilson · 2016-12-13T17:30:00Z

Thanks for this! I'll try to have a look at this more closely on Thursday. In the meantime, one of the acceptance criteria is that unit tests are written to cover the new functionality—can you add those in for us?

ysakasin · 2016-12-13T17:44:00Z

@DEGoodmanWilson

In the meantime, one of the acceptance criteria is that unit tests are written to cover the new functionality—can you add those in for us?

Maybe. I'll try.

ysakasin · 2016-12-16T15:27:22Z

@DEGoodmanWilson

Hi. I write unit tests.
Could you review these codes?

sebastiandero · 2016-12-28T09:14:22Z

@NKMR6194 Hey, can i help in some way? Help with test coverage maybe?

ysakasin · 2016-12-28T10:04:09Z

@sebastiandero Thanks. I have just learned of "coverage" meaning. I try writing more unit test. Could you wait?

sebastiandero · 2016-12-28T10:06:36Z

@NKMR6194 sure can, i have tried to use your fork and it does not seem to load any users into the brain, is there something i am missing?

ysakasin · 2016-12-28T10:20:11Z

@sebastiandero Do you use fix-users-in-brain branch? Hmm... I'll check code once more.

sebastiandero · 2016-12-28T10:44:14Z

@NKMR6194 yes I have, dont know what could have been the issue, i have setup a fork to help you with the test coverage

ysakasin · 2016-12-28T11:58:32Z

@sebastiandero How did you check users in brain? Users are loaded into brain on my environment.

sebastiandero · 2016-12-28T12:01:17Z

@NKMR6194 I checked using hubot-brain-inspect. I added the bot, wrote a message on slack and was not in the brain

ysakasin · 2016-12-28T12:14:47Z

@sebastiandero Oh... OK, I get your case.

sebastiandero · 2016-12-28T12:19:20Z

@NKMR6194 any idea on how to fix?

ysakasin · 2016-12-28T12:44:35Z

@sebastiandero May be fix it. Could you try it?

sebastiandero · 2016-12-28T12:50:14Z

@NKMR6194 Yes it is indeed fixed now, thank you so much, have a digital coffee ☕! You solved my byfar biggest frustration with this adapter, hope they merge it soon

sebastiandero

Tested using hubot-brain-inspect and all data is now present from the moment the bot connects!

sebastiandero · 2016-12-28T13:27:05Z

This fixes these issues: #352 #348 #326

DEGoodmanWilson · 2016-12-29T14:14:32Z

@aoberoi could you have a look at this PR?

aoberoi · 2016-12-29T19:14:38Z

excellent work @NKMR6194!

regarding the implementation, i've taken a quick glance and i don't have any big concerns at the moment. i need to take a little more time to look closer at the change and then i'll add any feedback i might have.

besides the implementation, i do have a concern about whether this change is something we want. before i was involved with this project, this functionality existed, and it was seemingly removed. i'd like to understand if it was removed intentionally and what the reason might have been. the most obvious potential reason would be around keeping all this state in process (in memory). that makes scaling a hubot service horizontally and keeping the state in sync difficult. it seems like we might be fine though, since hubot provides a pluggable persistence mechanism for brain state. as long as i can confirm that this solves the concern, and i don't find any additional reasons for why the functionality was removed in the past, i'd be happy to land this functionality.

aoberoi · 2016-12-29T23:16:06Z

src/bot.coffee

@@ -51,6 +51,11 @@ class SlackBot extends Adapter
    @client.on 'reaction_added', @reaction
    @client.on 'reaction_removed', @reaction
    @client.on 'authenticated', @authenticated
+    @client.on 'userChange', @userChange


'userChange' -> 'user_change'

aoberoi · 2016-12-29T23:33:37Z

src/bot.coffee

+      @robot.logger.error "Can't fetch users"
+      return
+
+    for id, user of res.members


wha you are getting here is the JavaScript for...of, but the usage looks confusing because it seems as though you are trying to do a comprehension and pull out properties id and user of members. since members is actually an array and not a normal object, you will get id that is the index in the array and user that is the value. this happens to work, but it doesn't read clearly.

I suggest:

@userChange member for member in res.members

aoberoi · 2016-12-29T23:35:27Z

src/bot.coffee

+      @userChange user
+
+  userChange: (user) =>
+    return unless user?.id?


just curious, when would there be no id?

Probably nothing that case.

aoberoi · 2016-12-29T23:38:09Z

src/bot.coffee

+  userChange: (user) =>
+    return unless user?.id?
+    newUser =
+      name: user.name


can we store id: user.id in here as well? if the idea that Slack-specific properties should go inside the slack property, then this is already not working since the ID that the user will be identified with in the brain is the Slack ID.

aoberoi · 2016-12-29T23:51:17Z

test/bot.coffee

+    should.equal user.name, @stubs.user.name
+    should.equal user.real_name, @stubs.user.real_name
+    should.equal user.email_address, @stubs.user.profile.email
+    should.equal user.slack.misc, @stubs.user.misc


add id to this

aoberoi · 2016-12-29T23:52:44Z

test/bot.coffee

+  it 'Should add a user data', ->
+    @slackbot.userChange(@stubs.user)
+
+    user = @slackbot.robot.brain.data.users[@stubs.user.id]


instead of accesing the brain data directly, wouldn't it make more sense to use the brain.userForId() method?

brain.userForId() create User instance and return it when brain have no user.id.
I'm concerned about that behavior.

good call 👍

aoberoi · 2016-12-29T23:54:23Z

test/stubs.coffee

@@ -129,6 +131,12 @@ beforeEach ->
  @stubs.channelsMock =
    setTopic: (id, topic) =>
      @stubs._topic = topic
+
+  @stubs.responseUsersList =
+    ok: 'ok'


aoberoi · 2016-12-29T23:54:39Z

test/stubs.coffee

+    ok: 'ok'
+    members: [@stubs.user, @stubs.userperiod]
+  @stubs.wrongResponseUsersList =
+    members: [@stubs.user, @stubs.userperiod]


remove members and add ok: false

aoberoi · 2016-12-30T00:08:14Z

@johnagan: care to comment on why robot.brain.users didn't survive the refactor you did in 900402e?

johnagan · 2016-12-30T02:50:07Z

The RTM client already maintains an updated user list in the dataStore and is well tested. IMO we should just expose that a little cleaner in the client class.

aoberoi · 2016-12-30T04:45:09Z

You bring up a fair point about trying to have just one source of truth. My evaluation currently points to using brain and so I created #385 to track this work.

Pros (in favor of robot.brain):

hubot users expect to use the brain API in their scripts.
hubot brain has a robust story for persistence/sync.

Cons (in favor of dataStore):

already tested as part of @slackapi/node-slack-sdk

@johnagan: I can be convinced the other way if you have anything to add.

sebastiandero · 2016-12-30T06:53:10Z

@johnagan the beauty and reason many people use Hubot is because it is so adaptable. Most people will write a bot with it and expect similar behavior on various chat services and hence adapters. Having a different approach to storing the user data imo makes no sense when talking about Hubot.

johnagan · 2016-12-30T14:05:00Z

Having a different approach to storing the user data imo makes no sense when talking about Hubot.

I'm pretty sure most adapters don't offer brain.users. I seem to remember only one other adapter using this, which lead me to believe this wasn't a common interface across connectors.

I'll double check and see.

johnagan · 2016-12-30T14:45:28Z

@sebastiandero you're right! I found it in a couple that I looked at. I'm sold 👍

sebastiandero · 2016-12-30T15:10:35Z

@johnagan having it would improve usability quite a bit, I think. I know that I have been desperately waiting for this since I created a ticket on it couple months ago.

aoberoi · 2016-12-30T22:33:57Z

yay! i've approved the changes, but it looks like we are falling a little short of the coverage target. any takers on adding a few tests?

ysakasin · 2016-12-31T01:39:16Z

@aoberoi Thank you reviewing. PR pass the coverage target. Should I squash this into one commit?

aoberoi · 2016-12-31T04:37:40Z

@NKMR6194: i personally prefer keeping commits in their original form rather than squashing. i hope the other maintainers agree because i'm merging! 🎆 🍾

aoberoi · 2016-12-31T04:39:10Z

i haven't done a release of this module myself yet, so i'm not going to do it late on a friday myself. you can expect a release early next week though!

halkeye · 2017-01-04T19:06:11Z

@aoberoi any chance you'll be able to do the release this week? (I hate to be that person)

I tried out the raw git master version and things seem to be pretty awesome.

aoberoi · 2017-01-06T06:21:35Z

@halkeye as of earlier today, the changes are out! thanks for your patience.

asalaheldin · 2017-01-22T10:14:01Z

Thanks a lot for this great work. I think this fix #326 as now after applying the new version of "hubot-slack" and "hubot-auth" I can see my slack users data inside hubot redis brain and this data include (slack names, real names, emails, ids, etc...).

Load users to robot.brain.users

25a8de9

DEGoodmanWilson self-assigned this Dec 13, 2016

DEGoodmanWilson added feature labels Dec 13, 2016

Test loading users into robot.brain.users

81cc337

ysakasin added 4 commits December 28, 2016 20:00

Test wrong response from web api

0a4ee02

Test invalid user data

c24ef28

Don't need wiping out this case

3a3b342

Test ignore SlackClient in user data

14cc53b

Load users when program started

e1737ac

sebastiandero approved these changes Dec 28, 2016

View reviewed changes

DEGoodmanWilson assigned aoberoi and unassigned DEGoodmanWilson Dec 29, 2016

aoberoi suggested changes Dec 29, 2016

View reviewed changes

Fixes by reviews

7f7194b

aoberoi approved these changes Dec 30, 2016

View reviewed changes

Test merging user data

902b00f

aoberoi merged commit 1585a31 into slackapi:master Dec 31, 2016

This was referenced Jan 10, 2017

brain.users lacks user.name as it equals user.id #348

Closed

Disabled users inside robot brain #184

Closed

SlackBot runs userChange on all users every time a value is set in the brain #305

Closed

sebastiandero mentioned this pull request Jan 11, 2017

hubot auth no longer works after upgrading "hubot-slack": "^4.0.0", hubot-archive/hubot-auth#42

Closed

This was referenced Jan 18, 2017

TypeError: Cannot read property 'toString' of undefined #391

Closed

Handle user_change event properly #392

Closed

ferrixh mentioned this pull request Jan 18, 2017

Redis brain data seems to be missing Slack Names - hubot-auth related #326

Closed

3 tasks

aoberoi mentioned this pull request Oct 17, 2017

hubot-slack with hubot-redis-brain [RangeError: Maximum call stack size exceeded] #212

Closed

sionleroux mentioned this pull request Mar 24, 2018

Flag to disable loading all users on hubot startup (saves space in backing store) #467

Closed

9 tasks

Load users to robot.brain.users #381

Load users to robot.brain.users #381

Conversation

ysakasin commented Dec 13, 2016 • edited Loading

PR Summary

Related Issues

Test strategy

codecov-io commented Dec 13, 2016 • edited Loading

Current coverage is 77.59% (diff: 78.94%)

DEGoodmanWilson commented Dec 13, 2016

ysakasin commented Dec 13, 2016

ysakasin commented Dec 16, 2016

sebastiandero commented Dec 28, 2016

ysakasin commented Dec 28, 2016

sebastiandero commented Dec 28, 2016

ysakasin commented Dec 28, 2016

sebastiandero commented Dec 28, 2016

ysakasin commented Dec 28, 2016

sebastiandero commented Dec 28, 2016

ysakasin commented Dec 28, 2016

sebastiandero commented Dec 28, 2016

ysakasin commented Dec 28, 2016

sebastiandero commented Dec 28, 2016

sebastiandero left a comment

Choose a reason for hiding this comment

sebastiandero commented Dec 28, 2016

DEGoodmanWilson commented Dec 29, 2016

aoberoi commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoberoi commented Dec 30, 2016

johnagan commented Dec 30, 2016

aoberoi commented Dec 30, 2016

sebastiandero commented Dec 30, 2016

johnagan commented Dec 30, 2016 • edited Loading

johnagan commented Dec 30, 2016

sebastiandero commented Dec 30, 2016

aoberoi commented Dec 30, 2016

ysakasin commented Dec 31, 2016

aoberoi commented Dec 31, 2016

aoberoi commented Dec 31, 2016

halkeye commented Jan 4, 2017

aoberoi commented Jan 6, 2017

asalaheldin commented Jan 22, 2017

ysakasin commented Dec 13, 2016 •

edited

Loading

codecov-io commented Dec 13, 2016 •

edited

Loading

johnagan commented Dec 30, 2016 •

edited

Loading