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

fix(conversation): disable addMember on a one-on-one conversation with deleted account (WPB-10259) #3349

Merged

Conversation

mchenani
Copy link
Member

@mchenani mchenani commented Aug 21, 2024

BugWPB-10259 [Android] : Duplicate 1:1 conversation - not showing one of the conv

…h deleted account


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

When the other peer of a one-on-one conversation gets deleted, the backend changes the conversation to a group conversation. Android app was allowing the user to add member to that conversation and it was a confusing flow; in this PR we're preventing this.


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@echoes-hq echoes-hq bot added the echoes: bugs Technical or functional defects in the product label Aug 21, 2024
Copy link
Contributor

github-actions bot commented Aug 21, 2024

Test Results

863 tests   863 ✅  9m 46s ⏱️
119 suites    0 💤
119 files      0 ❌

Results for commit de9d18c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@@ -394,7 +394,8 @@ private fun GroupConversationDetailsContent(
openFullListPressed = openFullListPressed,
onAddParticipantsPressed = onAddParticipantsPressed,
onProfilePressed = onProfilePressed,
lazyListState = lazyListStates[pageIndex]
lazyListState = lazyListStates[pageIndex],
allowAddMember = conversationSheetState.conversationSheetContent?.title?.isNotEmpty() ?: false
Copy link
Member

Choose a reason for hiding this comment

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

I would change this to check if the name is not null and there is only one member, since legacy group conversations did allow empty names
this way, legacy groups will still continue to work as before.

Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

else UIText.StringResource(R.string.group_unavailable_label).asString()
else UIText.StringResource(R.string.member_name_deleted_label).asString()
Copy link
Member

Choose a reason for hiding this comment

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

question: is there a reason for the title change?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but it was showing different message for the same situation! e.g. when you see a conversation for a deleted user, and open the conversation details, you see the title is not available! so I decided to make it concise.

Comment on lines 398 to 399
allowAddMember = conversationSheetState.conversationSheetContent?.title?.isNotEmpty() == true
|| groupParticipantsState.data.allCount > 1
Copy link
Member

Choose a reason for hiding this comment

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

lets move this into the conversationSheetState this way it can be tested

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but still I need to pass the participants counts to it; since the groupParticipants stored out of the conversationSheetState.

Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@mchenani mchenani enabled auto-merge (squash) August 27, 2024 08:08
Copy link

sonarcloud bot commented Aug 28, 2024

@mchenani mchenani merged commit e745bb2 into release/cycle-4.6 Aug 28, 2024
12 of 13 checks passed
@mchenani mchenani deleted the fix/disable-add-member-for-deleted-member-oneonone branch August 28, 2024 06:49
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: bugs Technical or functional defects in the product size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants