-
Notifications
You must be signed in to change notification settings - Fork 0
SES-4753 - Manage Admins (Promotion and leaving group) #2
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
Conversation
| onClick = { | ||
| (address as? Address.Group)?.let { | ||
| navigateTo(ConversationSettingsDestination.RouteManageMembers(it)) | ||
| navigateTo(ConversationSettingsDestination.RouteManageAdmins(it)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to update the qaTag above by creating a new one so QA can have automated tests linking to this button, because right now you are reusing the manage members one.
|
|
||
| val context = LocalContext.current | ||
|
|
||
| LaunchedEffect(showingError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can send toast directly from your viewmodels. This can simplify your logic a lot.
| } | ||
| LaunchedEffect(showingOngoingAction) { | ||
| if (showingOngoingAction != null) { | ||
| Toast.makeText(context, showingOngoingAction, Toast.LENGTH_SHORT).show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can simplify all this logic form the VM as well
|
|
||
| LaunchedEffect(uiState.toast) { | ||
| if (!uiState.toast.isNullOrEmpty()) { | ||
| Toast.makeText(context, uiState.toast, Toast.LENGTH_SHORT).show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can simplify all this logic form the VM as well
| /** | ||
| * Shared helper for group operations (same pattern with ManageGroupMembersViewModel). | ||
| */ | ||
| private fun performGroupOperation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are starting to re-use this pattern could be extracted somewhere meaningful and re-used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, can this be put inside the BaseGroupMemberViewModel as both vm's extend it?
| try { | ||
| task.await() | ||
| } catch (e: CancellationException) { | ||
| return@launch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't return here. You do need to catch it though, but rethrow the error instead of returning.
If you don't catch it here specifically the cancellation will go in the next catch and isn't rethrown which breaks your coroutine lifecycle
| val groupName: StateFlow<String> = groupInfo | ||
| .map { it?.first?.name.orEmpty() } | ||
| .stateIn(viewModelScope, SharingStarted.WhileSubscribed(), "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like this name flow is used by multiple viewModels now, should it be moved to the BaseGroupMembersViewModel for reuse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah makes sense.
This PR adds member promotion screen and resending admin promotions
This PR also includes handling how admins can leave groups.