Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zoom use pro accounts first #165

Merged
merged 10 commits into from
Nov 14, 2019
Merged

Zoom use pro accounts first #165

merged 10 commits into from
Nov 14, 2019

Conversation

kb0rg
Copy link
Contributor

@kb0rg kb0rg commented Nov 11, 2019

Closes #164
Closes #127

This filters availableSessions into separate arrays for accounts of Basic vs Pro user type.

It uses Basic accounts only if no sessions are available with Pro accounts. When a basic account is used, the Heimdall zoom link reply includes messaging about the time limit.

Add new method to create account from user instead of just email, so
type is preserved

This is so we can filter by account type later in the process
I could filter here, in getting accounts from users, but that
restricts the number of available sessions

This way I can filter when returning availableSessions, giving
priority to pro users but falling back to basic user accounts if
needed
@kb0rg
Copy link
Contributor Author

kb0rg commented Nov 11, 2019

This is working locally; I've logged the userResponse.data.users, availableSessions, availableProSessions, availableProSessions, and finally candidateSessions -- and verified that only the type 2 accounts are showing up in candidateSessions.

However, these commits were made by bypassing the prettier hook. Attempts to commit with the hook keep failing with the following error:

[EDIT: disregard. the below error has been found and fixed]

[error] lib/zoom.ts: SyntaxError: ',' expected. (115:5)
[error]   113 |     )
[error]   114 | 
[error] > 115 |     const availableSessions = accountMeetings
[error]       |     ^
[error]   116 |       .filter(([, availableForMeeting]) => availableForMeeting)
[error]   117 |       .map(([session]) => session)

I'm having trouble finding what I did that could be causing the linter fail, and would appreciate another set of eyes.

We have at least one reported instance of a meeting being created
with an account that was already in use
There is not enough data to debug how or why this happened
In the unlikely but I guess technically feasible event that this was
caused by the user starting a meeting Just after the live meetings
check had been made, this change would improve chances of avoiding
the problem happening again
@kb0rg
Copy link
Contributor Author

kb0rg commented Nov 11, 2019

This also addresses Issue #127 -- though I'm honestly not sure if it really closes it. Thoughts?

Add missing closing paran to accountMeetings Array from line
Move question mark in Account type optional param definition
Prvious commits were all made with prettier disabled, because I had
not yet figured out what syntax error was breaking linting

The commit with the syntax error fixes is easier to read without also
including all the linting, so I disabled prettier on that commit as
well

Thus, the linting for the last few commits all in one swell foop
As a warning to future us, if we scale to having more than 30 zoom
user accounts
Note that there is a time limit, and offer workarounds

Inform them instead of annoying them... hopefully
@kb0rg kb0rg marked this pull request as ready for review November 12, 2019 00:49
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Left a couple of notes, this looks like it'll be a nice easy win.

lib/zoom.ts Outdated Show resolved Hide resolved
lib/zoom.ts Outdated Show resolved Hide resolved
lib/zoom.ts Show resolved Hide resolved
scripts/zoom.js Outdated Show resolved Hide resolved
@Shadowfiend
Copy link
Contributor

Re: issue 127, I'm okay with letting that one close with this, unless and until we run into it again.

Copy link
Contributor Author

@kb0rg kb0rg left a comment

Choose a reason for hiding this comment

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

Alright, ready for re-review @Shadowfiend - thanks for the notes!

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Notes are fixed, let's do this thing.

@Shadowfiend Shadowfiend merged commit b3e3143 into master Nov 14, 2019
@Shadowfiend Shadowfiend deleted the zoom-use-pro-accounts-first branch November 14, 2019 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants