Skip to content

[IMPROVE] Add groups to the directory channels list#21687

Merged
sampaiodiego merged 20 commits intodevelopfrom
private-channels-directory
May 21, 2021
Merged

[IMPROVE] Add groups to the directory channels list#21687
sampaiodiego merged 20 commits intodevelopfrom
private-channels-directory

Conversation

@matheusbsilva137
Copy link
Contributor

Proposed changes (including videos or screenshots)

  • Add groups (private channels) to the directory channels list. Only groups in which the logged user is subscribed are shown in the list.

Issue(s)

Task - ClickUp

Steps to test or reproduce

Open Directory > Channels. Private channels (groups) will also be shown in the list.

Further comments

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

We'll need to find a better way to do this. The way you're proposing doesn't work in case the first find findByNameOrFNameAndTypeIncludingTeamRooms returns a long list of private channels the user is not a member of, so when calling getSubscribedRoomsForUserWithDetails it won't return anything, so the result from the endpoint will be empty, even though there are public channels that should have been returned.

one solution we could do is similar to the one implemented on teams.listRooms, which consists in first getting channels the user is member of and then use those channels in an $or statement to get the private channels from that list. but let's discuss the best approach.

@KevLehman
Copy link
Member

Adding to what Diego was saying, the way we did on other places was to first get the subscriptions of the user (or the __rooms prop from the user if possible) to know to which channels the user has access. Then, we used that in a query to fetch public & private channels that the user can see.

There are examples of this @ spotlight, teams.listRooms and other endpoints too.

Copy link
Member

@KevLehman KevLehman left a comment

Choose a reason for hiding this comment

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

Left a few comments on the query

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

looks like we're almost there 😬

from my tests it is showing private channels I cannot join/see, looks like from teams I've joined or public teams. I guess the first t: { $in: ['p', 'c'] } messed up with this.

what need to be done is exactly what is described on PR's description, so maybe the only required change would be using user.__rooms to find more rooms, wdyt?

@matheusbsilva137 matheusbsilva137 requested a review from a team May 5, 2021 16:21
KevLehman
KevLehman previously approved these changes May 11, 2021
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

there is an issue on frontend still..

}

findTeamMainRooms(options) {
return this._db.find({ teamMain: { $exists: true, $eq: true } }, options);
Copy link
Member

Choose a reason for hiding this comment

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

just a small change as both are equivalent but this is much easier to read:

Suggested change
return this._db.find({ teamMain: { $exists: true, $eq: true } }, options);
return this._db.find({ teamMain: true }, options);

return this._db.find(query, options);
}

findTeamMainRooms(options) {
Copy link
Member

Choose a reason for hiding this comment

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

sry, a picky naming suggestion, since this is already on "rooms" model, I think we don't need to say we're finding rooms:

Suggested change
findTeamMainRooms(options) {
findTeamMain(options) {

if my other suggestion is accepted, this method might not be needed anymore though


const teams = Promise.await(Team.getAllPublicTeams());
const teamIds = teams.map(({ _id }) => _id);
const teamsMains = Rooms.findTeamMainRooms({
Copy link
Member

Choose a reason for hiding this comment

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

@KevLehman 's suggestion was great, but finding and storing all main rooms here even though none of them might be used if "result" doesn't have any team doesn't seem the best approach.

My suggestion would be:

  • filter results to get all rooms with teams
  • perform a find grabbing only the name of that rooms
  • map results to add the belongsTo field

rough example

const result = Rooms.findByNameOrFNameAndRoomIdsIncludingTeamRooms( ..... );
const results = result.fetch();

const teamIds = results.filter(({ teamId }) => teamId).map(({ teamId }) => teamId);

const teams = Rooms.findByIds(teamIds, { fields: { name: 1 } });

return results.map((room) => {
  if (room.teamId) {
    const team = teams.find((team) => team._id === room.teamId);
    if (team) room.belongsTo = team.name;
  }
});

@sampaiodiego sampaiodiego dismissed KevLehman’s stale review May 21, 2021 00:35

changes addressed

@sampaiodiego sampaiodiego merged commit ea63438 into develop May 21, 2021
@sampaiodiego sampaiodiego deleted the private-channels-directory branch May 21, 2021 01:21
@sampaiodiego sampaiodiego mentioned this pull request May 28, 2021
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.

3 participants

Comments