WPB-19712: Allow team admin to update the channels to user-group association #4783

blackheaven · 2025-09-18T17:25:58Z

https://wearezeta.atlassian.net/browse/WPB-19712

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

WIP

Tests

eyeinsky

Just my comments, not all of them might be correct. Otherwise looks good and easy to read and understand.

eyeinsky · 2025-09-24T14:03:25Z

libs/wire-api/src/Wire/API/Error/Brig.hs


 type instance MapError 'UserGroupNotATeamAdmin = 'StaticError 403 "user-group-write-forbidden" "Only team admins can create, update, or delete user groups."

+type instance MapError 'UserGroupChannelNotFound = 'StaticError 400 "user-group-channel-not-found" "Specified Channel does not exists or belongs to the team"


I guess this could be a 404.

changed, thanks.

eyeinsky · 2025-09-24T14:34:53Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+      lmap
+        (bimap toUUID (fmap toUUID))
+        $ [resultlessStatement|
+          delete from user_group_channel where user_group_id = ($1 :: uuid) and conv_id <> any($2 :: uuid[])


Hmm, testing on postgres console SELECT 1 <> ANY(ARRAY[1, 2, 3]); gives me true, so if an existing conversation is in the new set (part of the array) it would be deleted, but it's the other way around we want, don't we? (Unless I'm mistaken on what the logic should be.) Perhaps add this to the test to make sure.

Also, to my brain ... conv_id NOT IN (..new_conv_ids..) would be easier to read, rather than ... <> any(...) (but that may be just habit).

That's also how my brain works... but not how hasql encoders work.

I guess it works because there is also an upsert operation following it.

I've tried something else.

eyeinsky · 2025-09-24T15:04:28Z

postgres-schema.sql

+);
+

-ALTER TABLE public.user_group_member OWNER TO "wire-server";


Does this row need to be deleted?

Dropped by mistake, thanks.

eyeinsky · 2025-09-24T16:10:22Z

libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserGroupStore.hs

    AddUser gid uid -> addUserImpl gid uid
    UpdateUsers gid uids -> updateUsersImpl gid uids
    RemoveUser gid uid -> removeUserImpl gid uid
+    UpdateUserGroupChannels _ gid convIds -> updateUserGroupChannelsImpl gid convIds


Maybe remove the TeamId field as it doesn't seem to be used (?).

it's used in the Mock interpreter (it's not a good idea though)

I think that is not a good reason to have it here.

dropped, I was thinking of the subsystem.

battermann · 2025-09-30T10:02:07Z

libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserGroupStore.hs

    AddUser gid uid -> addUserImpl gid uid
    UpdateUsers gid uids -> updateUsersImpl gid uids
    RemoveUser gid uid -> removeUserImpl gid uid
+    UpdateUserGroupChannels _ gid convIds -> updateUserGroupChannelsImpl gid convIds


I think that is not a good reason to have it here.

battermann · 2025-09-30T10:06:41Z

libs/wire-subsystems/src/Wire/UserGroupStore/Postgres.hs

+    session :: Session ()
+    session = do
+      statement (gid, convIds) deleteStatement
+      forM_ convIds $ \convId ->
+        statement (gid, convId) insertStatement


This should be a transaction.
And maybe it is more efficient to add them with a single insert statement like in insertGroupMembersStatement? Not sure if it is better.

aaaah, I have looked for it for so long, thanks, done.

battermann · 2025-09-30T10:07:49Z

libs/wire-api/src/Wire/API/Error/Brig.hs


 type instance MapError 'UserGroupNotATeamAdmin = 'StaticError 403 "user-group-write-forbidden" "Only team admins can create, update, or delete user groups."

+type instance MapError 'UserGroupChannelNotFound = 'StaticError 404 "user-group-channel-not-found" "Specified Channel does not exists or belongs to the team"


Suggested change

type instance MapError 'UserGroupChannelNotFound = 'StaticError 404 "user-group-channel-not-found" "Specified Channel does not exists or belongs to the team"

type instance MapError 'UserGroupChannelNotFound = 'StaticError 404 "user-group-channel-not-found" "Specified Channel does not exists or does not belong to the team"

applied (manually)

battermann · 2025-09-30T10:10:16Z

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

    :<|> Named
           "update-user-group-channels"
-           ( Summary "[STUB] Update user group channels. Replaces the channels with the given list."
+           ( Summary "Replaces the channels with the given list."


add the possible errors here with CanThrow decorators.

battermann · 2025-09-30T10:12:42Z

integration/test/Test/UserGroup.hs

+testUserGroupUpdateChannels :: (HasCallStack) => App ()
+testUserGroupUpdateChannels = do
+  (alice, tid, _) <- createTeam OwnDomain 1
+
+  ug <-
+    createUserGroup alice (object ["name" .= "none", "members" .= (mempty :: [String])])
+      >>= getJSON 200
+  gid <- ug %. "id" & asString
+
+  convId <-
+    postConversation alice (defProteus {team = Just tid})
+      >>= getJSON 201
+      >>= objConvId
+  updateUserGroupChannels alice gid [convId.id_] >>= assertSuccess
+
+  bindResponse (getUserGroup alice gid) $ \resp -> do
+    resp.status `shouldMatchInt` 200


check that non-admins can't update
non existing user group return 404
check non existing (other team) conversations return error
check maybe also that an empty list removes all users
leave FUTUREWORK to check the actual assiciated channels, once the get feature is implemented.

check maybe also that an empty list removes all users

I'm not sure how to do this: should we track why users have been added to a channel?

Done:

check that non-admins can't update
non existing user group return 404
check non existing (other team) conversations return error
leave FUTUREWORK to check the actual assiciated channels, once the get feature is implemented.

battermann · 2025-09-30T10:16:04Z

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

    :<|> Named
           "update-user-group-channels"
-           ( Summary "[STUB] Update user group channels. Replaces the channels with the given list."
+           ( Summary "Replaces the channels with the given list."


Can we double check that the payload contains a list of unqualified conv_ids is ok?

with someone specifically? because we changed back the RFC

battermann · 2025-09-30T10:19:14Z

libs/wire-subsystems/src/Wire/UserGroupSubsystem/Interpreter.hs

+updateChannels performer groupId channelIds = do
+  void $ getUserGroup performer groupId >>= note UserGroupNotFound
+  teamId <- getTeamAsAdmin performer >>= note UserGroupNotATeamAdmin
+  traverse_ (getTeamConv performer teamId >=> note UserGroupChannelNotFound) channelIds


…ciation

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 18, 2025

blackheaven marked this pull request as ready for review September 19, 2025 16:49

blackheaven requested a review from a team as a code owner September 19, 2025 16:49

eyeinsky reviewed Sep 24, 2025

View reviewed changes

battermann requested changes Sep 30, 2025

View reviewed changes

blackheaven requested a review from battermann October 1, 2025 18:34

WPB-19712: Allow team admin to update the channels to user-group asso…

909912b

…ciation

blackheaven force-pushed the gdifolco/WPB-19712_user-groups-update-endpoint branch from 4ed864c to 909912b Compare October 2, 2025 15:09


		type instance MapError 'UserGroupNotATeamAdmin = 'StaticError 403 "user-group-write-forbidden" "Only team admins can create, update, or delete user groups."

		type instance MapError 'UserGroupChannelNotFound = 'StaticError 400 "user-group-channel-not-found" "Specified Channel does not exists or belongs to the team"

		);


		ALTER TABLE public.user_group_member OWNER TO "wire-server";

WPB-19712: Allow team admin to update the channels to user-group association #4783

Are you sure you want to change the base?

WPB-19712: Allow team admin to update the channels to user-group association #4783

Uh oh!

Conversation

blackheaven commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

WIP

Uh oh!

eyeinsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blackheaven commented Sep 18, 2025 •

edited

Loading