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

Add suggested channels greeting message #106

Closed
wants to merge 3 commits into from

Conversation

Digbigpig
Copy link

This feature will check if new slack users have profile information which we use
to suggest matching slack channels

Currently looks for a 1:1 match with profile text to slack channel name.

I haven't ran or tested the code yet

This feature will check if new slack users have profile information which we use
to suggest matching slack channels

Currently looks for a 1:1 match with profile text to slack channel name.
Copy link
Member

@apex-omontgomery apex-omontgomery left a comment

Choose a reason for hiding this comment

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

General suggestions, as far as testing these out go we can deploy them to the test slack environment.

I'd like to know what allen thinks about including these messages after the 30 second wait and if they should go into the grouped futures list.

return results


def parse_suggestions_from_profile(profile: Dict) -> List[str]:
Copy link
Member

Choose a reason for hiding this comment

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

It's not very clear at first what this is doing.

  1. You have a list of profile fields we care about
  2. You want to iterate through these profile fields.
  3. Using non-empty profile fields, grab each item.

For some profile fields they are lists, and some profile fields they are single items.

This is because we are storing lists of items as strings joined by commas.

Stylistically I think something like this is a bit clearer.

def filter_non_empty_profile_fields(profile: Dict) -> List[str]:
    
    # These fields are a 1:N for possible channel matching split on commas
    csv_profile_fields = ["interests", "programming_languages", "disciplines"]
    # These fields can only be a single channel for the item    
    flat_profile_fields = ["city", "state" ]
    
    filtered_profile_field_values = []
    
    for field in csv_profile_fields:        
        if profile.get(field) is not None:
            for word in filter(None, profile[field].lower().split(",")):
                filtered_profile_field_values.append(word)
        
    for field in flat_profile_fields:        
        word = profile.get(field)
        if word is not None:
            filtered_profile_field_values.append(word)
    
    return filtered_profile_field_values

I don't remember if the api has a way to filter some of the profile values, or request the api to split the values when they have multiple items into a list.

It would be better if we stored them in the database as a 1:N relationship and then had the api return a list in the json.

possible_suggestions = parse_suggestions_from_profile(data)

# For each value in the profile data, check that we have a channel name that matches.
for info in possible_suggestions:
Copy link
Member

Choose a reason for hiding this comment

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

Not to worried about a double for loop but you could avoid the lookup if you converted the list of channels into a set, or a key value pair.

But I think the naming should be clearer:

for suggestion in possible_suggestions:
    for channel in channels:
        if suggestion in channel[1]:
            results.append((channel[0], channel[1]))
            break

pybot/endpoints/slack/utils/event_utils.py Show resolved Hide resolved
# For each value in the profile data, check that we have a channel name that matches.
for info in possible_suggestions:
for channel in channels:
if info in channel[1]:
Copy link
Member

@apex-omontgomery apex-omontgomery Sep 1, 2019

Choose a reason for hiding this comment

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

Not sure how you got that the channel can be indexed...

"channels": [
        {
            "id": "C0G9QF9GW",
            "name": "random",
            "is_channel": true,
            "created": 1449709280,
            "creator": "U0G9QF9C6",
            "is_archived": false,
            "is_general": false,
            "name_normalized": "random",
            "is_shared": false,
            "is_org_shared": false,
            "is_member": true,
            "is_private": false,
             ........
        },

I think you need to compare the channel['name'] to the suggestion. But you should also exclude the channel when channel['is_private'] == True as we don't want to make the names of these channels visible.

Copy link
Author

Choose a reason for hiding this comment

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

Oh thanks for catching that. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Based on the above, ignore the is_private part but we still need to make sure this works.

@aaron-junot
Copy link
Member

I'm not sure if this was supposed to solve #91 or #110 or something different. Please feel free to reopen if you'd like to finish this, but closing for now.

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

3 participants