From e038cc7c137ff34b428da1589b736cae867c51c6 Mon Sep 17 00:00:00 2001 From: David de Kloet <122978264+dskloetd@users.noreply.github.com> Date: Fri, 10 Jan 2025 08:36:07 +0100 Subject: [PATCH] NNS1-3504: Sort selectable universes by actionable proposals (#6131) # Motivation On the voting page we want to sort the universes (both in the selector and in the actionable proposals) based on the number of actionably proposals or otherwise alphabetically. Both the selector and the actionable proposals sections are rendered based on `selectableUniversesStore`. (This store is also used for other things, such as [finding the currently selected universe](https://github.com/dfinity/nns-dapp/blob/58b8d349c2912b678f69b1060342430f6992fb12/frontend/src/lib/components/accounts/IcrcWalletPage.svelte#L166), which is why it can still also contain non-voting universes such as ckBTC.) # Changes 1. Sort `selectableUniversesStore` based on actionable proposal count and title. # Tests 1. Simplify the existing tests for `selectableUniversesStore`. Because of the re-ordering, the tests were confusing in their previous form. 2. Add unit tests with multiple SNS projects. 3. Adapt `ActionableProposals.spec.ts` to the new ordering. 4. Manually tested at https://qsgjb-riaaa-aaaaa-aaaga-cai.dskloet-ingress.devenv.dfinity.network/proposals/?actionable # Todos - [x] Add entry to changelog (if necessary). --- CHANGELOG-Nns-Dapp-unreleased.md | 1 + .../derived/selectable-universes.derived.ts | 66 +++++++- .../selectable-universes.derived.spec.ts | 148 ++++++++++++------ .../lib/pages/ActionableProposals.spec.ts | 16 +- 4 files changed, 171 insertions(+), 60 deletions(-) diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index e5994a3ae6c..1ea39d8685d 100644 --- a/CHANGELOG-Nns-Dapp-unreleased.md +++ b/CHANGELOG-Nns-Dapp-unreleased.md @@ -22,6 +22,7 @@ proposal is successful, the changes it released will be moved from this file to - Allow `dfinity:` token prefix in QR code for ICP payment. - Recover from interrupted canister creation in the frontend. +- Sort projects on the Voting page based on actionable proposals and name. #### Deprecated diff --git a/frontend/src/lib/derived/selectable-universes.derived.ts b/frontend/src/lib/derived/selectable-universes.derived.ts index 49b98cf08cf..7b889338aac 100644 --- a/frontend/src/lib/derived/selectable-universes.derived.ts +++ b/frontend/src/lib/derived/selectable-universes.derived.ts @@ -1,3 +1,8 @@ +import { OWN_CANISTER_ID_TEXT } from "$lib/constants/canister-ids.constants"; +import { + actionableProposalCountStore, + type ActionableProposalCountData, +} from "$lib/derived/actionable-proposals.derived"; import { icrcCanistersStore, type IcrcCanistersStore, @@ -5,26 +10,71 @@ import { } from "$lib/derived/icrc-canisters.derived"; import { pageStore, type Page } from "$lib/derived/page.derived"; import type { Universe } from "$lib/types/universe"; +import { + createAscendingComparator, + createDescendingComparator, + mergeComparators, +} from "$lib/utils/sort.utils"; import { isAllTokensPath, isUniverseCkBTC } from "$lib/utils/universe.utils"; import { isNullish } from "@dfinity/utils"; import { derived, type Readable } from "svelte/store"; import { universesStore } from "./universes.derived"; +type UniverseWithActionableProposalCount = { + universe: Universe; + actionableProposalCount: number; +}; + +const compareNnsFirst = createDescendingComparator( + ({ universe: { canisterId } }: UniverseWithActionableProposalCount) => + canisterId === OWN_CANISTER_ID_TEXT +); + +const compareActionableProposalCount = createDescendingComparator( + ({ actionableProposalCount }: UniverseWithActionableProposalCount) => + actionableProposalCount +); + +const compareTitle = createAscendingComparator( + ({ universe: { title } }: UniverseWithActionableProposalCount) => + title.toLowerCase() +); + export const selectableUniversesStore = derived< - [Readable, Readable, IcrcCanistersStore], + [ + Readable, + Readable, + IcrcCanistersStore, + Readable, + ], Universe[] >( - [universesStore, pageStore, icrcCanistersStore], - ([universes, page, icrcCanisters]: [ + [universesStore, pageStore, icrcCanistersStore, actionableProposalCountStore], + ([universes, page, icrcCanisters, actionableProposalCounts]: [ Universe[], Page, IcrcCanistersStoreData, + ActionableProposalCountData, ]) => // Non-governance paths show all universes // The rest show all universes except for ckBTC, and ICRC Tokens - universes.filter( - ({ canisterId }) => - isAllTokensPath(page) || - (!isUniverseCkBTC(canisterId) && isNullish(icrcCanisters[canisterId])) - ) + universes + .filter( + ({ canisterId }) => + isAllTokensPath(page) || + (!isUniverseCkBTC(canisterId) && isNullish(icrcCanisters[canisterId])) + ) + .map((universe) => ({ + universe, + actionableProposalCount: + actionableProposalCounts[universe.canisterId] ?? 0, + })) + .sort( + mergeComparators([ + compareNnsFirst, + compareActionableProposalCount, + compareTitle, + ]) + ) + .map(({ universe }) => universe) ); diff --git a/frontend/src/tests/lib/derived/selectable-universes.derived.spec.ts b/frontend/src/tests/lib/derived/selectable-universes.derived.spec.ts index 6ef21dbe899..04ea195d0cd 100644 --- a/frontend/src/tests/lib/derived/selectable-universes.derived.spec.ts +++ b/frontend/src/tests/lib/derived/selectable-universes.derived.spec.ts @@ -1,18 +1,15 @@ import { OWN_CANISTER_ID } from "$lib/constants/canister-ids.constants"; -import { CKBTC_UNIVERSE_CANISTER_ID } from "$lib/constants/ckbtc-canister-ids.constants"; -import { CKETH_UNIVERSE_CANISTER_ID } from "$lib/constants/cketh-canister-ids.constants"; import { AppPath } from "$lib/constants/routes.constants"; import { selectableUniversesStore } from "$lib/derived/selectable-universes.derived"; -import { snsProjectsCommittedStore } from "$lib/derived/sns/sns-projects.derived"; +import { actionableSnsProposalsStore } from "$lib/stores/actionable-sns-proposals.store"; import { page } from "$mocks/$app/stores"; -import { - mockProjectSubscribe, - mockSnsFullProject, -} from "$tests/mocks/sns-projects.mock"; +import { principal } from "$tests/mocks/sns-projects.mock"; +import { mockSnsProposal } from "$tests/mocks/sns-proposals.mock"; import { resetCkETHCanisters, setCkETHCanisters, } from "$tests/utils/cketh.test-utils"; +import { setSnsProjects } from "$tests/utils/sns.test-utils"; import { get } from "svelte/store"; describe("selectable universes derived stores", () => { @@ -27,64 +24,125 @@ describe("selectable universes derived stores", () => { it("should return Nns, ckBTC and ckTESTBTC (flag for test is true) per default", () => { const store = get(selectableUniversesStore); - expect(store.length).toEqual(3); - expect(store[0].summary).toBeUndefined(); - expect(store[0].canisterId).toEqual(OWN_CANISTER_ID.toText()); - expect(store[1].summary).toBeUndefined(); - expect(store[1].canisterId).toEqual(CKBTC_UNIVERSE_CANISTER_ID.toText()); + expect(store.map(({ title }) => title)).toEqual([ + "Internet Computer", + "ckBTC", + "ckTESTBTC", + ]); }); it("should return CkETH in Accounts page", () => { setCkETHCanisters(); const store = get(selectableUniversesStore); - expect(store.length).toEqual(4); - expect(store[3].summary).toBeUndefined(); - expect(store[3].canisterId).toEqual(CKETH_UNIVERSE_CANISTER_ID.toText()); + expect(store.map(({ title }) => title)).toEqual([ + "Internet Computer", + "ckBTC", + "ckETH", + "ckTESTBTC", + ]); }); - it("should not return ckBTC if path is not Account", () => { + it("should not return ckBTC or ckETH if path is not Account", () => { page.mock({ routeId: AppPath.Neurons, data: { universe: OWN_CANISTER_ID.toText() }, }); const store = get(selectableUniversesStore); - // 1 length = only NNS - expect(store.length).toEqual(1); - expect(store[0].canisterId).not.toEqual( - CKBTC_UNIVERSE_CANISTER_ID.toText() - ); + expect(store.map(({ title }) => title)).toEqual(["Internet Computer"]); }); - it("should not return ckETH if path is not Account", () => { - page.mock({ - routeId: AppPath.Neurons, - data: { universe: OWN_CANISTER_ID.toText() }, + describe("with projects", () => { + it("should return Nns, ckBTC, ckTESTBTC (flag for test is true) and another project", () => { + setSnsProjects([ + { + projectName: "SNS Project", + }, + ]); + const store = get(selectableUniversesStore); + expect(store.length).toEqual(4); + expect(store.map(({ title }) => title)).toEqual([ + "Internet Computer", + "ckBTC", + "ckTESTBTC", + "SNS Project", + ]); }); - setCkETHCanisters(); - const store = get(selectableUniversesStore); - // 1 length = only NNS - expect(store.length).toEqual(1); - expect(store[0].canisterId).not.toEqual( - CKBTC_UNIVERSE_CANISTER_ID.toText() - ); - }); + it("should return NNS followed by SNS projects ordered alphabetically", () => { + page.mock({ + routeId: AppPath.Proposals, + data: { universe: OWN_CANISTER_ID.toText() }, + }); - describe("with projects", () => { - beforeEach(() => { - vi.spyOn(snsProjectsCommittedStore, "subscribe").mockImplementation( - mockProjectSubscribe([mockSnsFullProject]) - ); + const rootCanisterId1 = principal(1); + const rootCanisterId2 = principal(2); + const rootCanisterId3 = principal(3); + + setSnsProjects([ + { + rootCanisterId: rootCanisterId1, + projectName: "Bravo", + }, + { + rootCanisterId: rootCanisterId2, + projectName: "Alfa", + }, + { + rootCanisterId: rootCanisterId3, + projectName: "Charlie", + }, + ]); + + expect(get(selectableUniversesStore).map(({ title }) => title)).toEqual([ + "Internet Computer", + "Alfa", + "Bravo", + "Charlie", + ]); }); - it("should return Nns, ckBTC, ckTESTBTC (flag for test is true) and another project", () => { - const store = get(selectableUniversesStore); - expect(store.length).toEqual(4); - expect(store[3].summary).not.toBeUndefined(); - expect(store[3].canisterId).toEqual( - mockSnsFullProject.rootCanisterId.toText() - ); + it("should return NNS followed by SNS projects in reverse order of actionable proposals", () => { + page.mock({ + routeId: AppPath.Proposals, + data: { universe: OWN_CANISTER_ID.toText() }, + }); + + const rootCanisterIdA = principal(1); + const rootCanisterIdB = principal(2); + const rootCanisterIdC = principal(3); + + setSnsProjects([ + { + rootCanisterId: rootCanisterIdB, + projectName: "Bravo", + }, + { + rootCanisterId: rootCanisterIdA, + projectName: "Alfa", + }, + { + rootCanisterId: rootCanisterIdC, + projectName: "Charlie", + }, + ]); + + actionableSnsProposalsStore.set({ + rootCanisterId: rootCanisterIdC, + proposals: [mockSnsProposal, mockSnsProposal], + }); + + actionableSnsProposalsStore.set({ + rootCanisterId: rootCanisterIdB, + proposals: [mockSnsProposal], + }); + + expect(get(selectableUniversesStore).map(({ title }) => title)).toEqual([ + "Internet Computer", + "Charlie", + "Bravo", + "Alfa", + ]); }); }); }); diff --git a/frontend/src/tests/lib/pages/ActionableProposals.spec.ts b/frontend/src/tests/lib/pages/ActionableProposals.spec.ts index 1a1ff31ec9c..5faeb1c16e9 100644 --- a/frontend/src/tests/lib/pages/ActionableProposals.spec.ts +++ b/frontend/src/tests/lib/pages/ActionableProposals.spec.ts @@ -137,6 +137,8 @@ describe("ActionableProposals", () => { await po.getActionableSnses().getActionableSnsProposalsPos() ).toHaveLength(0); + // NOTE: Projects are sorted by reverse order of number of actionable + // proposals. actionableSnsProposalsStore.set({ rootCanisterId: principal0, proposals: [snsProposal0], @@ -155,20 +157,20 @@ describe("ActionableProposals", () => { await snsProposalsPos[0] .getUniverseWithActionableProposalsPo() .getTitle() - ).toEqual("Sns Project 0"); + ).toEqual("Sns Project 1"); const proposalCardPos0 = await snsProposalsPos[0].getProposalCardPos(); - expect(proposalCardPos0.length).toEqual(1); - expect(await proposalCardPos0[0].getProposalId()).toEqual("ID: 11"); + expect(proposalCardPos0.length).toEqual(2); + expect(await proposalCardPos0[0].getProposalId()).toEqual("ID: 22"); + expect(await proposalCardPos0[1].getProposalId()).toEqual("ID: 33"); expect( await snsProposalsPos[1] .getUniverseWithActionableProposalsPo() .getTitle() - ).toEqual("Sns Project 1"); + ).toEqual("Sns Project 0"); const proposalCardPos1 = await snsProposalsPos[1].getProposalCardPos(); - expect(proposalCardPos1.length).toEqual(2); - expect(await proposalCardPos1[0].getProposalId()).toEqual("ID: 22"); - expect(await proposalCardPos1[1].getProposalId()).toEqual("ID: 33"); + expect(proposalCardPos1.length).toEqual(1); + expect(await proposalCardPos1[0].getProposalId()).toEqual("ID: 11"); }); it("should have actionable query parameter in card href", async () => {