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 +101
bumi commented 2020-04-15 19:47:40 +00:00 (Migrated from github.com)
Copy Link

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

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
  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
        const contribution = {
```suggestion const contribution = { ```
raucao (Migrated from github.com) commented 2020-04-16 09:26:43 +00:00
Copy Link

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

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

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

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

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

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

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

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

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

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

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
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
kredits-2
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#53
No description provided.
Delete Branch "feature/zoom"

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?