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

recording visible to user #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pooja418
Copy link
Contributor

@pooja418 pooja418 commented Sep 6, 2022

Recordes session acees right
using visible and read permission now user can see the recording links.

Copy link
Contributor

@xJuvi xJuvi left a comment

Choose a reason for hiding this comment

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

There are unclarified questions. I think this PR isn't complete.

@@ -16,12 +16,7 @@
$('#classNotStarted').show();
}

var isMeetingRecorded={isMeetingRecorded};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did u remove this?
This is the hint if the current running meeting is recorded or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes i forgot it the current meeting recording hint should be as it was before.By mistakenly in testing i removed it.


$bbbURL=$BBBHelper->joinURL($this->object);

$my_tpl->setVariable("recordings", $this->buildRecordingUI());
Copy link
Contributor

Choose a reason for hiding this comment

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

You should refactor the whole part. This is only copied fromt he "moderator" section. But in the moderator section, it will be checked, if int he currect BBB object recordings are allowed. This check is missing in your changed.

See:

if ($values["choose_recording"]){
$my_tpl->setVariable("recordings", $this->buildRecordingUI());
$my_tpl->setVariable("Headline_Recordings", $this->txt("Headline_Recordings"));
$my_tpl->setVariable("checkbox_record_meeting", $this->txt("checkbox_record_meeting"));
}else{
$my_tpl->setVariable("CHOOSE_RECORDING_VISIBLE", "hidden");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes in the moderator section, it will be checked, if the current BBB object recordings are allowed. This check is missing in changes because user can't allow for it.Only moderator or admins can allow for the changes to that checkbox if they record Current BBB object or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's right. But it also controls if the recordings ui is visible or no. With your change, users can see the recording ui when recordings in the bbb object are disallowed. So i think is should be checked if recordings are allowed.

@pooja418
Copy link
Contributor Author

Yes it should be like this in if check condition
if ($values["choose_recording"]){
$my_tpl->setVariable("recordings", $this->buildRecordingUI());
$my_tpl->setVariable("Headline_Recordings", $this->txt("Headline_Recordings"));
}

@pooja418
Copy link
Contributor Author

Hello @xJuvi ,
since it's been a long time.I didn't get any response or comment from your side on this pull request.
May i know what happened here?
If still it has something to do then let me know so i can go further with changes.

@xJuvi
Copy link
Contributor

xJuvi commented Feb 24, 2023

Hey @pooja418
please add your suggestion as commit to this PR and not as comment inline this.
Then I can better check if it's correct and useful.

Kind regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants