Skip to content
This repository has been archived by the owner on Jun 12, 2022. It is now read-only.

support deleting board #73

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zhan9san
Copy link
Contributor

Fix #52

I just implement a small feature with simple UI, deleting a board, I hope you don't mind.
Could you kindly review it?

Question: Currently, column stats are deleted once a board is deleted. I am not sure whether task, comment, member and label states should be deleted as well. Do you have any suggestions?
If they are expected to be deleted, besides adding the following snippet in each single slice function, is there other ways?

    builder.addCase(deleteBoard.fulfilled, (state) => {
      columnAdapter.removeAll(state);
    });

Deleting a board is a dangerous operation, so it's better to close board first and then delete closed board like deleting-a-board in trello.
But it will introduce more effort to implement more feature as follow,

  1. Close a board
  2. List closed boards
  3. Re-open a closed board
  4. Delete a closed board permanently
  5. [Optional] Add a nested menu bar showing close board
  6. etc.

@rrebase
Copy link
Owner

rrebase commented Dec 13, 2021

Currently, column stats are deleted once a board is deleted. I am not sure whether task, comment, member and label states should be deleted as well.

It makes sense to delete all board related data: columns, tasks, comments (it's possible to make the deletions cascade in case of FK relationships)

besides adding the following snippet in each single slice function, is there other ways

Yup, need removeAll with deleteBoard.fulfilled for all relevant slices, not aware of other approaches from toolkit alone.

Deleting a board is a dangerous operation, so it's better to close board first and then delete closed board like deleting-a-board in trello.

Absolutely, but this can be a follow up and an extra confirmation alert (as we have for comment deletions) should be enough at first :)

@zhan9san
Copy link
Contributor Author

Hi @rrebase

Thanks for your suggestions.

It makes sense to delete all board related data: columns, tasks, comments (it's possible to make the deletions cascade in case of FK relationships)

I am sorry I didn't express myself clearly. I noticed the deletions cascade is supported in backend originally.
I mean do we need to delete the related stat in frontend? Like the second commit in this PR. Commit ID

need removeAll with deleteBoard.fulfilled for all relevant slices

As I don't have much experience in React/Redux, is there a method which is similar to deletions cascade in Redux?
If yes, there is no need to delete state explicitly 5 times, CommentSlice.tsx, ColumnSlice.tsx, LabelSlice.tsx, MemberSlice.tsx and TaskSlice.tsx.
It seems the answer is no.

BTW, If a column or task is deleted, the related comment would not be deleted in frontend while the related tasks would be deleted once a column is deleted. Do we need to create another issue/PR to handle it?

Could you kindly review it again?

@rrebase
Copy link
Owner

rrebase commented Dec 19, 2021

I mean do we need to delete the related stat in frontend

Yea, it makes sense to clean up the store (even though not strictly necessary as the data isn't persisted).

is there a method which is similar to deletions cascade

With the simple redux-toolkit setup, there's no cascade feature that would avoid the explicit deletion form each slice afaik.

BTW, If a column or task is deleted, the related comment would not be deleted in frontend while the related tasks would be deleted once a column is deleted. Do we need to create another issue/PR to handle it?

Good catch, feel free to create an issue/PR to resolve this :)

Comment on lines 160 to 165
const indexOfDeletedBoard = state.all.findIndex((obj) => {
if (obj.id == action.payload) {
return true;
}
return false;
});
Copy link
Owner

@rrebase rrebase Dec 19, 2021

Choose a reason for hiding this comment

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

We can keep this short

Suggested change
const indexOfDeletedBoard = state.all.findIndex((obj) => {
if (obj.id == action.payload) {
return true;
}
return false;
});
const indexOfDeletedBoard = state.all.findIndex(board => board.id === action.payload);

Copy link
Contributor Author

@zhan9san zhan9san Dec 20, 2021

Choose a reason for hiding this comment

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

Thank you @rrebase

The type of board.id is number while the type of action.payload and return value of deleteBoard is string.

So a new commit make all of them above be restricted to be string like fetchUserById typing-createasyncthunk does.

const { id } = useParams(); is used which makes the type of id is any. Actually, it passes a string type to Id/number type. That's why dispatch(deleteBoard(id)); works before.

Let me know if you have any concern.

@zhan9san zhan9san force-pushed the feature/delete-board-name branch from eb50466 to 7a39987 Compare December 28, 2021 03:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rename and delete option for the board
2 participants