feat: Slack pairings by tarikeshaq · Pull Request #553 · ubclaunchpad/rocket2

tarikeshaq · 2020-10-12T03:19:07Z

fixes #514 , closes https://github.com/ubclaunchpad/ideas/issues/8

Details

Creates basic donut style matching for Rocket.

What implemented so far:

A donut scheduler that currently runs every minute (for debugging purposes, will bump to a reasonable number once this is ready to land)
Some initial refactoring to allow the schedulers to have access to the db
Slack api calls needed to:
- Get a slack channel's id by name
- Create a private conversation given a list of users
Basic persistence of pairings in a new pairings table

TODOs before this is ready:

Add persistence, to prevent users from getting matched again
~~Add TTL to entries in the Pairings table~~ I can file a follow up for this, we can live without it for now
Add tests
- For slack API calls
- For persistence
Add documentation
- General documentation for the feature
- For the new environment variable
- Documentation for the new pairings table

Where I am currently:

Contemplating if I should add a table for the pairings or if it should be a part of the User model, leaning towards the latter with possibly opening a ticket for the former beyond this pr.
Went with creating a new table 😄 Now working on adding the TTL and making sure edge cases don't explode on us

I'll be updating the above as I go.

bobheadxi · 2020-10-12T03:25:13Z

chuck-sys · 2020-10-12T03:52:25Z

Contemplating if I should add a table for the pairings or if it should be a part of the User model, leaning towards the latter with possibly opening a ticket for the former beyond this pr.

IMO I think you should go with the former (though the difference in work is pretty significant). Seems less hacky and wouldn't leak too much information (example: /rocket user view can potentially see who you matched with).

And it would set the precedent for similar things in the future.

bobheadxi

nice progress! 🎉

(also, it seems your commit authors arent attached to your GitHub, not sure if intentional?)

bobheadxi · 2020-10-14T03:07:09Z

+    def get_job_args(self) -> Dict[str, Any]:
+        """Get job configuration arguments for apscheduler."""
+        return {'trigger':      'cron',
+                'minute': '*',


I'm guessing this is just temporary for testing, but could also just make this a configuration option! SLACK_PAIRING_FREQUENCY for example

Alrighty made it an env variable as a cron job string... not the cleanest so happy for recommendations!

codecov · 2020-11-01T07:08:46Z

Codecov Report

❌ Patch coverage is 83.52273% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.46%. Comparing base (7b313f1) to head (d2e2b87).
⚠️ Report is 20 commits behind head on master.

Files with missing lines	Patch %	Lines
app/model/pairing.py	65.00%	14 Missing ⚠️
db/dynamodb.py	57.14%	7 Missing and 2 partials ⚠️
tests/memorydb.py	76.92%	0 Missing and 3 partials ⚠️
interface/slack.py	87.50%	1 Missing and 1 partial ⚠️
app/scheduler/__init__.py	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
- Coverage   93.01%   92.46%   -0.55%     
==========================================
  Files          42       44       +2     
  Lines        2420     2589     +169     
  Branches      323      346      +23     
==========================================
+ Hits         2251     2394     +143     
- Misses        110      131      +21     
- Partials       59       64       +5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tarikeshaq · 2020-11-01T07:24:59Z

OK I got some time to work on this again (after like 20 days 😢) and Added a few tests.

I think this is ready for a round of review so I'll change it from a draft. My next focus will be docs and addressing any feedback that pops up. No rush on the review, we are all busy and this is decently large.

bobheadxi

Fantastic progress, really excited to see this land! Thanks for taking the time to do this ❤️

bobheadxi · 2020-11-01T09:49:36Z

+            self.bot.send_to_channel("Hello! \
+                You have been matched by Rocket \
+                please use this channel to get to know each other!",
+                                     group_name)


I wonder if we should have a configuration option SLACK_PAIRING_TIPS_LINK that can generate a button that links to tips for what to do in a pairing (i.e. "set up video call, talk about x and y") - for Launch Pad this could be a handbook page, for example

Hmmm Can we do this on a follow up or do ya think it's important? Could be a really good first issue for someone!

Yep! Can you add all the TODO items for follow-up issues in the PR description as a checklist, to make sure we don't forget?

Co-authored-by: Robert Lin <robertlin1@gmail.com>

bobheadxi · 2020-11-02T01:25:14Z

chuck-sys

Really good job on this! Seems to just need the docs now.

chuck-sys · 2020-11-02T16:56:04Z

+            self.bot.send_to_channel("Hello! \
+                You have been matched by Rocket \
+                please use this channel to get to know each other!",


That's a long string. Can you put it refactor it to be a static member for easy changing?

chuck-sys · 2020-11-02T16:57:48Z

+        Creates pairs of users that haven't been matched before
+
+        :param users: A list of slack ids of all users to match
+
+        If a pairing cannot be done, then the history of pairings is
+        purged, and the algorithm is run again.


Talk about what kind of algorithm is used here please.

chuck-sys · 2020-11-02T17:02:19Z

+        not_paired = list(
+            filter(lambda user: user not in already_added, users))


Suggested change

not_paired = list(

filter(lambda user: user not in already_added, users))

not_paired = [user for user in users if user not in already_added]

Feel free to not use this. I just want everything to fit nicely on one line.

P Y T h O n I C

chuck-sys · 2020-11-02T17:07:14Z

+        self.user1_slack_id = user1_slack_id
+        self.user2_slack_id = user2_slack_id


How would 3 people models be represented in the database? Or is it intended that 3 people pairings are a one-off?

chuck-sys · 2020-11-02T17:56:49Z

+        # If we have an odd number of people that is not 1
+        # We put the odd person out in one of the groups
+        # So we might have a group of 3


Suggested change

# If we have an odd number of people that is not 1

# We put the odd person out in one of the groups

# So we might have a group of 3

# If we have an odd number of people that is not 1,

# or if there is 1 person left unpaired:

# We put the odd person out in one of the groups

# So we might have a group of 3

chuck-sys · 2020-11-02T19:31:33Z

+        table_name = self.CONST.get_table_name(Model)
+        table = self.ddb.Table(table_name)
+        table.delete()
+        table.wait_until_not_exists()
+        self.__create_table(table_name)


May want to comment that this is the best way to remove all items from a table, because it makes fewer calls.

chuck-sys · 2020-11-02T19:33:55Z

+
+        :param users: the list of users to add to the private chat
+
+        :raise SlackAPIError if the slack API returned error openning the chat


Suggested change

:raise SlackAPIError if the slack API returned error openning the chat

:raise SlackAPIError if the slack API returned error opening the chat

chuck-sys · 2020-11-02T19:40:18Z

 AWS_SECRET_KEY='itsa secret'
 AWS_USERS_TABLE='users'
 AWS_TEAMS_TABLE='teams'
+AWS_PAIRINGS_TABLE='pairings'


May want to add the optional config options for others to see how it works.

chuck-sys · 2020-11-02T19:41:02Z

+
+    def test_valid_pairing(self):
+        """Test the pairing static method is_valid()."""
+        self.assertTrue(Pairing.is_valid(self.p0))


Don't forget to test the invalid cases as well!

chuck-sys · 2020-11-02T19:42:12Z

+        assert 'user1' in args
+        assert 'user2' in args
+        assert len(args) == 2


Suggested change

assert 'user1' in args

assert 'user2' in args

assert len(args) == 2

self.assertIn('user1', args)

self.assertIn('user2', args)

self.assertEqual(len(args), 2)

There are others.

bobheadxi · 2020-12-02T11:24:10Z

Nothing that this closes https://github.com/ubclaunchpad/ideas/issues/8 as well

tarikeshaq added the ⚒WIP label Oct 12, 2020

bobheadxi requested a review from chuck-sys October 12, 2020 03:24

bobheadxi reviewed Oct 12, 2020

View reviewed changes

Comment thread config/__init__.py Outdated

chuck-sys reviewed Oct 12, 2020

View reviewed changes

Comment thread app/scheduler/modules/donut.py Outdated

bobheadxi removed the ⚒WIP label Oct 12, 2020

tarikeshaq added 4 commits October 13, 2020 10:29

Adds pairing feature to rocket

4abe871

Add pairings table to dynamodb, with ttl stub

0c9f9b2

Adds pairing model

f76a14a

Adds persistance to pairing algorithm

eb3ff7d

tarikeshaq force-pushed the donut branch from bfaeb32 to eb3ff7d Compare October 13, 2020 19:18

bobheadxi reviewed Oct 14, 2020

View reviewed changes

bobheadxi changed the title ~~[WIP] Adds donut feature to rocket~~ feat: Slack pairings Oct 15, 2020

tarikeshaq added 5 commits October 31, 2020 22:44

Adds tests for slack api additions

6c07dc3

Change launchpad to launch pad

8b47828

Merge branch 'master' into donut

ee7b640

Adds tests for the scheduler

09d3bdb

Adds tests for model

0d35701

tarikeshaq marked this pull request as ready for review November 1, 2020 07:25

tarikeshaq requested review from a team as code owners November 1, 2020 07:25

bobheadxi requested changes Nov 1, 2020

View reviewed changes

tarikeshaq and others added 4 commits November 1, 2020 12:18

Apply suggestions from code review

128ae06

Co-authored-by: Robert Lin <robertlin1@gmail.com>

Feature toggle the pairings

4d39fe4

Makes frequency an environment variable

0b9f76a

Merge branch 'master' into donut

36fea81

Fix linting line too long

d2e2b87

chuck-sys requested changes Nov 2, 2020

View reviewed changes

		not_paired = list(
		filter(lambda user: user not in already_added, users))

	not_paired = list(
	filter(lambda user: user not in already_added, users))
	not_paired = [user for user in users if user not in already_added]

		self.user1_slack_id = user1_slack_id
		self.user2_slack_id = user2_slack_id


		:param users: the list of users to add to the private chat

		:raise SlackAPIError if the slack API returned error openning the chat

Uh oh!

Conversation

tarikeshaq commented Oct 12, 2020 • edited by bobheadxi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

What implemented so far:

TODOs before this is ready:

Where I am currently:

Uh oh!

bobheadxi commented Oct 12, 2020

Uh oh!

Uh oh!

Uh oh!

chuck-sys commented Oct 12, 2020

Uh oh!

bobheadxi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tarikeshaq commented Nov 1, 2020

Uh oh!

bobheadxi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobheadxi commented Nov 2, 2020

Uh oh!

chuck-sys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobheadxi commented Dec 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tarikeshaq commented Oct 12, 2020 •

edited by bobheadxi

Loading

codecov Bot commented Nov 1, 2020 •

edited

Loading