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 a Slack user cache #131

Merged
merged 13 commits into from
Sep 9, 2019
Merged

Add a Slack user cache #131

merged 13 commits into from
Sep 9, 2019

Conversation

ChrisAnn
Copy link
Contributor

@ChrisAnn ChrisAnn commented Sep 5, 2019

Part of the work for pre-loading all external users to the database.

Tested locally to ensure all users are added to the database and returned from the users api endpoint.

@ChrisAnn ChrisAnn added the wip Work in Progress label Sep 5, 2019
@@ -11,6 +10,7 @@ class Meta:
app_id = models.CharField(max_length=50, blank=False, null=False)
external_id = models.CharField(max_length=50, blank=False, null=False)
display_name = models.CharField(max_length=50, blank=False, null=False)
full_name = models.CharField(max_length=50, blank=True, null=True)
Copy link
Contributor Author

@ChrisAnn ChrisAnn Sep 5, 2019

Choose a reason for hiding this comment

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

This was added to consolidate slack profile data returning fullname and the ExternalUser model not including it. Having read the Slack docs, fullname != display_name so we can't substitute one for another.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Any idea how they differ? Should we consider also returning full_name in the API response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference seems to be this: https://get.slack.help/hc/en-gb/articles/115004692303-Change-how-names-are-displayed
We probably want full_name on the UI, so yeah, I'll add it to the API response.

break


def get_user_profile(external_id):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In another PR, we should replace all usages of settings.SLACK_CLIENT.get_user_profile with this to take advantage of the cache.

@ChrisAnn ChrisAnn removed the wip Work in Progress label Sep 5, 2019
@ChrisAnn ChrisAnn requested a review from milesbxf September 5, 2019 14:14
Copy link
Contributor

@milesbxf milesbxf left a comment

Choose a reason for hiding this comment

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

I've left a few minor comments, but looks good otherwise 👍

@@ -11,6 +10,7 @@ class Meta:
app_id = models.CharField(max_length=50, blank=False, null=False)
external_id = models.CharField(max_length=50, blank=False, null=False)
display_name = models.CharField(max_length=50, blank=False, null=False)
full_name = models.CharField(max_length=50, blank=True, null=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Any idea how they differ? Should we consider also returning full_name in the API response?


def update_user_cache():
cursor = None
while True:
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do while cursor != "": here and remove the check below (I've just remembered that #85 does this)

"name": "chrisevans",
"deleted": "False",
"color": "9f69e7",
"real_name": "Glinda Southgood",
Copy link
Contributor

Choose a reason for hiding this comment

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

😮 @evnsio you never told us about your real name!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dammit 🤦‍♀ . I thought I'd removed all of mention of Chris in the test data. Sorry :(

@ChrisAnn ChrisAnn force-pushed the slack-user-cache branch 3 times, most recently from c77a1af to 0a0e928 Compare September 6, 2019 13:15
@ChrisAnn ChrisAnn requested a review from milesbxf September 6, 2019 13:36
Copy link
Contributor

@milesbxf milesbxf left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ChrisAnn ChrisAnn merged commit 5733a6c into master Sep 9, 2019
@ChrisAnn ChrisAnn deleted the slack-user-cache branch September 9, 2019 10:20
@milesbxf milesbxf mentioned this pull request Sep 21, 2019
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.

2 participants