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

Clone cards together with the board #3430

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

Conversation

bahuma20
Copy link

@bahuma20 bahuma20 commented Nov 18, 2021

Summary

Add the possibility to clone cards together with the board.
image
Also allow customizations when cloning.
image
In advanced options, cards can be moved to the first list and archived cards can be restored.
image

Follow Up

  • Copy archived/deleted cards
  • Copy all cards to specific comments
  • Copy attachments
  • Copy comments

TODO

  • Write tests (I need help here, i don't know how to do this in Nextcloud)
  • Maybe adjust documentation. Right now only REST api doc was updated.

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

@stefan-niedermann
Copy link
Member

  1. We should use one term (clone or copy)
  2. Copy assignments, labels and due dates should be checked by default in my opinion
  3. @juliushaertl do we really want the other two checkboxes? I consider them being edge cases, but at least they should be visually seperated from the others because they have another context and are some kind of "destructive" (they "change" or "modify" the data whole copying)

@juliusknorr juliusknorr self-requested a review November 24, 2021 15:09
@danir-de
Copy link

  1. @juliushaertl do we really want the other two checkboxes? I consider them being edge cases, but at least they should be visually seperated from the others because they have another context and are some kind of "destructive" (they "change" or "modify" the data whole copying)

Restoring archived cards would in no way be an edge case!
People use deck cloning to create a complex deck preset they can clone for repeating projects/tasks.

@juliusknorr
Copy link
Member

Hello @bahuma20. I'm very sorry for the long delay on the feedback of this one, we'll have a look into it soonish to get this merged.

@@ -419,6 +419,25 @@ A 403 response might be returned if the users ability to create new boards has b

##### 200 Success

### POST /boards/{boardId}/clone - Clone a board
Copy link
Member

Choose a reason for hiding this comment

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

In order to have this available through the API the required routes would also need to be handled for the BoardApiController similar to https://github.com/nextcloud/deck/pull/3430/files#diff-f126d1440653a8583122ad9fa4e49497074f2eabe1c511d76288a547ab8f1575

@@ -80,11 +85,13 @@ class BoardService {
public function __construct(
BoardMapper $boardMapper,
StackMapper $stackMapper,
CardMapper $cardMapper,
Copy link
Contributor

Choose a reason for hiding this comment

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

We have 2 injections of CardMapper, see below at line 97.

@@ -662,6 +672,16 @@ public function clone($id, $userId) {
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can separate the board cloning logics in a different class - The BoardMapper or the Service.

return $newBoard;
}

private function cloneCards(Board $board, Board $newBoard, $withAssignments = false, $withLabels = false, $withDueDate = false, $moveCardsToLeftStack = false, $restoreArchivedCards = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can put this Card cloning logic into the CardMapper class by creating a static method.

name="notification"
icon="icon-sound"
:disabled="updateDueSetting"
:class="{ 'forced-active': board.settings['notify-due'] === 'all' }"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd better put this expression as a computed value.

@@ -103,11 +107,13 @@ public function setUp(): void {
$this->service = new BoardService(
$this->boardMapper,
$this->stackMapper,
$this->cardMapper,
Copy link
Contributor

Choose a reason for hiding this comment

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

Two CardMapper usages in the BoardService constructor.

@jancborchardt
Copy link
Member

  1. We should use one term (clone or copy)

Agree with @stefan-niedermann – we should use Copy everywhere since that is the more commonly used word.

@danir-de
Copy link

  1. We should use one term (clone or copy)
    Agree with @stefan-niedermann – we should use Copy everywhere since that is the more commonly used word.

Yes but Copy often requires pasting something and/or choosing where to copy it to.
Clone on the other hand is mostly used to create a copy (a clone) to the exact same location.

I'd say that Clone or Duplicate would be way more fitting than Copy.

@small1
Copy link
Contributor

small1 commented Dec 13, 2022

This would be nice to have merged. This would solve a problem for a few projects where they have asked for this type of feature.

@juliusknorr
Copy link
Member

I rebased this as there were quite some conflicting changes. Will have a look what is missing to get this in.

Comment on lines +652 to +655
// TODO: Undelete cards
// TODO: Copy attachments (or not?)
// TODO: Copy comments (or not?)
// TODO: Move to specific column
Copy link
Member

Choose a reason for hiding this comment

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

Still valid?

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove those from the code and track as enhancement follow up issues.

Comment on lines 663 to 665
usort($stacks, function (Stack $a, Stack $b) {
return $a->getOrder() - $b->getOrder();
});
Copy link
Member

Choose a reason for hiding this comment

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

Sorting the same as above twice?

@grnd-alt grnd-alt force-pushed the 2797-clone-cards branch 2 times, most recently from c0d6d4f to 4ce2d79 Compare May 24, 2024 13:36
make error message clear

SPDX Header BoardCloneModal.vue

Signed-off-by: grnd-alt <[email protected]>
Comment on lines +724 to +726
$this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_CARD_CREATE);
$this->changeHelper->cardChanged($card->getId(), false);
$this->eventDispatcher->dispatchTyped(new CardCreatedEvent($card));
Copy link
Contributor

Choose a reason for hiding this comment

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

$card should be replaced by $newCard

Copy link
Contributor

@luka-nextcloud luka-nextcloud left a comment

Choose a reason for hiding this comment

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

I think cypress tests should be added.

@cyrillappert
Copy link

cyrillappert commented Sep 12, 2024

We would also be very interested in this feature.

@TheBestDestroyer
Copy link

I would also be very interested in this feature. Why it has not been merged yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

duplicating a board copies its lists, but not the cards
10 participants