Search improve

[romanesko]

#2007

[RomaricMourgues]

Nice to have you back on the code!

I have some feedback, that's normal after this delay. There is mainly small improvements asked on my comments.

I also need to discuss with you about the current strategy of getting all the channels first and then looking at this big list on the search results. Take a look at how I did it for listing messages and adding the include_users in it: includeUsersInDirectChannel in src/services/channels/services/channel/service.ts

Thanks for the awesome work on tests and global structure of the code!

[RomaricMourgues]

Yes it was pretty elegant unfortunately 😬

…

On 15 Mar 2022, at 09:55, Roman Bykovskiy ***@***.***> wrote:  @romanesko commented on this pull request. In twake/backend/node/src/services/messages/web/controllers/views.ts: > + if (!exp || SimpleChannelsCache.now() > exp) { + if (!setIfMissing) { + return null; + } + this.set(key, await setIfMissing()); + } + return this.data[key]; + } + + set(key: string, value: { [key: string]: boolean }) { + this.expiration[key] = SimpleChannelsCache.now() + this.minutesTTL * 1000; + this.data[key] = value; + } +} + +const simpleChannelsCache = new SimpleChannelsCache(5); 🤦‍♂️ … mine is pretty elegant too ) ok, I'll change it to node-cache — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you commented.

[RomaricMourgues]

Interesting ! Scroll_id probably contain the information of pagination and stuff already I suppose. Thanks for the information you can remove it then ;)

…

On 15 Mar 2022, at 09:53, Roman Bykovskiy ***@***.***> wrote:  @romanesko commented on this pull request. In twake/backend/node/src/core/platform/services/search/adapters/elasticsearch/index.ts: > if (options.pagination.page_token) { esResponse = await this.client.scroll( - { scroll_id: options.pagination.page_token, ...esParamsWithScroll }, + { + scroll_id: options.pagination.page_token, + // ...esParamsWithScroll It doesn't work with query in _search/scroll. It expects only scroll_id that refers to the previous query, and it falls with error if query exists in this request. I tested on a local elastic search and found this pattern. The documentation mentions it in passing, but it doesn't work with query anyway. So I suppose we can remove it completely. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you commented.

[RomaricMourgues]

If you can share the generated elastic search query which should appear in the logs I can probably help I mean if the problem is in the elastic search query of course

…

On 15 Mar 2022, at 10:01, Roman Bykovskiy ***@***.***> wrote:  @romanesko commented on this pull request. In twake/backend/node/src/services/messages/services/views/service.ts: > @@ -222,6 +222,7 @@ export class ViewsService implements MessageViewsServiceAPI { ...(options.companyId ? { $in: [["company_id", [options.companyId]]] } : {}), ...(options.workspaceId ? { $in: [["workspace_id", [options.workspaceId]]] } : {}), ...(options.channelId ? { $in: [["channel_id", [options.channelId]]] } : {}), + // TODO: implement boolean for hasFiles Couldn't handle with has_files actually, need your help. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you commented.

[RomaricMourgues]

For code it is ok for me, but tests don't like it yet :)

[romanesko]

Is it possible that tests use non-elastic-search configuration? Can't figure out