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

Make recording of a session optional - Fix #1163 #1296

Merged
merged 2 commits into from
Jun 29, 2020

Conversation

yanosz
Copy link
Contributor

@yanosz yanosz commented Apr 14, 2020

Fix #1163 by making recording configurable

By this PR, recording is among the room-features configured in the sample env. Due to its privacy implications, it has to be enabled explicitly. In addition, the administator can configure recording-options using the frontend. Unless enabled explicitly, rooms are not recoreded. Note: This changes default behaviour.

If enabled, users can configure rooms to record session. This flag is set on a per-room basis and cannot be changed while the room is active (i.e. a session is running)

@coveralls
Copy link

coveralls commented Apr 14, 2020

Coverage Status

Coverage decreased (-0.03%) to 91.226% when pulling 0314762 on yanosz:master+1163 into 9a96df6 on bigbluebutton:v2.7-alpha.

@Arnei
Copy link

Arnei commented Apr 17, 2020

Hello,
Is this sketch already intended to be working? I ask because I tried this pull request, but it does not work for me. To be more specific, the new "Recording" room setting does not seem to have an effect on whether recording files are created by bigbluebutton or not (for example, files are created in /var/bigbluebutton/recording/raw no matter the setting). Or did I get something wrong?

@yanosz
Copy link
Contributor Author

yanosz commented Apr 17, 2020

Hello,
Is this sketch already intended to be working? I ask because I tried this pull request, but it does not work for me. To be more specific, the new "Recording" room setting does not seem to have an effect on whether recording files are created by bigbluebutton or not (for example, files are created in /var/bigbluebutton/recording/raw no matter the setting). Or did I get something wrong?

Heiho,

thanks for feedback and testing! Initially, I planed to do some testing, based on changes caused by #1222 and revisit this at this coming point in time. Basically, this patch was about prototyping the UX according to greenlight's architecture. It's my first PR for greenlight, anyway :-).

Nevertheless, I can do some debugging in case sbd. (@Arnei you?) is interested in using this patch as of now.

@yanosz
Copy link
Contributor Author

yanosz commented Apr 17, 2020

ok, CI is happy, parameters should be passed

@Arnei
Copy link

Arnei commented Apr 20, 2020

Ah, makes sense that it wouldn't prevent recording if it was just a UX prototype :D.

I've tried the newest commit, as it is labeled "pass parameter on creation", and now bigbluebutton won't create any archive files under /var/bigbluebutton/recording/rawno matter if the recording option was set to true or false on creation.

And thank you for working on this!

@yanosz yanosz force-pushed the master+1163 branch 3 times, most recently from 42976bd to 5f66565 Compare May 3, 2020 17:40
@zner0L
Copy link

zner0L commented May 4, 2020

@yanosz or anyone, can you tell me what's missing from this to be merged? I really need to be able to disable recordings, but if possible I would like to wait for a release before using these changes, but if it takes long I'll just patch it myself…

@yanosz
Copy link
Contributor Author

yanosz commented May 6, 2020

@zner0L Hard to say. There isn't much feedback from the maintainers, yet. I guess, that a fair amount of new comitters rushed into this project in the last days and things could take time.

I'd helpfull if you coud test the branch as of this PR, as of now and provide some feedback.

@farhatahmad
Copy link
Collaborator

Hi @yanosz,

Thanks for submitting the PR. The reason I haven't replied is because I'm still unsure with exactly how this should work. We haven't had the time internally to discuss this yet so I can't quite give a deadline on when this will be merged

@yanosz
Copy link
Contributor Author

yanosz commented May 7, 2020

Hei @farhatahmad

Thanks for submitting the PR. The reason I haven't replied is because I'm still unsure with exactly how this should work. We haven't had the time internally to discuss this yet so I can't quite give a deadline on when this will be merged

Thanks your time and answer! I'm also unsure, with exactly how this should work. In essence, greenlight promotes custimization, whereby regional peculiarities (such as the GDBR) can be with implemented very low effort. Nevertheless, the demand in #1163 appears to be sound. Also, I'm thinking, hat neither the architecture of greenlight, nor the UX for users and nor the complexity for customizers can reasonable address evovled, but local requirements such as GDPR-compliance, for certain. Should greenlight - as a project - implement various business use-cases and requirements or should it stay a minimalist appraoch for a bbb frontend, whereby other frontends exists for diverse use-cases? I'm not certain.

That being said, I'm also not certain, if GDPR-compliance actuallually justifies a fork of greenlight.

For the moment, it appears to reasonable to me to maintain this branch of greenlight and ask for testers.

@gilou
Copy link

gilou commented May 12, 2020

I might give it a go indeed, for legal and technical reasons, not enabling the recording by default is very important!

@Arnei
Copy link

Arnei commented May 15, 2020

Seems to be working mighty fine so far. Thanks again!

Things I found:

  1. Recording_SessionActive
    "Session Active" is displayed on room creation. It's not doing any harm, just confusing. Otherwise, "Session Active" is displayed correctly.

@yanosz
Copy link
Contributor Author

yanosz commented May 15, 2020

@Arnei Thanks for your feedback. I'll have a look at it this weekend.

@yanosz yanosz force-pushed the master+1163 branch 2 times, most recently from a59e7e2 to 28657ab Compare May 16, 2020 13:58
@Arnei
Copy link

Arnei commented May 19, 2020

Got another bug report:

Usually, when you try to join a conference that has not started yet, you get a message looking like so (I hope the german has no detrimental effect on the explanatory power of the image):
ConferenceHasntStartedYet

If I try to do the same thing in a room with recording enabled, after consenting, entering my name and clicking join, the consent button is displayed again:
ConferenceWaitError

The consent button will then stop doing anything useful, while the message "the conference hasn't started yet" won't ever be displayed. The message is hidden away in the span(28657ab#diff-9b2eed259771c1646a45c3e0ce623dfaR43).

@yanosz
Copy link
Contributor Author

yanosz commented May 20, 2020

@Arnei thanks for your feedback. I guess, that this is an interesting side effect of unstable DOM manipulations.

I think I'll ditch the cosent-button as explained in #1163 and have GDPR-related consent issues in #1619

@yanosz yanosz force-pushed the master+1163 branch 2 times, most recently from 7a7036e to 70e39f9 Compare May 21, 2020 12:53
@yanosz yanosz changed the title Note on recordings - prototype Make recording of a session optional - Fix #1163 May 21, 2020
@yanosz
Copy link
Contributor Author

yanosz commented May 21, 2020

I think, that this feature is ready for getting merged (sorry, for not marking this PR as draft before, I just discovered this functionality).

i'd be good to have some addtional, manual testing. To the best of my knowledge, there is no test automation for both the modal dialogs and the joiner aspect.

@basisbit: Ping.

@yanosz yanosz force-pushed the master+1163 branch 2 times, most recently from cddecd6 to 943822b Compare May 21, 2020 13:07
@basisbit
Copy link

basisbit commented May 28, 2020

@farhatahmad any chance to get this reviewed and merged soon-ish? About all BBB server providers who use Greenliight and need the recording feature in the EU are currently waiting for this to get merged (because of legal requirements) and so far have to manually patch every new release of greenlight.

Also, please take a look at how many people upvoted this issue.

@yanosz yanosz force-pushed the master+1163 branch 3 times, most recently from b50d33f to 01309a3 Compare May 29, 2020 08:43
@yanosz
Copy link
Contributor Author

yanosz commented May 29, 2020

Sorry for the spam ... I'was just trying to align the whitespace, when origins got messed up. It should be fine, again.

@basisbit
Copy link

basisbit commented Jun 5, 2020

yet another week without any reaction, not even a comment. nudge @farhatahmad ?

@farhatahmad
Copy link
Collaborator

@basisbit No need for the attitude :). My comment is the same as it's been since this topic was first brought up. You were also on the call last week when I mentioned that I plan to fully review and merge it into the next version of Greenlight (v2.7).

I've been focused on making v2.6.x stable due to the big changes that were made in that version. Would you prefer I leave the bugs and focus solely on this issue?

@yanosz I appreciate all the work you and everyone else put in to get this PR to the state it is now. I still plan on merging this once I can confirm the v2.6.x is stable enough so that I can begin working on v2.7

@matiasilva
Copy link

Great work, as always :)

config/application.rb Show resolved Hide resolved
sample.env Show resolved Hide resolved
@ichdasich
Copy link

I think the last item is for enabling that setting in the room configuration tab, no?

@farhatahmad farhatahmad changed the base branch from master to v2.7-alpha June 29, 2020 17:35
@farhatahmad farhatahmad merged commit 2e4010a into bigbluebutton:v2.7-alpha Jun 29, 2020
@farhatahmad
Copy link
Collaborator

Made some small changes/improvements in #1851. Feel free to let me know what you guys think :)

@sondos1992
Copy link

Hi
I've a question can i add automatic recording and download feature to BBB .

@basisbit
Copy link

basisbit commented Jul 5, 2020

@sondos1992 for support requests / such questions, please ask at the mailing list: https://groups.google.com/forum/#!forum/bigbluebutton-users

@sondos1992
Copy link

it keep showing error #254 connection to server can't join

@basisbit
Copy link

basisbit commented Jul 5, 2020

@sondos1992 for support requests / such questions, please ask at the mailing list: https://groups.google.com/forum/#!forum/bigbluebutton-greenlight

EmmyGraugans pushed a commit to EmmyGraugans/greenlight that referenced this pull request Jul 6, 2020
Co-authored-by: Ahmad Farhat <ahmad.af.farhat@gmail.com>
miztaka referenced this pull request Jul 21, 2020
* Small changes/improvements to the recording settings

* Replaced room warning with info flash

* Added global setting to enable/disable the recording consent feature
farhatahmad added a commit that referenced this pull request Jul 29, 2020
* Fix wrong conditional (reported by LGTM) (#1477)

Signed-off-by: Stefan Weil <sw@weilnetz.de>

Co-authored-by: Ahmad Farhat <ahmad.af.farhat@gmail.com>

* Bump rack from 2.2.2 to 2.2.3 (#1839)

Bumps [rack](https://github.com/rack/rack) from 2.2.2 to 2.2.3.
- [Release notes](https://github.com/rack/rack/releases)
- [Changelog](https://github.com/rack/rack/blob/master/CHANGELOG.md)
- [Commits](rack/rack@v2.2.2...2.2.3)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [FIX]  Unable to edit long recording names #1776  (#1780)

* Allow to set a filter for LDAP authentication

* [FIX] Unable to edit long recording names #1776

Co-authored-by: François Ménabé <francois.menabe@unistra.fr>
Co-authored-by: farhatahmad <ahmad.af.farhat@gmail.com>

* Desgin for Manage Users Tabs (#1777)

* Update _subtitle.html.erb

* Update _manage_users_tags.html.erb

* Update admins.scss

* Update _primary_themes.scss

* Update _manage_users_tags.html.erb

* Minor style changes to manage users (#1845)

* Maintenance banner moved to admin site (#1775)

* initial

* finish

* travis fixes

* travis again

* not required

* Co-authored-by: Tobias Fiebig <t.fiebig@tudelft.nl> (#1296)

Co-authored-by: Ahmad Farhat <ahmad.af.farhat@gmail.com>

* Enhance Room OpenGraph Metadata (#1601)

* Revert "Enhance Room OpenGraph Metadata (#1601)" (#1852)

This reverts commit 3b007c2.

* GRN2-xx: Tab title now displays the current page name (#1853)

* Tab title now displays the current page name

* Added page title for the rest of the pages

* Split Site Settings into 3 different tabs (#1858)

* Split Site Settings into 3 different tabs

* Fix copyright

* Added redirect to correct tab

* Make sure settings are displaying when they should

* Update en.yml (#1857)

* Build images for alpha branches (#1867)

* Upgraded jquery to latest version (#1896)

* Added favicon tag (#1898)

* Fixed XSS issue with role name (#1899)

* Update path for coloring redirect (#1908)

* Added a fourth section to the room uid (#1910)

* Fixed issue with insecure room sharing removal (#1914)

* Fixes typo (#1917)

Fixes typo: successfully was written incorrect.

* Fixed order of rooms in server rooms (#1915)

* Change default room sort to latest activity (#1919)

* GRN2-xx: Small changes/improvements to the recording settings (#1851)

* Small changes/improvements to the recording settings

* Replaced room warning with info flash

* Added global setting to enable/disable the recording consent feature

* Replace Legal with Terms (#1931)

* Added a more friendly OpenGraph description when invited to join a room (#1932)

* Fixed issue causing maintenance banner not to hide correctly (#1933)

* Hide recording menu and recording list when it is disabled (#1935)

* Hide recording menu and recording list when it is disabled

* Hide recording list when disabled

* GRN2-xx: Added an auto-refresh after 2 mins while waiting for room to start (#1947)

* Added an auto-refresh after 2 mins while waiting for room to start

* Fixed random issue with test case

* GRN2-xx: Added ability to preupload presentations to rooms (#1895)

* Added ability to preupload presentations to rooms (#1868)

* Added setting to site settings and allowed admins to change the presentation

* Added AWS S3 and GCS Storage ENV variables

* Added check to ensure file extension is correct

* Added icon to remove presentation

* Added testcases for preupload

* Add nginx redirect to solve issue with relative root

* Record title, instead of room name, in the popup (#1924)

* Update _public_recording_row.html.erb

* Update _recording_row.html.erb

Co-authored-by: Stefan Weil <sw@weilnetz.de>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: beckerr-rzht <beckerr@hochschule-trier.de>
Co-authored-by: François Ménabé <francois.menabe@unistra.fr>
Co-authored-by: MrKeksi <mrkeksi@users.noreply.github.com>
Co-authored-by: yanosz <yanosz@users.noreply.github.com>
Co-authored-by: Moritz Schlarb <moschlar@metalabs.de>
Co-authored-by: chronikum <34622984+chronikum@users.noreply.github.com>
Co-authored-by: Mitsutaka Sato <miztaka@honestyworks.jp>
Co-authored-by: hiroshisuga <45039819+hiroshisuga@users.noreply.github.com>
EmmyGraugans pushed a commit to EmmyGraugans/greenlight that referenced this pull request Jun 8, 2021
Co-authored-by: Ahmad Farhat <ahmad.af.farhat@gmail.com>
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.

[Feature Request] Make recording of a session optional