kredits/hubot-kredits
9
0
Fork 0
Code Issues 8 Pull Requests Releases 9 Activity

Add zoom meeting whitelist #56

Merged
bumi merged 6 commits from feature/zoom-meeting-whitelist into master 2020-05-18 08:43:26 +00:00
Conversation 13 Commits 6 Files Changed 2 +32 -2
bumi commented 2020-04-30 14:22:23 +00:00 (Migrated from github.com)
Copy Link

This allows to only record meetings for certain whitelisted meeting ids.

[closes: #55 ]

This allows to only record meetings for certain whitelisted meeting ids. [closes: #55 ]
raucao (Migrated from github.com) reviewed 2020-04-30 17:52:23 +00:00
integrations/zoom.js
raucao (Migrated from github.com) commented 2020-04-30 17:52:23 +00:00
Copy Link

While we're adding untested code, how about this? 😃

        process.env.KREDITS_ZOOM_MEETING_WHITELIST?.split(',').includes(object.id)
While we're adding untested code, how about this? :smiley: ```suggestion process.env.KREDITS_ZOOM_MEETING_WHITELIST?.split(',').includes(object.id) ```
raucao (Migrated from github.com) reviewed 2020-05-07 12:25:46 +00:00
integrations/zoom.js
raucao (Migrated from github.com) commented 2020-05-07 12:25:46 +00:00
Copy Link

@bumi Did you see this?

@bumi Did you see this?
bumi (Migrated from github.com) reviewed 2020-05-14 07:50:32 +00:00
integrations/zoom.js
bumi (Migrated from github.com) commented 2020-05-14 07:50:32 +00:00
Copy Link

actually that does not work for me:

process.env.KREDITS_ZOOM_MEETING_WHITELIST?.split(',').includes(object.id)
SyntaxError: Unexpected token '.'

(the . after the ?)

how does that ? work?

actually that does not work for me: ``` process.env.KREDITS_ZOOM_MEETING_WHITELIST?.split(',').includes(object.id) SyntaxError: Unexpected token '.' ``` (the `.` after the `?`) how does that `?` work?
galfert (Migrated from github.com) reviewed 2020-05-14 09:46:40 +00:00
integrations/zoom.js
galfert (Migrated from github.com) commented 2020-05-14 09:46:40 +00:00
Copy Link

It's called optional chaining: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

In Nodejs it's only supported since version 14.

But in this case, it would change the existing logic. It would not call handleZoomMeetingEnded() if there was no KREDITS_ZOOM_MEETING_WHITELIST environment variable defined, while the existing code would still call the function.

It's called optional chaining: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining In Nodejs it's only supported since version 14. But in this case, it would change the existing logic. It would not call `handleZoomMeetingEnded()` if there was no `KREDITS_ZOOM_MEETING_WHITELIST` environment variable defined, while the existing code would still call the function.
raucao (Migrated from github.com) reviewed 2020-05-14 09:58:18 +00:00
README.md
raucao (Migrated from github.com) commented 2020-05-14 09:58:18 +00:00
Copy Link
The Zoom integration creates contributions for meeting participations.
```suggestion The Zoom integration creates contributions for meeting participations. ```
raucao (Migrated from github.com) reviewed 2020-05-14 09:59:11 +00:00
README.md
raucao (Migrated from github.com) commented 2020-05-14 09:59:10 +00:00
Copy Link
A Zoom JWT app has to be set up and an [event webhook subscription](https://marketplace.zoom.us/docs/api-reference/webhook-reference/meeting-events/meeting-ending")
```suggestion A Zoom JWT app has to be set up and an [event webhook subscription](https://marketplace.zoom.us/docs/api-reference/webhook-reference/meeting-events/meeting-ending") ```
raucao (Migrated from github.com) reviewed 2020-05-14 10:00:51 +00:00
integrations/zoom.js
raucao (Migrated from github.com) commented 2020-05-14 10:00:51 +00:00
Copy Link

True. I missed the functional change.

True. I missed the functional change.
bumi (Migrated from github.com) reviewed 2020-05-14 10:12:54 +00:00
integrations/zoom.js
bumi (Migrated from github.com) commented 2020-05-14 10:12:54 +00:00
Copy Link

ah, thanks. good to know.

ah, thanks. good to know.
bumi commented 2020-05-18 07:33:28 +00:00 (Migrated from github.com)
Copy Link

mergeable?

mergeable?
raucao commented 2020-05-18 08:06:51 +00:00 (Migrated from github.com)
Copy Link

Did you at least run it once locally?

Did you at least run it once locally?
bumi commented 2020-05-18 08:32:06 +00:00 (Migrated from github.com)
Copy Link

yes, looked fine for me with manually calling the endpoint.
(and then removed the WIP/untested comment here)

yes, looked fine for me with manually calling the endpoint. (and then removed the WIP/untested comment here)
👍 1
raucao commented 2020-05-18 08:42:43 +00:00 (Migrated from github.com)
Copy Link

Great. Let's try it in prod then.

Great. Let's try it in prod then.
raucao (Migrated from github.com) approved these changes 2020-05-18 08:42:53 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
raucao
Labels
Clear labels
bug
Something is not working
dev environment
Config, builds, CI, deployment, etc.
docs
Documentation
duplicate
This issue or pull request already exists
enhancement
Improving existing functionality
feature
New functionality
idea
Something to consider
invalid
Not a bug
kredits-1
Small contribution
kredits-2
Medium contribution
kredits-3
Large contribution
question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
ui/ux
User interface, process design, etc.
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
bkero bumi fsmanuel galfert greg haythem hueso raucao slvrbckt
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/hubot-kredits#56
No description provided.
Delete Branch "feature/zoom-meeting-whitelist"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?