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

Zoom integration #53

Merged
bumi merged 11 commits from feature/zoom into master 2020-04-16 15:34:39 +00:00
Conversation 11 Commits 11 Files Changed 2
+104 -3
bumi commented 2020-04-15 19:47:40 +00:00 (Migrated from github.com)
Copy Link
Copy Source

closes #34

closes #34
raucao (Migrated from github.com) reviewed 2020-04-16 09:40:33 +00:00
raucao (Migrated from github.com) left a comment
Copy Link
Copy Source

LGTM 👍

Added some code suggestions.

LGTM :+1: Added some code suggestions.
integrations/zoom.js
raucao (Migrated from github.com) commented 2020-04-16 09:21:06 +00:00
Copy Link
Copy Source
  const { Contributor, Contribution } = kredits;
```suggestion const { Contributor, Contribution } = kredits; ```
raucao (Migrated from github.com) commented 2020-04-16 09:23:48 +00:00
Copy Link
Copy Source
        const contribution = {
```suggestion const contribution = { ```
raucao (Migrated from github.com) commented 2020-04-16 09:26:43 +00:00
Copy Link
Copy Source

This is a bit of an issue, because we sometimes use the Weekly Call room for other meetings. Not sure what the best solution is. Maybe just something generic like "Team/Community Call".

This is a bit of an issue, because we sometimes use the Weekly Call room for other meetings. Not sure what the best solution is. Maybe just something generic like "Team/Community Call".
raucao (Migrated from github.com) commented 2020-04-16 09:37:06 +00:00
Copy Link
Copy Source
      robot.logger.info(`[hubot-kredits] Ignoring zoom call ${data.uuid} (duration: ${meetingDetails.duration}, participants_count: ${meetingDetails.participants_count})`);
```suggestion robot.logger.info(`[hubot-kredits] Ignoring zoom call ${data.uuid} (duration: ${meetingDetails.duration}, participants_count: ${meetingDetails.participants_count})`); ```
raucao (Migrated from github.com) commented 2020-04-16 09:38:17 +00:00
Copy Link
Copy Source
          robot.logger.info(`[hubot-kredits] Contribution created: ${tx.hash}`);
```suggestion robot.logger.info(`[hubot-kredits] Contribution created: ${tx.hash}`); ```
raucao (Migrated from github.com) commented 2020-04-16 09:39:58 +00:00
Copy Link
Copy Source
  robot.router.post('/incoming/kredits/zoom/'+process.env.KREDITS_WEBHOOK_TOKEN, (req, res) => {
```suggestion robot.router.post('/incoming/kredits/zoom/'+process.env.KREDITS_WEBHOOK_TOKEN, (req, res) => { ```
raucao commented 2020-04-16 09:41:28 +00:00 (Migrated from github.com)
Copy Link
Copy Source

By the way, I just checked Rinkeby kredits, and none of our profiles have Zoom display names stored yet...

By the way, I just checked Rinkeby kredits, and none of our profiles have Zoom display names stored yet...
bumi commented 2020-04-16 10:31:53 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Yes, we need to collect all the zoom names.
but we also need to test that four our kosmos call we get the correct participants.

Yes, we need to collect all the zoom names. but we also need to test that four our kosmos call we get the correct participants.
raucao commented 2020-04-16 10:38:41 +00:00 (Migrated from github.com)
Copy Link
Copy Source

but we also need to test that four our kosmos call we get the correct participants.

Not sure what you mean by "test". The participants will only ever be whatever their current Zoom display name is. Hence, the integration needs to ignore non-existing ones I believe. It will be a normal situation to not have all participants use a name, or have the right in their profile (or have a profile at all).

Edit: I also forgot to comment on the code: we established that re-connects add the same user multiple times to the array of the Zoom call object, so we need to use unique values in order to prevent multiple contributions from being created.

> but we also need to test that four our kosmos call we get the correct participants. Not sure what you mean by "test". The participants will only ever be whatever their current Zoom display name is. Hence, the integration needs to ignore non-existing ones I believe. It will be a normal situation to not have all participants use a name, or have the right in their profile (or have a profile at all). Edit: I also forgot to comment on the code: we established that re-connects add the same user multiple times to the array of the Zoom call object, so we need to use unique values in order to prevent multiple contributions from being created.
bumi commented 2020-04-16 11:44:33 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I could not test the full integration yet, because I can not get data from the past meeting participants endpoint.
that's for paid plans only and I had issues with the JWT. That's what we need to pair on using the kosmos zoom account.

I could not test the full integration yet, because I can not get data from the [past meeting participants](https://marketplace.zoom.us/docs/api-reference/zoom-api/meetings/pastmeetingparticipants) endpoint. that's for paid plans only and I had issues with the JWT. That's what we need to pair on using the kosmos zoom account.
raucao commented 2020-04-16 13:52:09 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Ah, OK. Got it.

Ah, OK. Got it.
raucao (Migrated from github.com) reviewed 2020-04-16 15:11:07 +00:00
integrations/zoom.js
@@ -0,0 +16,4 @@
const walletTransactionCount = await kredits.provider.getTransactionCount(kredits.signer.address);
let nonce = walletTransactionCount;
async function createContributionFor (displayName, meeting) {
raucao (Migrated from github.com) commented 2020-04-16 15:11:07 +00:00
Copy Link
Copy Source

I think we should not do this. It will be a normal thing in a public call that not everyone is a contributor with a profile.

I think we should not do this. It will be a normal thing in a public call that not everyone is a contributor with a profile.
raucao (Migrated from github.com) reviewed 2020-04-16 15:13:31 +00:00
integrations/zoom.js
@@ -0,0 +72,4 @@
if (meetingDetails.duration < 15 || meetingDetails.participants_count < 3) {
robot.logger.info(`[hubot-kredits] Ignoring zoom call ${data.uuid} (duration: ${meetingDetails.duration}, participants_count: ${meetingDetails.participants_count})`);
return;
raucao (Migrated from github.com) commented 2020-04-16 15:13:31 +00:00
Copy Link
Copy Source

What's the case of tx being falsey (maybe makes sense to explain that in a comment)? Btw, I think we can replace the then with saving the result in a variable from the await, like so:

const tx = await createContributionFor(displayName, meetingDetails);
if (tx) log(foo);
What's the case of tx being falsey (maybe makes sense to explain that in a comment)? Btw, I think we can replace the `then` with saving the result in a variable from the `await`, like so: ```js const tx = await createContributionFor(displayName, meetingDetails); if (tx) log(foo); ```
bumi (Migrated from github.com) reviewed 2020-04-16 15:15:30 +00:00
integrations/zoom.js
@@ -0,0 +72,4 @@
if (meetingDetails.duration < 15 || meetingDetails.participants_count < 3) {
robot.logger.info(`[hubot-kredits] Ignoring zoom call ${data.uuid} (duration: ${meetingDetails.duration}, participants_count: ${meetingDetails.participants_count})`);
return;
bumi (Migrated from github.com) commented 2020-04-16 15:15:30 +00:00
Copy Link
Copy Source

not sure how to best handle this one: https://github.com/67P/hubot-kredits/pull/53/files#diff-74e18db4da1da4461c9ccb134c582a3eR26

if the contributor is not found then there is no tx.

not sure how to best handle this one: https://github.com/67P/hubot-kredits/pull/53/files#diff-74e18db4da1da4461c9ccb134c582a3eR26 if the contributor is not found then there is no tx.
raucao (Migrated from github.com) reviewed 2020-04-16 15:17:05 +00:00
integrations/zoom.js
@@ -0,0 +72,4 @@
if (meetingDetails.duration < 15 || meetingDetails.participants_count < 3) {
robot.logger.info(`[hubot-kredits] Ignoring zoom call ${data.uuid} (duration: ${meetingDetails.duration}, participants_count: ${meetingDetails.participants_count})`);
return;
raucao (Migrated from github.com) commented 2020-04-16 15:17:05 +00:00
Copy Link
Copy Source

Wait, if the contributor is not found, then the promise wouldn't resolve, would it? In that case it should reject.

Wait, if the contributor is not found, then the promise wouldn't resolve, would it? In that case it should reject.
raucao commented 2020-05-01 08:12:33 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Dang, it slipped through my review that the documentation for this feature was missing in the README. (If it's not documented, it's not a feature.)

Edit: created #57 for it.

Dang, it slipped through my review that the documentation for this feature was missing in the README. (If it's not documented, it's not a feature.) Edit: created #57 for it.
raucao added the kredits-2 label 2025-01-23 21:24:50 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
bumi
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 labels kredits-2
Milestone
No items
No Milestone
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/hubot-kredits#53