diff --git a/frame/ranked-collective/src/lib.rs b/frame/ranked-collective/src/lib.rs index a65d184d6bf06..5055a1019bdd5 100644 --- a/frame/ranked-collective/src/lib.rs +++ b/frame/ranked-collective/src/lib.rs @@ -465,24 +465,7 @@ pub mod pallet { pub fn demote_member(origin: OriginFor, who: AccountIdLookupOf) -> DispatchResult { let max_rank = T::DemoteOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; - let mut record = Self::ensure_member(&who)?; - let rank = record.rank; - ensure!(max_rank >= rank, Error::::NoPermission); - - Self::remove_from_rank(&who, rank)?; - let maybe_rank = rank.checked_sub(1); - match maybe_rank { - None => { - Members::::remove(&who); - Self::deposit_event(Event::MemberRemoved { who, rank: 0 }); - }, - Some(rank) => { - record.rank = rank; - Members::::insert(&who, &record); - Self::deposit_event(Event::RankChanged { who, rank }); - }, - } - Ok(()) + Self::do_demote_member(who, Some(max_rank)) } /// Remove the member entirely. @@ -503,13 +486,9 @@ pub mod pallet { let who = T::Lookup::lookup(who)?; let MemberRecord { rank, .. } = Self::ensure_member(&who)?; ensure!(min_rank >= rank, Error::::InvalidWitness); - ensure!(max_rank >= rank, Error::::NoPermission); - for r in 0..=rank { - Self::remove_from_rank(&who, r)?; - } - Members::::remove(&who); - Self::deposit_event(Event::MemberRemoved { who, rank }); + Self::do_remove_member(who, Some(max_rank))?; + Ok(PostDispatchInfo { actual_weight: Some(T::WeightInfo::remove_member(rank as u32)), pays_fee: Pays::Yes, @@ -654,8 +633,9 @@ pub mod pallet { /// Promotes a member in the ranked collective into the next role. /// - /// A `maybe_max_rank` may be provided to check that the member does not get promoted beyond - /// a certain rank. Is `None` is provided, then the rank will be incremented without checks. + /// A `maybe_max_rank` may be provided to check that the rank of `who` is not higher than + /// a given `maybe_max_rank`, otherwise fails with `NoPermission` error. if `None` is + /// provided, then the rank of `who` will be incremented without the check. pub fn do_promote_member( who: T::AccountId, maybe_max_rank: Option, @@ -683,5 +663,51 @@ pub mod pallet { } Ok(()) } + + /// Decrement the rank of an existing member by one. If the member is already at rank zero, + /// then it is removed entirely. + /// + /// A `maybe_max_rank` may be provided to check that the rank of `who` is not higher than + /// a given `maybe_max_rank`, otherwise fails with `NoPermission` error. if `None` is + /// provided, then the rank of `who` will be decremented without the check. + pub fn do_demote_member(who: T::AccountId, maybe_max_rank: Option) -> DispatchResult { + let mut record = Self::ensure_member(&who)?; + let rank = record.rank; + if let Some(max_rank) = maybe_max_rank { + ensure!(max_rank >= rank, Error::::NoPermission); + } + Self::remove_from_rank(&who, rank)?; + let maybe_rank = rank.checked_sub(1); + match maybe_rank { + None => { + Members::::remove(&who); + Self::deposit_event(Event::MemberRemoved { who, rank: 0 }); + }, + Some(rank) => { + record.rank = rank; + Members::::insert(&who, &record); + Self::deposit_event(Event::RankChanged { who, rank }); + }, + } + Ok(()) + } + + /// Remove the member entirely. + /// + /// A `maybe_max_rank` may be provided to check that the rank of `who` is not higher than + /// a given `maybe_max_rank`, otherwise fails with `NoPermission` error. if `None` is + /// provided, then `who` will be removed from any rank without the check. + pub fn do_remove_member(who: T::AccountId, maybe_max_rank: Option) -> DispatchResult { + let MemberRecord { rank, .. } = Self::ensure_member(&who)?; + if let Some(max_rank) = maybe_max_rank { + ensure!(max_rank >= rank, Error::::NoPermission); + } + for r in 0..=rank { + Self::remove_from_rank(&who, r)?; + } + Members::::remove(&who); + Self::deposit_event(Event::MemberRemoved { who, rank }); + Ok(()) + } } } diff --git a/frame/ranked-collective/src/tests.rs b/frame/ranked-collective/src/tests.rs index c11d4877d3577..d6633d98187f8 100644 --- a/frame/ranked-collective/src/tests.rs +++ b/frame/ranked-collective/src/tests.rs @@ -355,19 +355,35 @@ fn promote_demote_by_rank_works() { assert_ok!(Club::promote_member(RuntimeOrigin::signed(2), 6)); assert_ok!(Club::add_member(RuntimeOrigin::signed(2), 7)); assert_ok!(Club::promote_member(RuntimeOrigin::signed(2), 7)); + assert_ok!(Club::promote_member(RuntimeOrigin::signed(2), 7)); // #3 as rank 3 can demote/remove #4 & #5 but not #6 & #7 assert_ok!(Club::demote_member(RuntimeOrigin::signed(3), 4)); assert_ok!(Club::remove_member(RuntimeOrigin::signed(3), 5, 0)); assert_noop!(Club::demote_member(RuntimeOrigin::signed(3), 6), Error::::NoPermission); assert_noop!( - Club::remove_member(RuntimeOrigin::signed(3), 7, 1), + Club::remove_member(RuntimeOrigin::signed(3), 7, 2), Error::::NoPermission ); + assert_noop!( + Club::remove_member(RuntimeOrigin::signed(3), 7, 1), + Error::::InvalidWitness + ); - // #2 as rank 5 can demote/remove #6 & #7 + // #2 as rank 5 can demote #6 until removed + type Rank0 = EnsureRanked; + assert_eq!(Rank0::try_origin(RuntimeOrigin::signed(6)).unwrap(), 1); + assert_ok!(Club::demote_member(RuntimeOrigin::signed(2), 6)); + assert_eq!(Rank0::try_origin(RuntimeOrigin::signed(6)).unwrap(), 0); assert_ok!(Club::demote_member(RuntimeOrigin::signed(2), 6)); - assert_ok!(Club::remove_member(RuntimeOrigin::signed(2), 7, 1)); + assert!(Rank0::try_origin(RuntimeOrigin::signed(6)).is_err()); + assert_noop!(Club::demote_member(RuntimeOrigin::signed(2), 6), Error::::NotMember); + + // #2 as rank 5 can remove #7 + assert_eq!(Rank0::try_origin(RuntimeOrigin::signed(7)).unwrap(), 2); + assert_ok!(Club::remove_member(RuntimeOrigin::signed(2), 7, 2)); + assert!(Rank0::try_origin(RuntimeOrigin::signed(7)).is_err()); + assert_noop!(Club::demote_member(RuntimeOrigin::signed(2), 7), Error::::NotMember); }); }