Protect group from license borrowing #630
Conversation
izendegi
force-pushed
the
ProtectedGroup
branch
5 times, most recently
from
fd5c702 to
5eee4e5
Compare
There was a problem hiding this comment.
Thank you for all of your timely updates! This is very nice and should help users who needs this functionality. After applying the suggested changes, please make sure that it is still working as expected on your system (as I believe you may be one of the users of this functionality).
|
Hi, @jrchamp We've been using the original code for more than a year but haven't tested these last changes extensively (I'm on a leave for a couple of weeks), so please give me some weeks to test them and refine if needed. |
|
Hi @izendegi The code looks good to us, so we'll wait for your testing to make sure this version is working for your system. Thank you for your efforts on this! |
|
Hi, @jrchamp I've already tested that the last changes work as expected and added a specific error to throw when there is no user to borrow the license from. |
izendegi
force-pushed
the
ProtectedGroup
branch
from
1fd44ef to
4bb1fb2
Compare
classes/webservice.php
Outdated
| if ($leastrecentlyactivepaiduserid) { | ||
| $this->make_call("users/$leastrecentlyactivepaiduserid", ['type' => ZOOM_USER_TYPE_BASIC], 'patch'); | ||
| } else { | ||
| throw new moodle_exception('errornousersfound', 'mod_zoom'); | ||
| } |
There was a problem hiding this comment.
I agree that it is good to check to match sure that we're making an API call with a good value. However, it doesn't look like this moodle_exception is being caught and it looks like that will prevent the meeting host from being able to join their meeting if there are no licenses available. Where I don't love the idea of failing silently, I do worry about preventing them (and only them) from being able to join the meeting.
There was a problem hiding this comment.
I added that exception in the first place because when that situation happens (there is no user to borrow the license from) another exception was already thrown when trying to get the PRO license and there isn't any available.
I agree that throwing an exception is not the best thing to do, so instead of that, I've added the control to try to get the PRO license only if there limit is not reached or if it has been removed from another user, does it make sense to you? Maybe adding a message when this happens could be a good thing to do...
izendegi
force-pushed
the
ProtectedGroup
branch
2 times, most recently
from
a3bcbf6 to
e6b13bb
Compare
Co-authored-by: Jonathan Champ <[email protected]>
Co-authored-by: Jonathan Champ <[email protected]>
|
I've also rebased this branch from upstream (v5.2.5) |
izendegi
force-pushed
the
ProtectedGroup
branch
from
e6b13bb to
ffd67cf
Compare
On this PR we