Skip to content

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Sep 29, 2020

Fixes #2139

@nickvergessen nickvergessen added 3. to review bug feature: chat 💬 Chat and system messages feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Sep 29, 2020
@nickvergessen nickvergessen added this to the 💚 Next Major (21) milestone Sep 29, 2020
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.

Tested with group folders and external storages and mentions now work.

However, there is a problem with external storages. After mentioning a user with access to the file through the external storage, but not through a share, the mentioned user will not be able to join the conversation (there will be an error and the conversation will be removed from the list).

Either mentioning other users with access to the file should be limited only to group folders or something similar to #2102 should be done for external storages too.

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.

Integration tests are not happy.

@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-to-mention-groupfolder-users-in-file-chats branch from d72e435 to ecb30e5 Compare November 16, 2020 12:40
@nickvergessen
Copy link
Member Author

Rebased as CI result was purged

@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-to-mention-groupfolder-users-in-file-chats branch from ecb30e5 to ddb20b9 Compare December 3, 2020 15:53
@nickvergessen
Copy link
Member Author

Rebased once more, lets see

@nickvergessen
Copy link
Member Author

Fixed the integration test and rebased

@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-to-mention-groupfolder-users-in-file-chats branch from bc1454d to 8c13370 Compare April 7, 2021 12:08
@nickvergessen
Copy link
Member Author

Three last failing tests:

/drone/server/apps/spreed/tests/integration/features/conversation/files.feature:223
/drone/server/apps/spreed/tests/integration/features/conversation/files.feature:450
/drone/server/apps/spreed/tests/integration/features/conversation/files.feature:476

They all 3 assume that the owner of a file can no longer access a room of a file they joined before when there is no share anymore. From my POV that is not the expected behaviour and with default expiration date can cause unintended information loss. I would therefor change the expected result.
Did I misunderstand something? @danxuliu

@danxuliu
Copy link
Member

danxuliu commented Apr 8, 2021

Yes, currently if a share is removed the participants can no longer access the associated room.

From my point of view that is the right behaviour, though :-) After all the room is linked to a file, so if the user can no longer access the file I would not expect the user to be able to access the room either.

Having said that, yes, if a share expires that could lead to information loss. Maybe if a share is deleted/expired the room should be innaccessible for any other participant except the owner, just like the file itself? Although if the owner never joined the room that could be a problem... 🤔

@nickvergessen
Copy link
Member Author

From my point of view that is the right behaviour, though :-) After all the room is linked to a file, so if the user can no longer access the file I would not expect the user to be able to access the room either.

Well the test cases we talk about are from owner side, so they can still access the file.

@danxuliu
Copy link
Member

danxuliu commented Apr 8, 2021

From my point of view that is the right behaviour, though :-) After all the room is linked to a file, so if the user can no longer access the file I would not expect the user to be able to access the room either.

Well the test cases we talk about are from owner side, so they can still access the file.

Right, I missed the owner part 🤦 Fine by me then.

@nickvergessen
Copy link
Member Author

Updated the tests

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 adjusted again the integration tests, as the tests for non-owner users no longer with access to the file (participant2) are already covered in their own scenarios. I also moved again "joins room" after "deletes shares" in the first scenario, as that ensures that the owner will be able to join the room after the share is deleted even if she was not in the conversation before.

Independently of that, I found an issue:

  • As an admin, add user A and user B to a group
  • As an admin, create an external storage for that group (and allow sharing in the external storage)
  • In the regular window, log in as user A
  • Open the Files app
  • Open the shared folder and create a file
  • In a private window, log in as user B (this is needed for the mount cache to be updated)
  • In the regular window, reload the folder
  • In the Chat tab for the new file, start/join the conversation
  • Mention user B
  • In the Sharing tab, share the file by link
  • In the private window, open Talk and join the conversation
  • As an admin, remove user B from the group
  • In the private window, reload Talk and try to join the conversation again

The user will be kicked out from the conversation. However, if the shared link is opened user B will be able to join the conversation in the sidebar of the share page, and it will be possible to open the conversation directly from Talk from that point on. If instead of an external storage a group folder is used the same problem appears (although I would swear that I also saw it working with a group folder 🤷).

This should not be a very common scenario, though, so fine by me to move it to an issue and fix it at a later point.

@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-to-mention-groupfolder-users-in-file-chats branch from 0a55d06 to f212cb2 Compare April 13, 2021 08:17
@nickvergessen
Copy link
Member Author

Tests passed, fixing up

@nickvergessen nickvergessen requested a review from danxuliu April 13, 2021 08:17
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.

Tested and works 👍 (except for the issue mentioned before, and another small issue mentioned below)

Besides the mentions, file rooms also work now for files in external storages 🎉

One detail worth noting (not a blocker, only something to keep in mind): if a user who is not logged in is added to a group that user will not appear in the mentions until the user logs in again. The opposite is true if the user is removed, that user will still appear as a candidate in the mentions until the user logs in again.

I guess this happens due to the use of the user mount cache, which I think that will not get updated until the file system is mounted again for the user.

Similarly, the need for a cache update might cause too some unexpected behaviours for freshly created external storages and group folders, like the storage/folder not being seen as accessible by a second user and thus not being able to start/join a conversation in a file until that second user reloads the page after the storage/folder was created and the file was added to it 🤷

Probably there is nothing that can be done about that, but I have no idea.

@nickvergessen
Copy link
Member Author

That's acceptable and also happens with activities and file comments 🥴

@nickvergessen nickvergessen merged commit 378fa07 into master Apr 13, 2021
@nickvergessen nickvergessen deleted the bugfix/noid/allow-to-mention-groupfolder-users-in-file-chats branch April 13, 2021 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review bug feature: chat 💬 Chat and system messages feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Users in a group folder can not be mentioned in File rooms

3 participants