* MM-42282: handle teamId parameter correctly
As per https://community-daily.mattermost.com/core/pl/ugs7ue6e4j8a7cgegk1bxje8to, `ThreadStore.GetThreadsForUser` accepts a `teamId` parameter, but incorrectly handles an empty value of `""` as looking only for channels with an empty `teamId` (aka DMs and GMs) instead of finding all channels and effectively ignoring the team property.
Fixes: https://mattermost.atlassian.net/browse/MM-42282
* break up getThreadsForUser, leverage errgroup
This change breaks up `GetThreadsForUser` in the `ThreadStore` into its constituent `GetTotalUnreadThreads`, `GetTotalThreads`, `GetTotalUnreadMentions`, and the original `GetThreadsForUser` but now solely returning the thread structures. Instead of a monolithic method at the store level, the application layer now handles calling bulk requests, leveraging `errgroup` for simpler parallelization.
This change brings with it a few benefits:
* Simpler code, including more idiomatic usage of squirrel
* Simpler SQL, joining tables only when configured conditions require same. (No performance benefit here, since an unused LEFT JOIN generally has no overhead.)
* Discrete Grafana metrics for each store method, giving us better insight into the performance characteristics in play.
* **Performance boost**: reduced overhead when clearing push notifications.
This last point is what prompted the re-re-reactoring in this PR. As I broke things up, I realized that `clearPushNotificationSync` only used the `TotalUnreadMentions`, but asked for the count of total threads and total unread threads. By exposing the discrete methods, this code path avoids two aggregate queries. We clear notifications when marking a thread as read, and when marking a channel with unread mentions as viewed, so I expect we'll see at least a modest boost to performance from simply not wasting these cycles anymore.
No performance improvements are expected from this PR for the general case of using `GetThreadsForUser` to populate the threads view.
* never discard errors from building queries
* no MustSql
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
This is more of a general refactor of the initialization
process which should allow us to pass services more
easily.
The changes are minimal to keep the scope limited.
For now, the objective is to pass the file service
to the Channels product. For that, it was required
to move some of the enterprise interfaces under Channels
from Server.
We also create a filestore field in the server to
avoid creating filestore reference every time
we make a filestore operation. This will be later
passed on to the Channels product.
Also removed an unnecessary test.
The test was working so far because we were creating
the filebackend every time for every request. But
we should go via UpdateConfig call which would fail,
were we to assign an invalid filestore name.
So we were actually testing for a different thing.
Therefore, removed the test.
```release-note
NONE
```
* Add push notification check to ping
* Add test type and missing details
* Address feedback
* Fix function name
* Address feedback
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* CRT changes
* Lint fix
* Moved postId check
* Moved postId check
* Lint fix
* Misc
* Added test case for badge count while clearing notifications
* Fixed count when CRT is on and added isCRTEnabled, teamId to isIdLoaded notifications
* test fix
* isCRTEnabledForUser capitalised
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* Move Channels into App
In this PR, we make Channels as part of App
instead of Server. This is part of the transition period
of moving fields from Server to Channels.
For now, Channels contains Server. So the hierarchy is
App -> Channels -> Server.
And as a first step, we also move httpService to Channels.
```release-note
NONE
```
* Fixing another test
```release-note
NONE
```
* new method
```release-note
NONE
```
MM-38093: updates notifications copy on CRT replies
Updates texts form push and email notifications when the user has CRT "on"
and the post is a reply to a thread.
Email batches text aware of CRT threads
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* CRT: desktop thread notifications
* Fixes go lint
* Adds default for desktop CRT notifications
* Adds email and push notifications for CRT threads
Adds user ids of thread followers with CRT to crtMentions so they will get
notified appropriately.
* Minor change
* Refactor a bit
CRTMentions.addMention had a bug on the return and de-duplication.
This commit fixes duplicate notifications by looking up if the user is to be
notified on CRT on both email and push notifications.
* Minor refactor
* Changes according to review comments
- Fixes adding to followers a user that had explicitly unfollowed a
thread.
- Simplified send email according to email_threads option
- Send mentions and followers in separate arrays via the websocket
- Fixes push notifications message for push_threads
* Adds a comment on a buggy use case
* Updates comment to correct ticket link
* Fixes when user notifications is set to all
There was a bug where if user had set notifications to all
then they would receive desktop notifications even for non following threads.
A similar bug existed in push notifications, where if a user has set it
to all the threads setting would still be considered.
This commit fixes that by adding users to notificationsForCRT
StringArray when they have the non thread setting to 'all'.
* Fixes notifications to users unfollowing threads
Users which had previously explicitly unfollowed a thread
should not receive notifications about those threads.
* Update store mocks
* Fixes push notifications for CRT
Push notification about replies for CRT users should have a title of
"Reply to Thread".
CRT users with global user setting to 'UserNotifyAll' should not get
notifications for unfollowed threads.
This commit fixes those issues.
* Fixes i18n error
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* MM-21357: Use typed constant for channel types
https://mattermost.atlassian.net/browse/MM-21357
```release-note
- Introduced a new type ChannelType for all channel types.
- Updated the Client4.UpdateChannelPrivacy method to ChannelType.
```
* Address review comments
```release-note
NONE
```
* telemetry fix
```release-note
NONE
```
* MM-36894: Racy test: TestExecuteCommandInDirectMessageChannel
We were using the pointer to the same post object
in multiple push notifications because we were sending
it in a loop of mentioned user list.
To fix this, we clone the post every time.
https://mattermost.atlassian.net/browse/MM-36894
```release-note
NONE
```
* Refactor markdown parsing
```release-note
NONE
```
* format using `goimports -local github.com/mattermost/mattermost-server/v5 -w`
* added goimports lint check to .golangci.yml
* format using `goimports -local github.com/mattermost/mattermost-server/v5 -w` for a corner case
* make app-layers, *-mocks and store-layers for ci check
Co-authored-by: Mahmudul Haque <mahmudulhaque@protonmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* Moving diagnostics into a service
* Fixing golint checks
* Fixing tests
* Renaming from diagnostics to telemetry
* Adding missing files
* Initializing telemetry earlier in the server startup
* Fixing tests
* Adding a log for the telemetryID initialization error
* Addressing PR review comments
* Fixing merge problem
* Removing some extra Diagnostics mentions
* Making tests pass
* MM-27116: Drain push notification channel before closing
Without this, pending push notifications will not be sent when shutting down
or restarting the server.
* Fix race
* Re-arrange server shutdown
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
This avoids the race of assigning the sema field from the start method
which runs in a goroutine.
The race that happens is
==================
WARNING: DATA RACE
Write at 0x00c003a9d1b0 by goroutine 67:
github.com/mattermost/mattermost-server/v5/app.(*PushNotificationsHub).start()
/home/agniva/mattermost/mattermost-server/app/notification_push.go:264 +0x90
Previous read at 0x00c003a9d1b0 by goroutine 69:
github.com/mattermost/mattermost-server/v5/app.(*Server).createPushNotificationsHub()
/home/agniva/mattermost/mattermost-server/app/notification_push.go:260 +0x1d5
* Removing some other fake apps
* More FakeApp removed
* Removing entirely FakeApp
* Fixing some tests
* Fixing get Cluster id from get plugin status
* Fixing failing tests
* Fixing tests
* Fixing test initialization for web
* Fixing InitServer for server tests
* Fixing InitServer for server tests
* Reverting go.sum and go.mod
* Removing unneded HTMLTemplates function in App layer
* Moving back some functions to its old place to easy the review
* Moving back some functions to its old place to easy the review
* Using the last struct2interface version
* Generating store layers
* Fixing merge problems
* Addressing PR comments
* Small fix
* Fixing app tests build
* Fixing tests
* fixing tests
* Fix tests
* Fixing tests
* Fixing tests
* Fixing tests
* Moving license to server struct
* Adding some fixes to the test compilation
* Fixing cluster and some jobs initialization
* Fixing some license tests compilation problems
* Fixing recursive cache invalidation
* Regenerating app layers
* Fix test compilation
Co-authored-by: mattermod <mattermod@users.noreply.github.com>
Before this, for every single request we would create a new http.Client
from scratch. This was costly because every new client has internal
TCP connections which it reuses.
Therefore, originally, beyond a throughput of 800-1200 rps with a
concurrency of 50, beyond which it would run out of file descriptors.
Now it can support upto 8000-10000 rps.
An improvement by a factor of 10.
Throughput numbers:
BenchmarkPushNotification
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 11445.091601 reqs/s
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9190.761518 reqs/s
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 8334.394764 reqs/s
BenchmarkPushNotification-8 219 5999304 ns/op 3369224 B/op 42127 allocs/op
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9590.399780 reqs/s
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9347.137463 reqs/s
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9053.784250 reqs/s
BenchmarkPushNotification-8 223 5522636 ns/op 3368470 B/op 42034 allocs/op
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 10250.783365 reqs/s
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9123.292899 reqs/s
BenchmarkPushNotification: notification_push_test.go:1478: throughput: 9173.553180 reqs/s
This is the aggregate count using benchstat.
The number is for a single run of sending 50 requests.
name time/op
PushNotification-8 5.61ms ± 7%
name alloc/op
PushNotification-8 3.36MB ± 1%
name allocs/op
PushNotification-8 42.0k ± 1%
Tested on a machine with ulimit -n = 4096.
