CON-328 Add HTTP call to the slack when a meeting is ended by Ramadhan0 · Pull Request #505 · andela/mrm_api

Ramadhan0 · 2019-10-11T11:09:52Z

Description

This feature ensures that an HTTP call is made to the slack integration micro-service when a meeting is ended, either by the user or when the meeting time runs out.

It sends out different arguments including the organizer_email, room_id, and the event_id.

How to test it:

update your .env file with this variable
SLACK_NOTIFICATION_URL=https://www.softwaretestinghelp.com/tools/30-top-website-link-verification-testing-tools/
make sure all services are up including redis
open insomnia or postman and hit the end event mutation structured as below,
mutation{ endEvent(calendarId:"andela.com_38393338303730373538@resource.calendar.google.com", eventId:"0manph5i3e8lnld6tnvb0o1pcc", organizerEmail:"converge.notification@andela.com", roomId: "18", startTime:"2018-08-19T23:00:00-07:00", endTime:"2018-08-20T05:00:00-07:00", meetingEndTime: "2018-08-14T07:00:00-07:00"){ event{ eventId eventTitle checkedIn cancelled meetingEndTime room{ id name calendarId } } } }
check-in the celery.err.log file for the response to be 200 after finishing the task with a notify-slack name.

Type of change

Please select the relevant option

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Others (cosmetics, styling, improvements)
This change requires a documentation update

How Has This Been Tested?

Unit
Integration
End-to-end

New feature

JIRA

CON-328
CV3-63

MCFrank16

Good job @Ramadhan0

MCFrank16 · 2019-10-17T06:27:32Z

helpers/event/event_slack_notifier.py

@@ -0,0 +1,12 @@
+import requests
+import os


where will you use this os on?

joshuaocero

@Ramadhan0 there are a couple of things you need to work on. Please look into my feedback.

joshuaocero · 2019-10-21T06:40:01Z

config.py

    CELERY_RESULT_BACKEND = os.getenv('CELERY_RESULT_BACKEND')

+    # slack bot url
+    notify_url = os.getenv('SLACK_NOTIFICATION_URL')


Please maintain consistency in variable naming. As you can see, all the other config variables are uppercased.

joshuaocero · 2019-10-21T06:42:49Z

helpers/event/event_slack_notifier.py

+"""
+
+@celery.task(name="notify-slack")
+def notify(event_Id):


Please refer to variable naming conventions for event_Id. (https://www.python.org/dev/peps/pep-0008/#function-and-variable-names)

joshuaocero · 2019-10-21T06:44:02Z

helpers/event/event_slack_notifier.py

+
+@celery.task(name="notify-slack")
+def notify(event_Id):
+    requests.get(url=Config.notify_url+str(event_Id))


Why are you casting event_Id to a string. Is it necessary? Also, please refer to variable naming conventions for event_Id. (https://www.python.org/dev/peps/pep-0008/#function-and-variable-names)

the event_id is an integer and the URL is a string
so, without converting event_id to a string was causing an error @joshuaocero

joshuaocero · 2019-10-21T06:44:49Z

api/events/schema.py

+            event_reject_reason
+        ):
            raise GraphQLError("Event cancelled but email not sent")
+        event_Id=kwargs['event_id']


Please refer to variable naming conventions for event_Id. (https://www.python.org/dev/peps/pep-0008/#function-and-variable-names)

joshuaocero · 2019-10-21T06:44:56Z

api/events/schema.py

+        ):
            raise GraphQLError("Event cancelled but email not sent")
+        event_Id=kwargs['event_id']
+        notify.delay(event_Id)


Please refer to variable naming conventions for event_Id. (https://www.python.org/dev/peps/pep-0008/#function-and-variable-names)

joshuaocero · 2019-10-21T06:47:54Z

helpers/event/event_slack_notifier.py

@@ -0,0 +1,11 @@
+import requests


Make this filename leaner. It is already in a folder called events, no need to prefix the filename with event_.

joshuaocero · 2019-10-21T06:49:37Z

helpers/event/event_slack_notifier.py

+"""
+
+@celery.task(name="notify-slack")
+def notify(event_Id):


Please make this function name more descriptive. notify what? Think of a better function name. Ask yourself what exactly the function is doing then name it appropriately.

joshuaocero · 2019-10-21T06:55:34Z

tests/test_events/test_slack_notifier.py

+class test_slack_notifier(unittest.TestCase):
+
+    @patch('helpers.event.event_slack_notifier.requests.get')
+    def test_add_room(self, mock_get_request):


Why is this function called test_add_room? You need to pay more attention to detail. Avoid unnecessary copying and pasting.

mifeille · 2019-11-07T12:17:40Z

alembic/versions/2ee259ae22e5_add_organizer_column_event.py

+
+def upgrade():
+    op.add_column('events', sa.Column(
+        'organizer_email', sa.String(), nullable=True))


I just checked Samuel's PR and I saw that the organizer email is sent as a list since it is taking two emails, is this data type appropriate for storing it?

I just wanna know from the list of 2 emails, how will I know which one is the actual organizer_email @mifeille

mifeille

Please validate organizerEmail and roomId fields.

- ensure the returned url is comprised with the organizer_email and room_id [Delivers CON-328]

Ramadhan0 assigned MCFrank16 Oct 11, 2019

Ramadhan0 added the wip label Oct 11, 2019

Ramadhan0 force-pushed the story/CON-328-HTTP-call-to-the-slack branch 28 times, most recently from 82fb602 to 775708d Compare October 15, 2019 16:53

Ramadhan0 force-pushed the story/CON-328-HTTP-call-to-the-slack branch 6 times, most recently from f22261b to b936f9b Compare October 16, 2019 12:11

Ramadhan0 added Ready for Review and removed wip labels Oct 16, 2019

Ramadhan0 assigned mifeille Oct 16, 2019

Ramadhan0 requested review from MCFrank16, joshuaocero and mifeille October 16, 2019 15:37

Ramadhan0 unassigned MCFrank16 and mifeille Oct 16, 2019

MCFrank16 approved these changes Oct 17, 2019

View reviewed changes

helpers/event/event_slack_notifier.py Outdated

@@ -0,0 +1,12 @@

import requests

import os

Copy link

MCFrank16 Oct 17, 2019

Choose a reason for hiding this comment

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

where will you use this os on?

MCFrank16 self-requested a review October 17, 2019 06:32

Ramadhan0 force-pushed the story/CON-328-HTTP-call-to-the-slack branch from b936f9b to 5981a90 Compare October 17, 2019 07:09

mifeille previously approved these changes Oct 17, 2019

View reviewed changes

joshuaocero suggested changes Oct 21, 2019

View reviewed changes

Ramadhan0 force-pushed the story/CON-328-HTTP-call-to-the-slack branch 5 times, most recently from 6e54f6f to c8db89e Compare October 22, 2019 14:40

MCFrank16 approved these changes Oct 28, 2019

View reviewed changes

mifeille reviewed Nov 7, 2019

View reviewed changes

adafia approved these changes Nov 19, 2019

View reviewed changes

mifeille reviewed Nov 19, 2019

View reviewed changes

Ramadhan0 and others added 2 commits December 10, 2019 10:32

CON-328-story: allow HTTP call to the slack when a metting is ended

57ea920

(CON-328-story): add organizer_email and room_id

dd9dbb1

- ensure the returned url is comprised with the organizer_email and room_id [Delivers CON-328]

Conversation

Ramadhan0 commented Oct 11, 2019 • edited by MCFrank16 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

JIRA

Uh oh!

MCFrank16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuaocero 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!

joshuaocero Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mifeille left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ramadhan0 commented Oct 11, 2019 •

edited by MCFrank16

Loading

joshuaocero Oct 21, 2019 •

edited

Loading