Plugin API: GetChannels method

[mattermod]

Introduce a GetChannels method to the base plugin API. This should serve as the basis for all bulk queries of channels.

type GetChannelsOptions struct {
    // ChannelTypes optionally limits the types of channels to return.
    // See model.CHANNEL*OPEN, model.CHANNEL_PRIVATE, model.CHANNEL_DIRECT, and     model.CHANNEL*GROUP.
	ChannelTypes []string
	// UserIs optionally limits the channels to those containing the given user.
	UserId string
	// TeamId optionally limits the channels to those in the given team.
	teamId string
	//
	IncludeDeleted bool
}

GetChannels(options *model.GetChannelsOptions, page, perPage int) ([]*model.Channel, *model.AppError)

We may later build plugin helpers upon this RPC method to streamline certain kinds of queries, e.g. get all public channels for a single user, or get all direct messages for a single user.

---

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-18566

[dnguy078]

i'd like to take look at this

[hanzei]

Thanks @dnguy078 👍

[dnguy078]

@hanzei could i get some 👀 on this. Wanted to see if i was heading in the right direction with this ticket. had some questions around the channels_store.go test; i left them in the PR.

[0xVolodya]

Can I take this

[hanzei]

Awesome, thanks @nadalfederer 👍

[0xK4d1r]

I would like to work on this.

[hanzei]

That's great @kadir96. Please let me know if you have any questions.

[0xK4d1r]

@hanzei Should the struct in the issue description be implemented as-is or it is an example? Also, I see that previous PRs added GetChannelsWithOptions() to channel store. Is this how I should proceed too? Would it be ok to extend the struct with teamId, userId and includeDeleted and update GetChannels() in the channel store instead of adding GetChannelsWithOptions()?

[hanzei]

@kadir96 Good suggestion with GetChannels(). Yes, it makes sense to change the signature to GetChannels(teamId, userId string, includeDeleted bool, ChannelTypes []string) (*model.ChannelList, *model.AppError). The app layer would then take care of mapping the values of GetChannelsOptions the the correct call to GetChannels.

IncludeDeleted should also be part of GetChannelsOptions. 👍

[0xK4d1r]

@hanzei Page and PerPage params are missing in the signature you mentioned and when they are added, it becomes GetChannels(teamId, userId string, includeDeleted bool, ChannelTypes []string, page, perPage int) (*model.ChannelList, *model.AppError) and I think it becomes very bloated.

I was thinking changing the signature to GetChannels(opts model.GetChannelOptions) (*model.ChannelList, *model.AppError) like here and here. What do you think?

[hanzei]

@kadir96 Bloating the signature is a good point. But I like keeping page and perPage outside the options struct to ensure that people really think about it and pass it. What about:
GetChannels(options *model.GetChannelsOptions, page, perPage int) ([]*model.Channel, *model.AppError)?

[0xK4d1r]

@hanzei after your last comment, I began implementing and I started to think having page and perPage outside of the struct is not a good idea if this feature is implemented by updating the GetChannels() in the store.

The callers (1 2 3) need to get all channels matching to be functional and to do so they will need to supply some weird page and perPage params to get whole data at once for ex: a.Srv().Store.Channel().GetChannels(model.ChannelGetOptions{TeamId: teamId, UserId: userId}, 1, 1000). It does not feel right.

Should we introduce a new method instead of updating GetChannels()?

[0xK4d1r]

ping @hanzei. I would love to hear your thoughts on my last comment.

[0xK4d1r]

I think that GetChannels() method name in the store is somewhat ambitious(?) since its scope is very limited. All it does is returning all channels of a user in a team. Does it make sense to rename this method to something like GetChannelsOfUserInTeam() and implement GetChannels() for this feature? To prevent code duplication, both methods could use a method to build the base query. If you want, I can prepare a prototype so that you can review.

[hanzei]

@kadir96 admittedly I'm don't know too much about the same schema of store method. @streamer45 Do you have thoughts on the above proposal?

[streamer45]

I think the suggestion is reasonable. We can rename the current ChannelStore.GetChannels() method to GetChannelsForTeamForUser() which is more specific and use the generic name for this issue's purposes, similarly to what happens with PostStore.GetPosts().

As far as the page/perPage dilemma we usually include those fields in the options structure: model.GetPostsOptions and model.UserGetOptions are some examples. I'd probably do the same here, for consistency more than anything.

[hanzei]

While I see the argument for consistency it's more important to me to be clear on the arguments. I'm 3/5

[0xK4d1r]

I saw some comments regarding whether including page/perPage in the options on one of previous pr for this issue. Maybe it helps to decide.

[hanzei]

I think #12649 (comment) is the final point.

[0xK4d1r]

So, I will rename ChannelStore.GetChannels() to ChannelStore. GetChannelsForTeamForUser() and implement GetChannels(options *model.GetChannelsOptions, page, perPage int) ([]*model.Channel, *model.AppError) for this issue.

Is it ok?

[streamer45]

I think there's a slight confusion here. I am discussing the store method, which is internal and not part of any user exposed API and I think it's fine for that to accept a single options parameter since technically all fields would be optional from the store point of view.

ChannelStore.GetChannels() will be then wrapped by a higher layer (app) method which can have the page/perPage parameters directly in its signature. This would also mean having an additional structure store.GetChannelsOptions that embeds (and extends) the proposed model.GetChannelsOptions.

As a side note, unless strictly necessary, I would avoid passing a pointer to the options structure but use value semantics instead.

[0xK4d1r]

I think the approach you shared is way better and I would go with it. I can implement like this right away but I could not find similar approach, would not this approach require any proposal or something like that so it can be reviewed, discussed and decided to have it or not? It seems a big move to me (Its just my 2 cents of course :))

[streamer45]

Well that's what we are doing in a sense. This is a public issue after all :)

The biggest change we are making is renaming a store method. Everything else is adding some behaviour which is currently missing. As long as we keep existing functionality unaltered I think we are good. But you are right, let's keep it simple so you can focus on the issue at hand. We can always revisit the store parts in a separate PR.

I think we can go ahead with the store method renaming and passing this model.GetChannelsOptions struct to both app and store layer methods which will end up having an almost identical signature.

GetChannels(page, perPage int, options model.GetChannelsOptions) ([]*model.Channel, *model.AppError)