Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Clone Group functionality #4056

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Add Clone Group functionality #4056

merged 1 commit into from
Sep 5, 2024

Conversation

maxphilippov
Copy link
Collaborator

  • useCreateGroup returned function (CreateGroup) now accepts optional groupTemplateId same way Android/iOS version do, this is a group chat id to base new chat on

  • resolves add "clone group" #3933

@maxphilippov maxphilippov requested review from WofWca, Simon-Laux, nicodh and farooqkz and removed request for WofWca July 30, 2024 17:12
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

LGTM apart from parameter mutation, did not test.

if (groupImage && groupImage !== '') {
await BackendRemote.rpc.setChatProfileImage(accountId, chatId, groupImage)
}
groupMembers.push(...chat.contactIds)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it OK to mutate the parameter? Would it not add the same members twice to the same array if you cloned the group twice?
I'd just clone the array.

return BackendRemote.rpc.addContactToChat(accountId, chatId, contactId)
})
)
finalGroupName = `Copy: ${chat.name}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not internationalized, but perhaps it's fine.

const createGroup = useCallback(async () => {
const isVerified = await areAllContactsVerified(accountId, groupMembers)
const createGroup = useCallback(
async (groupTemplateId?: number | null) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
async (groupTemplateId?: number | null) => {
async (groupTemplateId?: number) => {

[accountId, groupImage, groupMembers, groupName]
)

return async (groupTemplateId?: number | null) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return async (groupTemplateId?: number | null) => {
return async (groupTemplateId?: number) => {

@r10s
Copy link
Member

r10s commented Jul 31, 2024

thanks for pushing this forward.

some remarks:

  • the group must not be created immediately - otherwise, you are not able to create eg. an unverified group by removing or adding members, which is one of the main purposes about group cloning.

  • in general, ppl will want to modify the member list, so we should directly end up there.

  • also, the "Copy" prefix is not nice - i fear, ppl will not change that and you end up with such weird names as "Copy: Copy: sth."

  • avatar is not cloned

so, in general, we should do the same as on android: open the "Create Group" screen with some memberlist/name/avatar preset from the old group, but crate the group only when the user explicitly hits "Create".

see android/ios to get a feeling about the flow

- useCreateGroup returned function (CreateGroup) now accepts
  optional groupTemplateId same way Android/iOS version do,
  this is a group chat id to base new chat on

- resolves #3933
@maxphilippov maxphilippov marked this pull request as ready for review September 5, 2024 06:06
@maxphilippov maxphilippov merged commit a2ad1c7 into main Sep 5, 2024
7 checks passed
@maxphilippov maxphilippov deleted the maxph/clone_group branch September 5, 2024 12:21
@r10s r10s mentioned this pull request Sep 12, 2024
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.

add "clone group"
4 participants