Add support for Talk Sidebar in public share pages #2107
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danxuliu
added
2. developing
enhancement
feature: talk-sidebar ⬅️
Member
|
18? |
Member
Author
|
Once I finish the UI the other pending issues should be easy and quick to fix once we agreed on how to proceed, so I would try to get this for 17 too to make @jospoortvliet happy :-P We can always defer it for 18 if turns out to be more difficult than expected. |
danxuliu
force-pushed
the
add-support-for-talk-sidebar-in-public-share-pages
branch
from
8e68917 to
21fac18
Compare
Member
Author
|
Once #2220 is merged this can be rebased and reviewed. There are still two pending issues, but they should be handled in follow up pull requests (and, most likely, for a later release): do not permanently join the room when the user accesses it through the public share page (although it should be discussed whether that is needed or not) and provide buttons to open and close the sidebar. |
Member
|
To the rebase maschine 🤖 |
Until now file rooms were available only to users with direct access to the file. Now file rooms are available to any user or guest too if the link is publicly shared (with a link share, for example). Public shares are identified by a share token instead of a file id, so a new endpoint, which is a counterpart of FilesController but for share tokens, was added. The file room, however, is still associated to the file id like before. When checking if a participant can join a room if the current user is a user without direct access to the file or a guest it is not even possible to know if the file id belongs to a publicly shared file. Due to this when the room is got for a share token the share token is stored in the session and then used in following requests when checking whether the participant can join a room or not. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Self-joined users and guests can join the room for a file if the file is shared by link. In order to check that, however, the share token should have been previously stored in the session, as the room is linked to the file id and users without direct access to a file can not find out if the file is shared by link or not. Therefore self-joined users and guests must get the room for the share (which stores the share token in the session) before being able to join the room. Besides that, in the case of self-joined users they must be logged in too. Otherwise the session is regenerated on each new request, which prevents getting the share token stored in a previous request. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
File rooms were available only to users with direct access to the file, so those were the users that could be mentioned. Now file rooms are publicly available if the file is shared by link, so the mentions now include all the users with direct access to the file like before plus all the self-joined users and guests currently in the room. Note, however, that self-joined users and guests can still not mention users with direct access to the file that have not joined the room yet, but only those that joined the room already (plus any participant that is currently in the room). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The absolute position causes the avatar container to have a different height when the contained div has a text (guest avatars) or not (user avatars, which use an image); now the flex property is propagated, which causes the container to always have the same size as the contained div. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When the public share page is loaded "publicshare.js" is initialized, which modifies the page to add a Talk sidebar. The default layout has the header, content and footer in a flex column; when the sidebar is added the layout id modified to still have the header and content in a flex column, but the content is now a flex row that includes "#app-content" and the sidebar, and the footer is moved inside "#app-content" so it does not affect the sidebar. The Talk sidebar includes a call container at the top, which is only shown during calls, and below it a call button and a chat view which are always shown. The CSS styles are a mix of the styles for the public share auth page and the Files app, as well as some rules copied from the main "style.scss" file. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The "I see that the current page is the shared link I wrote down" step is defined in the server but needs to be extended in Talk to also set (in the tests) the Talk sidebar as the ancestor of the chat view. In order to extend a step the whole Context that it belongs to must be extended. Note, however, that the step in the child Context should not include any "Given", "When" or "Then" annotation, as it would clash with the step defined in the parent Context (the "Override" annotation is simply informative, as it is ignored by Behat); as long as the method has the same signature the step from the child Context will be used instead if the step from the parent Context. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
…rectly. This prevents joining the room for a file shared by link and protected by password if the password has not been entered yet. Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a user with access to a file joins its file room the user is added as a persistent participant, so a "XXX joined the conversation" system message is shown. However, if a user does not have direct access to a file and joins its room the user is not added as a persistent participant, so the system message should not be shown in that case. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
"OC.getCurrentUser()" gets its returned value from the "data-user" and "data-user-displayname" attributes of the "head" element of the page, which in some cases may not be the correct ones for Talk (for example, in the public share page, as it uses the incognito mode which hides the current user). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The public share page uses the incognito mode, so "OC.getCurrentUser()" never returns a user, even if the user is actually logged in. However, Talk API requests honour the actual current user, so the Talk UI needs to honour it as well. Moreover, when the external signaling server is used, the Talk UI needs to honour the current user not only to show the right user name and avatar, but to be able to even connect with the external signaling server. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The default avatar size in VideoView is 128px, but due to the limited space available in the public share page it is reduced to 64px in the CSS. However, as "imageplaceholder()" is called while the element is still hidden it does not get the size set in CSS, but the one given to the function. This causes a mismatch in the text size once the element is shown with a expected size of 128px on an avatar of only 64px, so the same size should be used in both cases. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
force-pushed
the
add-support-for-talk-sidebar-in-public-share-pages
branch
from
21fac18 to
891a73f
Compare
Member
|
Looks good in general and the comments here are low priority. The only thing I think we should change is: |
delete-merged-branch
bot
deleted the
add-support-for-talk-sidebar-in-public-share-pages
branch
Member
|
/backport to stable17 |
|
The backport to stable17 failed. Please do this backport manually. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adds the Talk sidebar to the public share page, so users and guest can now chat and have calls when opening a link share.
This pull request is not finished yet. Here are the missing things:
The UI is not finished yet; besides the code being a copy/paste mess full of commented lines, there are issues like breaking the public share page layout or starting to load the chat messages before the sidebar has been fully shown, which breaks the visibility calculation of the chat messages and makes the chat view work strangely.
More importantly, the chat view does not work properly in the public share page for registered users when the internal signaling server is used (this can be seen in the failing acceptance tests), and it does not work at all when the external signaling server is used. The problem is similar to Wrong signaling ticket on video verification for logged in users #1537, but with a little quirk; even if the templates are modified to include the current user in the public layout it will still not work, because the ShareController enables the incognito mode in the public share page, so no user is returned by
OC_Userand thus the template will contain no user anyway.This could be fixed by applying Fix OC.getCurrentUser() on guest pages server#14178 to public pages too and removing the incognito mode for public share pages. However... I guess that the incognito mode is there for a reason :-P
A different option (Edit: the one finally used) would be to fix it fully in Talk, without touching the server, by adding and endpoint to get the current user (or returning it along with the room token when calling the endpoint for
PublicShareController::getRoom()) and then change all uses ofOC.getCurrentUser()for a wrapper that checks if a user was explicitly got and if not fallbacks toOC.getCurrentUser(). @nickvergessen @rullzer Opinions on that? Any other idea is also welcome, of course ;-)Another minor issues is that when a user with access to the file but who has not joined its room yet opens the public share page she is added as a participant of the room (and the "XXX joined the conversation" message is shown). If the user is joining through the public share page instead of through the Files app maybe she should be seen as a self-joined user and thus not join permanently the room?
Only tested with link shares; not tested with mail shares yet or any other share that uses a token.
Other things to consider for follow-up pull requests: