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

Chore/allow collections move #1001

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Jan 20, 2025

Problem

previously we disallowed users from shifting items inside collections. this pr allows shifting of collection items between collections.

  1. this means that collection items can only be moved between collections and not into folders
  2. similarly, folder items can only be moved between folders and not into collections

Solution

  1. fetch data using getChildrenOf and filter
  2. show the menu tooltip on collection items
  3. this is a frontend only change - the backend method will still allow users if they post directly and are verified

Tests

  1. create 2 collections
  2. create a folder
  3. create pages inside the collections + the folder
  4. create links inside the collections
  5. for the page inside the folder, verify that you cannot move it to another collection
  6. for the pages inside the collection + the link, verify that you can move them between collections
  7. verify that the pages + link once moved, doesnt show inside the original collectino both on the dashboard and the sidebar
  8. verify that the pages + link once moved, shows inside the new collection on the sidebar + the dashboard

@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Jan 20, 2025

Datadog Report

Branch report: chore/allow-collections-move
Commit report: 72cae15
Test service: isomer-studio

✅ 0 Failed, 257 Passed, 36 Skipped, 44.97s Total Time
➡️ Test Sessions change in coverage: 1 no change

@seaerchin seaerchin marked this pull request as ready for review January 21, 2025 08:05
@seaerchin seaerchin requested a review from a team as a code owner January 21, 2025 08:05
@@ -59,7 +60,7 @@ const SuspendableMoveItem = ({
pl="2.25rem"
size="xs"
onClick={handleOnClick}
leftIcon={<BiFolder />}
leftIcon={type === "Collection" ? <BiData /> : <BiFolder />}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think I've seen this if-else a couple of times, might be good to standardise to use a common function that determines the icon to use

For example, ICON_MAPPINGS inside apps/studio/src/features/dashboard/components/DirectorySidebar/constants.ts has this mapping for all the resources currently.

@@ -71,7 +72,7 @@ const MoveResourceContent = withSuspense(
const parentDest = resourceStack[resourceStack.length - 2]
const curResourceId = moveDest?.resourceId
const { data, fetchNextPage, hasNextPage, isFetchingNextPage } =
trpc.resource.getFolderChildrenOf.useInfiniteQuery(
trpc.resource.getChildrenOf.useInfiniteQuery(
Copy link
Contributor

Choose a reason for hiding this comment

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

issue(perf): I think we should still continue using getFolderChildrenOf, otherwise it seems a bit heavy for the client browser to perform the filtering on the entire list of resources, rather than letting the backend perform an initial filtering to folder-like resources.

Comment on lines +271 to +272
parent.type !== ResourceType.Folder &&
parent.type !== ResourceType.Collection)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be good to add a comment about the frontend-side filtering, which makes moving items from Folder to Collection (and vice versa) not possible to a normal user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants