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

feat(recording) - add frontend support for recording consent #10633

Merged
merged 7 commits into from
Oct 20, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Oct 4, 2023

☑️ Resolves

  • Fix Frontend "Recording consent" #10409
  • Solution is based on MVP + V2 description (⏺️ Recording consent - Overview #10348 (comment))
    • Option to require consent globally in Admin settings
    • When consent is required 1:
      • Enforce the media settings dialog
      • Show a checkbox that needs ticking before moderator/user can start or join call
      • Moderator setting is visible but disabled
    • When consent is optional 2:
      • Moderator setting is enabled with "Off" and "Required"
      • Disabled, while call is currently on
    • If user accidentally joined without consent, will be blocked by API
  • Increase preview size in MediaSettings, align all content elements up to its width

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

Admin settings

image

Moderator settings

Off (hidden) Required (disabled) Configurable V2
image image image

User view (Media settings)

Off (hidden) Required (not checked) Required (checked)
image image image

🚧 Tasks

  • Adjust wording / styles
  • Test with recording package
  • Test frontend (enter in AdminSettings -> Recording backend credentials for working signaling server)

🏁 Checklist

@Antreesy Antreesy added this to the 💙 Next Major (28) milestone Oct 4, 2023
@Antreesy Antreesy self-assigned this Oct 4, 2023
@Antreesy Antreesy force-pushed the feat/10409/recording-consent-support branch 3 times, most recently from 4a15b7c to 39e8e67 Compare October 6, 2023 09:03
@Antreesy Antreesy marked this pull request as ready for review October 6, 2023 09:03
@Antreesy
Copy link
Contributor Author

Antreesy commented Oct 6, 2023

cc @marcoambrosini for design review and wording for MVP + V2 solution
cc @danxuliu as it touches signaling, to ensure it won't break anything

@Antreesy Antreesy force-pushed the feat/10409/recording-consent-support branch from 6884314 to 83adcdc Compare October 6, 2023 13:31
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

The wording seems ok, I'd style the component a bit differently, similar to the quote component:

  • border instead of background (better text contrast)
  • Rounded corners
  • Rounded warning bar on the left
  • No warning icon
  • Same width as the video component above

Quick sketch:
IMG_8921 2

@nickvergessen
Copy link
Member

I'd style the component a bit differently

It's the wode spread NcNoteCard component. If that should be restyled, I'd should be done upstream I guess.
The usecase is exactly what the component was designed for 🙈

@Antreesy

This comment was marked as resolved.

Copy link
Member

@danxuliu danxuliu 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 two cases in which joining a call fails if recording consent is enabled: switching between breakout rooms and requesting the password of a share (video verification). In those cases the recording consent is not asked, so the API prevents the user from joining the call.

How to test (scenario 1):

  • Open Talk settings
  • Enable recording consent for all calls
  • Create a new group conversation
  • Add another user
  • Set up breakout rooms with 2 rooms
  • Start a call in that conversation; as it is the first time that a call is started the media settings screen will be shown
  • Give recording consent
  • Start the actual call
  • In a private window, log in as the other user
  • Join the conversation
  • Join the call; as it is the first time that a call is started the media settings screen will be shown
  • Give recording consent
  • Join the actual call
  • In the original window, start the breakout rooms session

Expected result

The user switches to the call in the breakout room (I would say that asking for recording consent again would be overkill and that the recording consent from the parent room should be reused; maybe this could be even done in the API itself)

Actual result

The user switches to the call in the breakout room, but is immediately kicked out

How to test (scenario 2):

  • Open Talk settings
  • Enable recording consent for all calls
  • Open the Files app
  • Create a link share
  • Protect the share with a password
  • Enable video verification
  • Copy the link
  • In a private window, open the link
  • Request the password
  • Set a name for the guest

Expected result

The media settings screen (or just a recording consent dialog, but I guess it would be better to reuse the media settings screen) is shown to give recording consent

Actual result

The user seems to join the call but is immediately kicked out

Independently of that, the media settings screen should be shown even if the user unchecked Always show preview for this conversation when recording consent needs to be given to join a call:

How to test

  • Open Talk settings
  • Enable recording consent for all calls, or make it optional and enable it in a specific call
  • Create a new conversation, and if recording consent is optional, enable it
  • Start a call in that conversation; as it is the first time that a call is started the media settings screen will be shown
  • Uncheck Always show preview for this conversation
  • Give recording consent
  • Start the actual call
  • Leave the call
  • Reload the page
  • Start a call

Expected result

The media settings screen is shown to give recording consent

Actual result

The user seems to join the call but is immediately kicked out

@nickvergessen
Copy link
Member

Case 1 (sending to breakout room) I guess we can fix on the API level by ignoring the setting there and basically allow "sending users" there. However when stopping breakout rooms they would join the parent again in which case we can not detect on API level if that is happening, so we would require the client to send the consent flag there again.

@nickvergessen
Copy link
Member

However when stopping breakout rooms they would join the parent again in which case we can not detect on API level if that is happening, so we would require the client to send the consent flag there again.

But that is also a bit annoying, because it will be listed as another consent entry, but I guess it's the best step we have atm

@nickvergessen
Copy link
Member

The reconnection when your permissions are modified is also affected?

@danxuliu
Copy link
Member

The reconnection when your permissions are modified is also affected?

Good catch. Yes, reconnections do not work because the consent is not sent again. The silent parameter is not taken into account either, I will check tomorrow as that might need a separate fix to be able to backport it (and therefore this pull request would need a rebase on that fix to be able to also fix renegotiations when consent is required).

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

@Antreesy I think the use of selects here is not appropriate.
I think that the admin settings should be radio buttons, with an explanation always shown and not in a warning badge.
On the other hand, for the moderators on a per-conversation basis, we should use a toggle.

@danxuliu
Copy link
Member

The silent parameter is not taken into account either, I will check tomorrow as that might need a separate fix to be able to backport it (and therefore this pull request would need a rebase on that fix to be able to also fix renegotiations when consent is required).

Fix for silent parameter is in #10688 The second commit can be used as a reference on what to change (after rebasing) to send the consent again on reconnections.

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

The media settings screen is not shown if the user unchecked Always show preview for this conversation when recording consent needs to be given. Please refer to the last How to test section in #10633 (review)

Independently of that, the consent sometimes is sent again and sometimes it is not when using breakout rooms (first How to test section in #10633 (review)). I have not been able to find a pattern, maybe it is related to fetching the room information to compare the parent room when switching to a different breakout room to clean or not to clean the consent, but I do not know.

When requesting the password of a share it might be good to leave the room if the media settings screen is closed without joining the call. Leaving the call or the conversation should update the sidebar and remove the call view and the chat and show This conversation has ended, although it seems that was lost at some point 🤔 As leaving the call does not currently do that I guess it would be fine to leave the conversation when the media settings is closed in a follow up once the existing code was also fixed.

To end in a bright note, the consent is now sent again in forced reconnections :-)

@Antreesy Antreesy force-pushed the feat/10409/recording-consent-support branch from 6ff4393 to 7f38eec Compare October 17, 2023 13:28
@Antreesy
Copy link
Contributor Author

...if the user unchecked Always show preview for this conversation

I might have lost these code lines... Moved logic from components to the store, to sync it properly between MediaSettings, ConversationSettingsDialog and CallButton (tests included) 🟢

...maybe it is related to fetching the room information

I have relied on breakoutRoomsStore, but it works differently for moderators and non-moderators. Fixed 🟢

Leaving the call or the conversation should update the sidebar and remove the call view and the chat and [show This conversation has ended]

with internal signling only, we fetch conversation within an interval, and show context you mentioned. but with HPB it wait for signaling message. Don't know the logic here (cc @nickvergessen), but it seems like when the share owner leaves conversation, it's removed on server side. So, we may track signaling message, that owner left the room, and fetch the conversation within some timeout => and if it was deleted, all should go back to normal => will raise a follow-up issue

Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

In case the consent is configurable on conversation level, I noticed when a moderator switches on the consent requirement while a participant has the conversation open ( on a chat view), it's not updated for them thus there is no consent requested when joining the call.

src/store/conversationsStore.js Show resolved Hide resolved
src/views/FilesSidebarChatView.vue Show resolved Hide resolved
@nickvergessen
Copy link
Member

In case the consent is configurable on conversation level, I noticed when a moderator switches on the consent requirement while a participant has the conversation open ( on a chat view), it's not updated for them thus there is no consent requested when joining the call.

Missing an update trigger via the HPB then, so it's a backend issue.

@nickvergessen
Copy link
Member

In case the consent is configurable on conversation level, I noticed when a moderator switches on the consent requirement while a participant has the conversation open ( on a chat view), it's not updated for them thus there is no consent requested when joining the call.

Also fixed in #10727 now

…g consent (MVP)

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
…for users (MVP)

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
…nversation (V2)

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the feat/10409/recording-consent-support branch from 192a519 to 0663481 Compare October 18, 2023 11:29
@Antreesy
Copy link
Contributor Author

Rebased onto master, should work for breakout rooms, and fetch conversation on change
Consent is kept for main view and sidebar integrations (turns out it's possible to promote people to moderators there with occ command at least)

Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Breakout rooms session should be closed when the moderator wants to toggle the consent requirement in the conversation settings . It renders Bad request otherwise ( maybe add a hint in toast ? )

Overall, nothing is blocking 🦅 .

@Antreesy Antreesy merged commit 49dc220 into master Oct 20, 2023
35 checks passed
@Antreesy Antreesy deleted the feat/10409/recording-consent-support branch October 20, 2023 14:13
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

(I did not see that the review was dismissed before actually testing the pull request 🤦)

Tested and works 👍

A minor detail: when the device selector is shown in request password/video verification clicking on Always show preview for this conversation or in the menu to start a silent call has no effect (the checkbox is not modified, and the menu is not opened).

Nevertheless, those options should not be even visible, because every time a password is requested again a new room will be created, so there is no point in remembering Always show preview for this conversation, and the call should not be silently started, as the owner of the file should be notified that a password is being requested.

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

Successfully merging this pull request may close these issues.

Frontend "Recording consent"
5 participants