Heimdall prompts users of the zoom command to post meeting notes after the meeting #56

kb0rg · 2019-06-14T08:48:10Z

Closes #46

To encourage folks to externalize conversations that happen in zoom chats, Heimdall will keep an eye on meetings initiated by the zoom command, and send a note to the remind the people using that zoom to follow up by posting meeting notes.

TODO:

get the api call to get meeting details working
actually implement the whole watch-meeting interval thing
make sure Heimdall's reply in in the correct thread
(maybe) tag the user who called the command?
clean up branch (squash ~~some o~~ these commits ?)
clean up branch more (update const values, remove console logs, etc. In short: convert local dev --> deployable)
add "meeting never started" response

env-var.list

scripts/zoom.js

lib/zoom.ts

kb0rg

starting a review bc I'm leaving some comments/ questions in the diff, and.. yeah.

scripts/zoom.js

If meeting never starts after specified delay, we stop watching If meeting starts and ends, we prompt the user to post notes in flow If meeting starts, but does not end within specified delay, we assume it still happened, and send a prompt anyway If there are errors at any point in this process, we log them, and in some cases send user a message about the error

Remove function param no longer needed Remove notes

If meeting never starts, send prompt to start another if needed If meeting starts and ends within timeout, send prompt to user to post meeting notes If meeting starts but does not end within timeout, we assume it still happened and send the prompt to post notes, but do not mention user

Update constants values to reasonable defaults Remove debug logging

kb0rg · 2019-06-20T00:26:11Z

I do believe this puppy is ready for review, @Shadowfiend

Shadowfiend · 2019-06-20T13:06:27Z

Oh snizzle snaps.

Shadowfiend

Left some thoughts. The naming stuff and nested if stuff I think is best addressed here, while the return value structure, a potential revision to use async/await, and pulling this functionality into Session, I think is best left for a follow-up refactoring PR.

Shadowfiend · 2019-06-20T13:35:53Z

lib/zoom.ts

@@ -144,4 +157,4 @@ class Account {
  }
 }

-export { getSession, Session }
+export { getSession, Session, getMeetingDetails }


Let's make a note to refactor this one and pull it into Session. I think that + refactoring to async/await below can be one bundle.

created issue #62 for the refactoring follow-up

Shadowfiend · 2019-06-20T13:36:55Z

scripts/zoom.js

      })
      .catch(err => {
        robot.logger.error(
          "Failed to fetch next available meeting:",
          util.inspect(err),
        )
        res.send("Uh-oh, there was an issue finding an available meeting :(")
+        return


You don't need this return, as it's basically the same as letting the function end.

ah good to know. at some point in my testing I was hitting a .then() block when I didn't expect to, but I think I over-corrected and just put returns everywhere :)

yeahhhh nope. flat-out over-compensating. cleaned em up.

Shadowfiend · 2019-06-20T13:38:14Z

scripts/zoom.js

+}
+
+function watchMeeting(meeting) {
+  let startIntervalIdPromise = new Promise((resolve, reject) => {


I would rename this meetingStartedPromise. When this promise resolves, it indicates whether the meeting started (true) or not ("never started", which incidentally should probably just be false).

good call. I thought there was a reason I did this.. but the reason may very well have been based on a misunderstanding. I'll clean this up (and report back here if I end up running into the thing I was trying to avoid)

~~yeahhhh nope. flat-out over-compensating. cleaned em up.~~ (wrong thread)

Shadowfiend · 2019-06-20T13:38:35Z

scripts/zoom.js

+    }, MEETING_START_TIMEOUT_DELAY)
+  })
+  return startIntervalIdPromise.then(meetingStartStatus => {
+    let endIntervalIdPromise = new Promise((resolve, reject) => {


Similarly, this can be meetingEndedPromise.

We'll also want to consider how this promise resolves. Right now it can resolve with one of three values:

"never started"

true

null

I think we'll want to align these into one type, which could be a set of constant strings:

"never started"

"finished"

"timed out"

We can do this in a refactoring follow-up.

created issue #62 for the refactoring follow-up

Shadowfiend · 2019-06-20T13:41:48Z

scripts/zoom.js

+      })
+      .then(meeting => {
+        robot.logger.info(`Start watching meeting: ${meeting.id}`)
+        return watchMeeting(meeting)


watchMeeting could fluently be named waitForMeetingEnd, which would make this read waitForMeetingEnd(...).then(do stuff).

Shadowfiend · 2019-06-20T13:42:46Z

scripts/zoom.js

+      .then(meeting => {
+        robot.logger.info(`Start watching meeting: ${meeting.id}`)
+        return watchMeeting(meeting)
+          .then(fulfilledPromise => {


At this point this is no longer a promise, it's now the final value that the endIntervalIdPromise/meetingEndedPromise resolves. So this is really just meetingEndStatus or something of the sort.

Shadowfiend · 2019-06-20T13:43:48Z

scripts/zoom.js

+        robot.logger.info(`Start watching meeting: ${meeting.id}`)
+        return watchMeeting(meeting)
+          .then(fulfilledPromise => {
+            if (fulfilledPromise) {


Rather than nesting ifs here, I think you can compare fulfilledPromise to the three possibilities from my comment above in a single if/else if/etc.

Clarifying question re your comment above:

The naming stuff and nested if stuff I think is best addressed here, while the return value structure [...] I think is best left for a follow-up refactoring PR

Is there a way to clean up the nested if now, w/o addressing the return value structure at the same time? (I'm interpreting them as related)

If not, would you prefer that I punt both to v2 or address both here?

addressed in 27392cb944bc4

Shadowfiend · 2019-06-20T13:44:27Z

scripts/zoom.js

+              robot.logger.info(
+                `Stopped watching, meeting still going: ${meeting.id}`,
+              )
+              return


Similarly, you don't need any of these returns as they are all at the end of their respective branches in the function.

Shadowfiend · 2019-06-20T13:44:59Z

scripts/zoom.js

+              util.inspect(err),
+            )
+            // We assume the meeting still happened, so reply (but without `@`)
+            res.send(`Don't forget to post meeting notes when your call ends!`)


I would include “Something went wrong watching the meeting; don't forget...” Otherwise there's an unexplained difference in behavior for the users!

kb0rg · 2019-06-20T19:00:29Z

@Shadowfiend I think I hit all the for-this-iteration notes, and moved the for-next-iteration notes into a new issue. Ready for re-review when you have a chance - let me know if you spot anything I missed!

Shadowfiend

Okay this looks good… Will wait to merge until we chat about announcing the change.

Shadowfiend reviewed Jun 14, 2019

View reviewed changes

env-var.list Show resolved Hide resolved

Shadowfiend reviewed Jun 14, 2019

View reviewed changes

scripts/zoom.js Outdated Show resolved Hide resolved

kb0rg commented Jun 14, 2019

View reviewed changes

lib/zoom.ts Outdated Show resolved Hide resolved

kb0rg commented Jun 14, 2019

View reviewed changes

scripts/zoom.js Outdated Show resolved Hide resolved

kb0rg commented Jun 14, 2019

View reviewed changes

scripts/zoom.js Outdated Show resolved Hide resolved

kb0rg force-pushed the post-zoom-meeting-notes branch from 52746ce to 4a374ef Compare June 14, 2019 23:18

Shadowfiend reviewed Jun 15, 2019

View reviewed changes

scripts/zoom.js Outdated Show resolved Hide resolved

kb0rg force-pushed the post-zoom-meeting-notes branch from bc64c1c to b2d2d3f Compare June 18, 2019 18:40

kb0rg added 3 commits June 19, 2019 13:30

Add zoom credential variables for local dev

426c86c

Add function to hit get meeting details endpoint

3836a6d

kb0rg force-pushed the post-zoom-meeting-notes branch from 1f16210 to 5d766f1 Compare June 19, 2019 20:35

kb0rg added 4 commits June 19, 2019 16:26

Clean up

3bddf35

Remove function param no longer needed Remove notes

Add prompt about unused meeting

bbeae69

Prep for deploy

3b0a734

Update constants values to reasonable defaults Remove debug logging

kb0rg changed the title ~~[WIP] Heimdall prompts users of the zoom command to post meeting notes after the meeting~~ Heimdall prompts users of the zoom command to post meeting notes after the meeting Jun 20, 2019

Shadowfiend requested changes Jun 20, 2019

View reviewed changes

kb0rg mentioned this pull request Jun 20, 2019

Follow up Heimdall-scheduled Zoom calls: v2 #62

Open

kb0rg added 3 commits June 20, 2019 11:22

Remove redundant returns

20c594c

Clarify variable names and user facing messaging

96f1b47

Clarify return values and clean up conditionals

27392cb

Shadowfiend approved these changes Jun 20, 2019

View reviewed changes

Shadowfiend merged commit 06f473b into master Jun 21, 2019

Shadowfiend deleted the post-zoom-meeting-notes branch June 21, 2019 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heimdall prompts users of the zoom command to post meeting notes after the meeting #56

Heimdall prompts users of the zoom command to post meeting notes after the meeting #56

kb0rg commented Jun 14, 2019 •

edited

Loading

kb0rg left a comment

kb0rg commented Jun 20, 2019

Shadowfiend commented Jun 20, 2019

Shadowfiend left a comment

Shadowfiend Jun 20, 2019

kb0rg Jun 20, 2019

Shadowfiend Jun 20, 2019

kb0rg Jun 20, 2019

kb0rg Jun 20, 2019

Shadowfiend Jun 20, 2019

kb0rg Jun 20, 2019

kb0rg Jun 20, 2019 •

edited

Loading

Shadowfiend Jun 20, 2019

kb0rg Jun 20, 2019

Shadowfiend Jun 20, 2019

Shadowfiend Jun 20, 2019

Shadowfiend Jun 20, 2019

kb0rg Jun 20, 2019

kb0rg Jun 20, 2019

kb0rg Jun 20, 2019

Shadowfiend Jun 20, 2019

Shadowfiend Jun 20, 2019

kb0rg commented Jun 20, 2019

Shadowfiend left a comment

Heimdall prompts users of the zoom command to post meeting notes after the meeting #56

Heimdall prompts users of the zoom command to post meeting notes after the meeting #56

Conversation

kb0rg commented Jun 14, 2019 • edited Loading

kb0rg left a comment

Choose a reason for hiding this comment

kb0rg commented Jun 20, 2019

Shadowfiend commented Jun 20, 2019

Shadowfiend 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

kb0rg Jun 20, 2019 • edited Loading

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

kb0rg commented Jun 20, 2019

Shadowfiend left a comment

Choose a reason for hiding this comment

kb0rg commented Jun 14, 2019 •

edited

Loading

kb0rg Jun 20, 2019 •

edited

Loading