Skip to content

Don't search in lobbied conversations #8057

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.

Sign up for GitHub

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

Jump to bottom
Merged
nickvergessen merged 3 commits into from
Sep 30, 2022
Merged

Don't search in lobbied conversations #8057

nickvergessen merged 3 commits into from
Sep 30, 2022

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Sep 30, 2022

Fix #8054

cc @jld3103

@nickvergessen nickvergessen added 3. to review bug feature: chat 💬 Chat and system messages feature: api 🛠️ OCS API for conversations, chats and participants feature: meetings 📅 Covering the webinary usecase incl. Lobby feature: search 🔎 labels Sep 30, 2022
@nickvergessen nickvergessen added this to the 💟 Next Major (26) milestone Sep 30, 2022
@nickvergessen nickvergessen self-assigned this Sep 30, 2022
@nickvergessen
Copy link
Member Author

nickvergessen commented Sep 30, 2022 •
edited
Loading

/backport 0710aa0 to stable25

@nickvergessen
Copy link
Member Author

nickvergessen commented Sep 30, 2022 •
edited
Loading

/backport 0710aa0 to stable24

@nickvergessen
Copy link
Member Author

nickvergessen commented Sep 30, 2022 •
edited
Loading

/backport 0710aa0 to stable23

provokateurin
provokateurin reviewed Sep 30, 2022
View reviewed changes
lib/Search/MessageSearch.php
}

if ($room->getLobbyState() !== Webinary::LOBBY_NONE) {
$participant = $room->getParticipant($user->getUID(), false);
Copy link
Member

@provokateurin provokateurin Sep 30, 2022

Choose a reason for hiding this comment

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

Why is the order of different to the if clause above and split into two parts?

Copy link
Member Author

@nickvergessen nickvergessen Sep 30, 2022

Choose a reason for hiding this comment

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

because in the Current it's a one-time thing, so it does not matter as much, while here we would potentially set up your user in hundreds of conversations even if they don't have a lobby in the end.

vitormattos
vitormattos approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@vitormattos vitormattos left a comment

Choose a reason for hiding this comment

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

I think that is important to create an issue to implement integration tests covering this scenario

@nickvergessen nickvergessen force-pushed the bugfix/8054/dont-search-lobbied-conversations branch from 1493217 to 87f8ec1 Compare September 30, 2022 15:13
@nickvergessen nickvergessen merged commit 40bbcda into master Sep 30, 2022
@nickvergessen nickvergessen deleted the bugfix/8054/dont-search-lobbied-conversations branch September 30, 2022 20:13
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Sep 30, 2022
Closed
1 task
@nickvergessen
Copy link
Member Author

nickvergessen commented Oct 1, 2022

/backport to stable25

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Oct 1, 2022

The backport to stable25 failed. Please do this backport manually.

@nickvergessen
Copy link
Member Author

nickvergessen commented Oct 7, 2022

/backport to stable25

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Oct 7, 2022

The backport to stable25 failed. Please do this backport manually.

@nickvergessen
Copy link
Member Author

nickvergessen commented Oct 7, 2022

/backport 0710aa0 to stable25

@nickvergessen
Copy link
Member Author

nickvergessen commented Oct 7, 2022

/backport 0710aa0 to stable24

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Oct 7, 2022

The backport to stable25 failed. Please do this backport manually.

@nickvergessen
Copy link
Member Author

nickvergessen commented Oct 7, 2022

/backport 0710aa0 to stable23

This was referenced Oct 7, 2022
Merged
Merged
@nickvergessen nickvergessen mentioned this pull request Oct 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@provokateurin provokateurin provokateurin approved these changes

+1 more reviewer

@vitormattos vitormattos vitormattos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nickvergessen nickvergessen

Labels

4. to release bug feature: api 🛠️ OCS API for conversations, chats and participants feature: chat 💬 Chat and system messages feature: meetings 📅 Covering the webinary usecase incl. Lobby feature: search 🔎

Projects

None yet

Milestone

v16.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

Messages from a room with a lobby can still be found when searching for them

4 participants

@nickvergessen @vitormattos @provokateurin