From 6cba8ef9504e290033a82d4360d8189905fe2478 Mon Sep 17 00:00:00 2001 From: Hamish Peebles Date: Mon, 18 Dec 2023 16:49:02 +0000 Subject: [PATCH] Join community members to public channel if it has its gate removed (#5033) --- backend/canisters/community/CHANGELOG.md | 1 + .../community/impl/src/model/members.rs | 1 + .../impl/src/updates/update_channel.rs | 13 +++- .../integration_tests/src/client/community.rs | 34 ++++++++- .../src/communities/join_community_tests.rs | 27 ++----- .../src/communities/update_channel_tests.rs | 73 +++++++++++++++---- .../src/diamond_membership_tests.rs | 17 ++++- backend/libraries/group_chat_core/src/lib.rs | 3 + .../libraries/types/src/diamond_membership.rs | 2 +- 9 files changed, 127 insertions(+), 44 deletions(-) diff --git a/backend/canisters/community/CHANGELOG.md b/backend/canisters/community/CHANGELOG.md index 2c61de0ebe..497777d6a5 100644 --- a/backend/canisters/community/CHANGELOG.md +++ b/backend/canisters/community/CHANGELOG.md @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Suppress notifications and @s for suspect messages ([#5006](https://github.com/open-chat-labs/open-chat/pull/5006)) - Make Diamond membership gate check synchronous ([#5027](https://github.com/open-chat-labs/open-chat/pull/5027)) - Auto join Diamond members to newly created Diamond gated channels ([#5028](https://github.com/open-chat-labs/open-chat/pull/5028)) +- Join community members to public channel if it has its gate removed ([#5033](https://github.com/open-chat-labs/open-chat/pull/5033)) ### Fixed diff --git a/backend/canisters/community/impl/src/model/members.rs b/backend/canisters/community/impl/src/model/members.rs index 25579d7d6f..28453d410c 100644 --- a/backend/canisters/community/impl/src/model/members.rs +++ b/backend/canisters/community/impl/src/model/members.rs @@ -342,6 +342,7 @@ pub struct CommunityMemberInternal { impl CommunityMemberInternal { pub fn leave(&mut self, channel_id: ChannelId, now: TimestampMillis) { if self.channels.remove(&channel_id) { + self.channels_removed.retain(|c| c.value != channel_id); self.channels_removed.push(Timestamped::new(channel_id, now)); } } diff --git a/backend/canisters/community/impl/src/updates/update_channel.rs b/backend/canisters/community/impl/src/updates/update_channel.rs index ea2347238b..a3a14f106d 100644 --- a/backend/canisters/community/impl/src/updates/update_channel.rs +++ b/backend/canisters/community/impl/src/updates/update_channel.rs @@ -4,6 +4,7 @@ use canister_tracing_macros::trace; use community_canister::update_channel::{Response::*, *}; use group_chat_core::UpdateResult; use ic_cdk_macros::update; +use types::OptionUpdate; #[update] #[trace] @@ -44,9 +45,15 @@ fn update_channel_impl(mut args: Args, state: &mut RuntimeState) -> Response { now, ) { UpdateResult::Success(result) => { - if result.newly_public && channel.chat.gate.is_none() { - for m in state.data.members.iter_mut() { - join_channel_unchecked(channel, m, true, now); + if channel.chat.is_public.value && channel.chat.gate.is_none() { + // If the channel has just been made public or had its gate removed, join + // existing community members to the channel + if result.newly_public || matches!(result.gate_update, OptionUpdate::SetToNone) { + for m in state.data.members.iter_mut() { + if !m.channels_removed.iter().any(|c| c.value == channel.id) { + join_channel_unchecked(channel, m, true, now); + } + } } } diff --git a/backend/integration_tests/src/client/community.rs b/backend/integration_tests/src/client/community.rs index 0686395de1..e7542283a1 100644 --- a/backend/integration_tests/src/client/community.rs +++ b/backend/integration_tests/src/client/community.rs @@ -41,7 +41,7 @@ pub mod happy_path { use candid::Principal; use pocket_ic::PocketIc; use types::{ - ChannelId, CommunityCanisterChannelSummary, CommunityCanisterCommunitySummary, + AccessGate, ChannelId, CommunityCanisterChannelSummary, CommunityCanisterCommunitySummary, CommunityCanisterCommunitySummaryUpdates, CommunityId, CommunityRole, EventIndex, EventsResponse, MessageContentInitial, MessageId, MessageIndex, Rules, TextContent, TimestampMillis, UserId, }; @@ -77,6 +77,38 @@ pub mod happy_path { } } + pub fn create_gated_channel( + env: &mut PocketIc, + sender: Principal, + community_id: CommunityId, + is_public: bool, + name: String, + gate: AccessGate, + ) -> ChannelId { + let response = super::create_channel( + env, + sender, + community_id.into(), + &community_canister::create_channel::Args { + is_public, + name: name.clone(), + description: format!("{name}_description"), + rules: Rules::default(), + subtype: None, + avatar: None, + history_visible_to_new_joiners: is_public, + permissions_v2: None, + events_ttl: None, + gate: Some(gate), + }, + ); + + match response { + community_canister::create_channel::Response::Success(result) => result.channel_id, + response => panic!("'create_channel' error: {response:?}"), + } + } + pub fn leave_channel(env: &mut PocketIc, sender: Principal, community_id: CommunityId, channel_id: ChannelId) { let response = super::leave_channel( env, diff --git a/backend/integration_tests/src/communities/join_community_tests.rs b/backend/integration_tests/src/communities/join_community_tests.rs index fee720e7af..446fbe4c2b 100644 --- a/backend/integration_tests/src/communities/join_community_tests.rs +++ b/backend/integration_tests/src/communities/join_community_tests.rs @@ -7,7 +7,7 @@ use pocket_ic::PocketIc; use std::collections::HashSet; use std::ops::Deref; use test_case::test_case; -use types::{AccessGate, CommunityId, Empty, MessageContent, Rules}; +use types::{AccessGate, CommunityId, Empty, MessageContent}; #[test] fn join_public_community_succeeds() { @@ -273,30 +273,15 @@ fn user_joined_to_all_public_channels(diamond_member: bool) { let channel1 = client::community::happy_path::create_channel(env, user1.principal, community_id, true, random_string()); let channel2 = client::community::happy_path::create_channel(env, user1.principal, community_id, true, random_string()); let channel3 = client::community::happy_path::create_channel(env, user1.principal, community_id, false, random_string()); - let channel4_response = client::community::create_channel( + let channel4 = client::community::happy_path::create_gated_channel( env, user1.principal, - community_id.into(), - &community_canister::create_channel::Args { - is_public: true, - name: random_string(), - description: random_string(), - rules: Rules::default(), - subtype: None, - avatar: None, - history_visible_to_new_joiners: true, - permissions_v2: None, - events_ttl: None, - gate: Some(AccessGate::DiamondMember), - }, + community_id, + true, + random_string(), + AccessGate::DiamondMember, ); - let channel4 = if let community_canister::create_channel::Response::Success(result) = channel4_response { - result.channel_id - } else { - panic!() - }; - let community_summary = client::local_user_index::happy_path::join_community(env, user.principal, canister_ids.local_user_index, community_id); diff --git a/backend/integration_tests/src/communities/update_channel_tests.rs b/backend/integration_tests/src/communities/update_channel_tests.rs index 3968a3ffb7..927161b737 100644 --- a/backend/integration_tests/src/communities/update_channel_tests.rs +++ b/backend/integration_tests/src/communities/update_channel_tests.rs @@ -2,10 +2,12 @@ use crate::env::ENV; use crate::rng::random_string; use crate::{client, TestEnv}; use std::ops::Deref; -use types::OptionUpdate; +use test_case::test_case; +use types::{AccessGate, OptionUpdate}; -#[test] -fn make_private_channel_public_succeeds() { +#[test_case(true)] +#[test_case(false)] +fn members_added_if_channel_made_public_or_gate_removed(make_public: bool) { let mut wrapper = ENV.deref().get(); let TestEnv { env, @@ -15,17 +17,47 @@ fn make_private_channel_public_succeeds() { } = wrapper.env(); let user1 = client::register_diamond_user(env, canister_ids, *controller); - let user2 = client::local_user_index::happy_path::register_user(env, canister_ids.local_user_index); + let user2 = client::register_diamond_user(env, canister_ids, *controller); + let user3 = client::local_user_index::happy_path::register_user(env, canister_ids.local_user_index); - let community_id = client::user::happy_path::create_community(env, &user1, &random_string(), true, vec!["abc".to_string()]); - let channel_id = - client::community::happy_path::create_channel(env, user1.principal, community_id, false, "xyz".to_string()); + let community_id = client::user::happy_path::create_community(env, &user1, &random_string(), true, vec![random_string()]); + + let channel_id = if make_public { + client::community::happy_path::create_channel(env, user1.principal, community_id, false, random_string()) + } else { + client::community::happy_path::create_gated_channel( + env, + user1.principal, + community_id, + true, + random_string(), + AccessGate::DiamondMember, + ) + }; + + client::local_user_index::happy_path::invite_users_to_channel( + env, + user1.principal, + canister_ids.local_user_index, + community_id, + channel_id, + vec![user2.user_id], + ); + client::local_user_index::happy_path::join_channel( + env, + user2.principal, + canister_ids.local_user_index, + community_id, + channel_id, + ); for i in 0..5 { client::community::happy_path::send_text_message(env, &user1, community_id, channel_id, None, i.to_string(), None); } - client::local_user_index::happy_path::join_community(env, user2.principal, canister_ids.local_user_index, community_id); + client::community::happy_path::leave_channel(env, user2.principal, community_id, channel_id); + + client::local_user_index::happy_path::join_community(env, user3.principal, canister_ids.local_user_index, community_id); client::community::happy_path::update_channel( env, user1.principal, @@ -38,15 +70,26 @@ fn make_private_channel_public_succeeds() { avatar: OptionUpdate::NoChange, permissions_v2: None, events_ttl: OptionUpdate::NoChange, - gate: OptionUpdate::NoChange, - public: Some(true), + gate: if !make_public { OptionUpdate::SetToNone } else { OptionUpdate::NoChange }, + public: make_public.then_some(true), }, ); - // Check that user2 has been added to the channel and that it is now public - let channel_summary = client::community::happy_path::channel_summary(env, &user2, community_id, channel_id); + // Check that user2 has not been re-added to the channel + let user2_channel_summary = client::community::happy_path::summary(env, &user2, community_id); - assert!(channel_summary.is_public); - assert_eq!(channel_summary.min_visible_event_index, 7.into()); - assert_eq!(channel_summary.min_visible_message_index, 5.into()); + assert!(!user2_channel_summary.channels.iter().any(|c| c.channel_id == channel_id)); + + // Check that user3 has been added to the channel + let user3_channel_summary = client::community::happy_path::channel_summary(env, &user3, community_id, channel_id); + + if make_public { + assert!(user3_channel_summary.is_public); + assert_eq!(user3_channel_summary.min_visible_event_index, 10.into()); + assert_eq!(user3_channel_summary.min_visible_message_index, 5.into()); + } else { + assert!(user3_channel_summary.gate.is_none()); + assert_eq!(user3_channel_summary.min_visible_event_index, 0.into()); + assert_eq!(user3_channel_summary.min_visible_message_index, 0.into()); + } } diff --git a/backend/integration_tests/src/diamond_membership_tests.rs b/backend/integration_tests/src/diamond_membership_tests.rs index 0100d3bcbe..1ad476a140 100644 --- a/backend/integration_tests/src/diamond_membership_tests.rs +++ b/backend/integration_tests/src/diamond_membership_tests.rs @@ -129,7 +129,10 @@ fn membership_renews_automatically_if_set_to_recurring(ledger_error: bool) { .is_active()); let new_balance = client::icrc1::happy_path::balance_of(env, canister_ids.icp_ledger, user.user_id); - assert_eq!(new_balance, 1_000_000_000 - 40_000_000); + assert_eq!( + new_balance, + 1_000_000_000 - (2 * DiamondMembershipPlanDuration::OneMonth.icp_price_e8s() as u128) + ); } #[test_case(true)] @@ -240,7 +243,10 @@ fn update_subscription_succeeds(disable: bool) { )); let new_balance = client::icrc1::happy_path::balance_of(env, canister_ids.icp_ledger, user.user_id); - assert_eq!(new_balance, 1_000_000_000 - 20_000_000); + assert_eq!( + new_balance, + 1_000_000_000 - DiamondMembershipPlanDuration::OneMonth.icp_price_e8s() as u128 + ); } else { let one_year_millis = DiamondMembershipPlanDuration::OneYear.as_millis(); assert_eq!( @@ -253,6 +259,11 @@ fn update_subscription_succeeds(disable: bool) { )); let new_balance = client::icrc1::happy_path::balance_of(env, canister_ids.icp_ledger, user.user_id); - assert_eq!(new_balance, 1_000_000_000 - 170_000_000); + assert_eq!( + new_balance, + 1_000_000_000 + - (DiamondMembershipPlanDuration::OneMonth.icp_price_e8s() + + DiamondMembershipPlanDuration::OneYear.icp_price_e8s()) as u128 + ); } } diff --git a/backend/libraries/group_chat_core/src/lib.rs b/backend/libraries/group_chat_core/src/lib.rs index 65081433e0..ef1e4e8b20 100644 --- a/backend/libraries/group_chat_core/src/lib.rs +++ b/backend/libraries/group_chat_core/src/lib.rs @@ -1399,6 +1399,7 @@ impl GroupChatCore { ) -> UpdateSuccessResult { let mut result = UpdateSuccessResult { newly_public: false, + gate_update: OptionUpdate::NoChange, rules_version: None, }; @@ -1497,6 +1498,7 @@ impl GroupChatCore { if let Some(gate) = gate.expand() { if self.gate.value != gate { self.gate = Timestamped::new(gate.clone(), now); + result.gate_update = OptionUpdate::from_update(gate.clone()); events.push_main_event( ChatEventInternal::GroupGateUpdated(Box::new(GroupGateUpdated { @@ -1872,6 +1874,7 @@ pub enum UpdateResult { pub struct UpdateSuccessResult { pub newly_public: bool, + pub gate_update: OptionUpdate, pub rules_version: Option, } diff --git a/backend/libraries/types/src/diamond_membership.rs b/backend/libraries/types/src/diamond_membership.rs index ffdc9315e0..a736bae885 100644 --- a/backend/libraries/types/src/diamond_membership.rs +++ b/backend/libraries/types/src/diamond_membership.rs @@ -62,7 +62,7 @@ impl DiamondMembershipPlanDuration { DiamondMembershipPlanDuration::OneMonth => 200_000_000, // 2 CHAT DiamondMembershipPlanDuration::ThreeMonths => 500_000_000, // 5 CHAT DiamondMembershipPlanDuration::OneYear => 1_500_000_000, // 15 CHAT - DiamondMembershipPlanDuration::Lifetime => 60_000_000_000, // 60 CHAT + DiamondMembershipPlanDuration::Lifetime => 6_000_000_000, // 60 CHAT } }