Add slack bot integration #935

darkdarkdragon · 2018-04-21T11:57:21Z

add slack settings tab on user's settings page
send same messages to user's slack as to gitcoin's slack

Checklist

linter status: 100% pass
changes don't break existing behavior
commit message follows commit guidelines

Affected core subsystem(s)

backend
frontend

Testing

Tested manually

Refers/Fixes

Fixes: #259

- send same messages to user's slack as to gitcoin's slack Fixes: gitcoinco#259

mbeacom

Thanks for the contribution! A few review notes, but overall looks pretty good so far! I'll pull this down and test shortly!

mbeacom · 2018-04-23T05:00:26Z

app/dashboard/notifications.py

    conv_details = ""
    usdt_details = ""
    try:
        conv_details = f"@ (${round(convert_token_to_usdt(bounty.token_name),2)}/{bounty.token_name})"
        usdt_details = f"({bounty.value_in_usdt_now} USD {conv_details} "
    except Exception:
        pass  # no USD conversion rate
+    except Exception as e:


Can we modify this method to msg = '' and return msg regardless, so we aren't mixing return types and drop the second except Exception as e

mbeacom · 2018-04-23T05:00:49Z

app/dashboard/notifications.py

+    return True
+
+
+def build_message_for_slack(bounty, event_name):


Can you add a docstring here like the one added below?

I've added it with previous commit

mbeacom · 2018-04-23T05:04:27Z

app/dashboard/notifications.py

+            except Exception as e:
+                print(e)
+    except IndexError:
+        return False


Should be able to pass on this since sent will be False at lines 233-234.

mbeacom · 2018-04-23T05:04:57Z

app/dashboard/notifications.py

+            except Exception as e:
+                print(e)
+    except IndexError:
+        return False
    except Exception as e:
        print(e)
        return False


Same as above.

mbeacom · 2018-04-23T05:07:59Z

app/marketing/models.py

@@ -37,6 +37,9 @@ class EmailSubscriber(SuperModel):
    priv = models.CharField(max_length=30, default='')
    github = models.CharField(max_length=255, default='')
    keywords = ArrayField(models.CharField(max_length=200), blank=True, default=[])
+    repos = ArrayField(models.CharField(max_length=200), blank=True, default=[])
+    slack_token = models.CharField(max_length=255, default='')


Can we move these new fields to: dashboard.models.Profile ?

mbeacom · 2018-04-23T05:09:06Z

app/marketing/views.py

+        else:
+            es.metadata['ip'].append(ip)
+        es.save()
+        msg = "Updated your preferences.  "


This message should be wrapped in the gettext_lazy helper: _()

mbeacom · 2018-04-23T05:10:29Z

app/marketing/views.py

@@ -615,6 +619,44 @@ def email_settings(request, key):
    return TemplateResponse(request, 'settings/email.html', context)


+def slack_settings(request):
+


Can you replace this new line with a docstring?

mbeacom · 2018-04-23T05:11:31Z

app/retail/templates/settings/slack.html

+{% load i18n static %}
+
+{% block settings_content %}
+    <form id="settings" method="POST">


Can you update this file to use 2 space indentation?

mbeacom · 2018-04-23T05:19:33Z

app/marketing/views.py

+        repos = request.POST.get('repos').split(',')
+        channel = request.POST.get('channel', '')
+        es.slack_token = token
+        es.repos = repos


I'm not certain, but we might want to validate the user is a member of the tracked repo or limit trackable repos to those returned from GH for the authenticated user.

I'm actually checking if notfication needs to be sent using users github name:
subscribers = EmailSubscriber.objects.filter(profile__handle=username, repos__contains=[repo])
that way user will be able to get notification only for his own repos.
but I don't know how this will behave if user wants to monitor repos that belong to organization,
and I don't know how to test such use case.
Any thoughts on how I can test this use case?

mbeacom · 2018-04-23T05:19:52Z

app/marketing/views.py

@@ -417,6 +417,10 @@ def funnel(request):
        'body': 'Feedback',
        'href': '/settings/feedback',
    },
+    {
+        'body': 'Slack',
+        'href': '/settings/slack',


Can you modify this to use reverse()

codecov · 2018-04-23T15:03:02Z

Codecov Report

Merging #935 into master will decrease coverage by 16.05%.
The diff coverage is 8.21%.

@@             Coverage Diff             @@
##           master     #935       +/-   ##
===========================================
- Coverage   33.32%   17.27%   -16.06%     
===========================================
  Files         100       99        -1     
  Lines        6361     6419       +58     
  Branches      768      776        +8     
===========================================
- Hits         2120     1109     -1011     
- Misses       4142     5304     +1162     
+ Partials       99        6       -93

Impacted Files	Coverage Δ
app/app/urls.py	`0% <ø> (-93.55%)`	⬇️
app/dashboard/views.py	`0% <0%> (-16.1%)`	⬇️
app/dashboard/helpers.py	`13.55% <0%> (-15.86%)`	⬇️
app/marketing/views.py	`0% <0%> (-11.66%)`	⬇️
app/dashboard/models.py	`41.55% <100%> (-22.71%)`	⬇️
app/dashboard/notifications.py	`8.04% <5.26%> (-7.03%)`	⬇️
app/external_bounties/forms.py	`0% <0%> (-100%)`	⬇️
app/external_bounties/router.py	`0% <0%> (-100%)`	⬇️
app/app/sitemaps.py	`0% <0%> (-72.5%)`	⬇️
... and 37 more

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 1032f16...5dfb9e1. Read the comment docs.

stickler-ci · 2018-04-23T15:03:34Z

app/dashboard/migrations/0056_auto_20180423_1343.py

+        migrations.AddField(
+            model_name='profile',
+            name='repos',
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=200), blank=True, default=[], size=None),


E501 line too long (140 > 120 characters)

stickler-ci · 2018-04-23T15:03:35Z

app/marketing/views.py

    },
-]
+    {


E122 continuation line missing indentation or outdented

darkdarkdragon · 2018-04-23T22:33:11Z

@mbeacom Can you provide some feed? I think I've covered all comments.

owocki · 2018-04-23T23:24:49Z

when i try to migrate

root@d9cd23efb95b:/code/app# ./manage.py migrate
/usr/local/lib/python3.6/site-packages/psycopg2/__init__.py:144: UserWarning: The psycopg2 wheel package will be renamed from release 2.8; in order to keep installing from binary please use "pip install psycopg2-binary" instead. For details see: <http://initd.org/psycopg/docs/install.html#binary-install-from-pypi>.
  """)
CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0057_auto_20180419_0910, 0056_auto_20180423_1343 in dashboard).
To fix them run 'python manage.py makemigrations --merge'

owocki · 2018-04-23T23:27:31Z

http://bits.owocki.com/2t1W1X2z3R2f/Screen%20Shot%202018-04-23%20at%205.25.44%20PM.png

usability feedback:

at first glance on this field, it'd be nice to have some p text that says what it does if i put my slack token in it
itd be nice to have a link to the slack API token generation on the first field
for the second field, do i enter full URLs, just a substring?
is there validation on the slack channel to post to field? do i include a # or not?

owocki · 2018-04-23T23:30:05Z

doesnt seem to work with these settings.. what am i doing wrong? http://bits.owocki.com/1t0i3Y333C0t/Screen%20Shot%202018-04-23%20at%205.29.51%20PM.png

mbeacom · 2018-04-23T23:50:02Z

app/dashboard/migrations/0056_auto_20180423_1343.py

@@ -0,0 +1,30 @@
+# Generated by Django 2.0.4 on 2018-04-23 13:43


You'll need to rebase with master, delete this file, and recreate your migrations via: make migrations

- add slack settings tab on user's settings page

aae7d84

- send same messages to user's slack as to gitcoin's slack Fixes: gitcoinco#259

mbeacom suggested changes Apr 23, 2018

View reviewed changes

mbeacom added frontend This needs frontend expertise. backend This needs backend expertise. bot labels Apr 23, 2018

fixes gitcoinco#259

35ce8f3

mbeacom added the waiting on contributor label Apr 23, 2018

stickler-ci reviewed Apr 23, 2018

View reviewed changes

mbeacom and others added 2 commits April 23, 2018 11:04

Merge branch 'master' into slacknotify

34ffca7

fixes gitcoinco#259

64626ef

mbeacom added the in progress label Apr 23, 2018

Merge commit '34ffca761a8efe533e49d973c81238c5428c5fd9' into slacknotify

5dfb9e1

mbeacom suggested changes Apr 23, 2018

View reviewed changes

darkdarkdragon closed this Apr 24, 2018

mbeacom removed the in progress label Apr 24, 2018

darkdarkdragon deleted the slacknotify branch April 24, 2018 10:27

owocki mentioned this pull request Apr 24, 2018

Add slack bot integration #955

Merged

3 tasks

gitcoinbot mentioned this pull request Apr 27, 2018

Add slack bot integration #259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add slack bot integration #935

Add slack bot integration #935

darkdarkdragon commented Apr 21, 2018

mbeacom left a comment

mbeacom Apr 23, 2018

mbeacom Apr 23, 2018

darkdarkdragon Apr 23, 2018

mbeacom Apr 23, 2018

mbeacom Apr 23, 2018

mbeacom Apr 23, 2018

mbeacom Apr 23, 2018

mbeacom Apr 23, 2018

mbeacom Apr 23, 2018

mbeacom Apr 23, 2018

darkdarkdragon Apr 23, 2018

mbeacom Apr 23, 2018

codecov bot commented Apr 23, 2018 •

edited

Loading

stickler-ci Apr 23, 2018

stickler-ci Apr 23, 2018

darkdarkdragon commented Apr 23, 2018

owocki commented Apr 23, 2018

owocki commented Apr 23, 2018 •

edited

Loading

owocki commented Apr 23, 2018

mbeacom Apr 23, 2018

		@@ -615,6 +619,44 @@ def email_settings(request, key):
		return TemplateResponse(request, 'settings/email.html', context)


		def slack_settings(request):

		@@ -0,0 +1,30 @@
		# Generated by Django 2.0.4 on 2018-04-23 13:43

Add slack bot integration #935

Add slack bot integration #935

Conversation

darkdarkdragon commented Apr 21, 2018

Checklist

Affected core subsystem(s)

Testing

Refers/Fixes

mbeacom left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdarkdragon commented Apr 23, 2018

owocki commented Apr 23, 2018

owocki commented Apr 23, 2018 • edited Loading

owocki commented Apr 23, 2018

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2018 •

edited

Loading

owocki commented Apr 23, 2018 •

edited

Loading