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

Swik 907 delete deck and transfer ownership #1073

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

Conversation

TBoonX
Copy link
Member

@TBoonX TBoonX commented Oct 29, 2018

I added a new button to deck edit and a workflow for delete/remove/transfer ownership of a deck. Not all deck routes are working yet.
Please verify that the workflow is correct.
Do you have suggestions for visual improve the user list when transfering ownership?

Missing:

  • when being on a subdeck - differences?
  • Intl texts

@TBoonX
Copy link
Member Author

TBoonX commented Oct 29, 2018

TODO is also Kostis comment:

Not sure if I have it in the flow, but all those options should be available only to the owner/creator of the subdeck or deck we want to delete. That is, for a simple editor of the deck in question:

  1. the deck edit panel SHOULD NOT have the delete deck button
  2. the remove subdeck button SHOULD NOT show the modal to choose from, action should be as it is now.

@kprist
Copy link
Member

kprist commented Oct 30, 2018

I have opened PR slidewiki/deck-service#149 ( not ready yet of course ), and also filled in some missing calls in this branch as well to better test.

  1. we should have an "are you sure" ? message before deleting a deck when none of the cases where we prompt with info or for more actions to the user.
  2. instead of the home page, after deleting a deck we should redirect to the user's "my decks"
  3. my suggestion for the delete subdeck modal
    image was that we show this when user clicks on "remove subdeck" icon to to the top right of the toolbar:
    image, in case the only parent (usage) of this deck was the parent currently displayed as well, and from which the user wants to remove the subdeck.
  4. likewise, there should not exist a "delete deck" button under a subdeck ( regardless if it was loaded as such: a subdeck is a deck that has usage )
  5. some modals don't have "cancel":
    image
  6. in the previous modal, I hit esc and the transfer modal appeared, I think it's a bug, or something Kurt left TODO in case we wanted to show the tranfer panel. For subdecks I think we should keep it simple, either remove from tree (like before) or delete altogether.
  7. in other cases the modal shows fine, it also lists all current members of all groups it seems, even greater 👍

For deleting a subdeck, you should use actions/decktree/deleteTreeNode that uses the API, /decktree/node/delete. If the user has selected to also delete the node, please send in addition to the selector a property purge equal to true . The service will also delete the node if it's a deck and if the restrictions we are discussing apply.

@TBoonX
Copy link
Member Author

TBoonX commented Oct 30, 2018

What are the possible use cases?

  1. deletion of deck with no usage
    1.1 no editors - just deletion
    1.2 transfer ownership instead of delete the whole deck
  2. deletion/removable of subdeck
    2.1 viewing the subdeck as subdeck in the parent deck
    2.1.1 delete subdeck as child everywhere and deck completely
    2.1.2 remove subdeck from current parent parent
    2.1.3 remove subdeck from all parent parents
    2.2 viewing subdeck as own deck
    2.2.1 delete subdeck as child everywhere and deck completely
    2.2.2 remove subdeck from all parent decks

Thus the delete button in edit view for root decks and subdecks opened as own decks. When viewing a subdeck as child of its parent we have the trash icon button for deleting the node.

… deck edit only on roots and added modal when removing a subdeck as node.
@kprist
Copy link
Member

kprist commented Oct 30, 2018

I'm confused with (2) use cases, let me rephrase them as I understand them:

  1. deletion/removal of subdeck
    2.1 viewing the subdeck as subdeck in the parent deck
    2.1.1 remove subdeck from current tree AND delete, as long as usage is limited to current parent
    2.1.2 remove subdeck from current tree (exactly what we do now)
    2.1.3 remove subdeck from all parent parentsif a subdeck is subdeck to other parents, do not offer option to delete.
    2.2 viewing subdeck as own deck => do not offer option to delete at all (like we have now)
    2.2.1 delete subdeck as child everywhere and deck completely
    2.2.2 remove subdeck from all parent decks

@TBoonX
Copy link
Member Author

TBoonX commented Oct 30, 2018

If 2.1.1 could have more than one usage entry, then 2.1.3 is valid because usage and parents is the same thing?

@kprist
Copy link
Member

kprist commented Oct 30, 2018

2.1.1 means there's only one usage: the parent currently in view
2.1.3 if there are more that one parents in usage, we don't support deleting, ONLY removing from current parent -> it will still be available in some other deck tree

TBoonX and others added 2 commits October 30, 2018 17:29
…on and related code. Change of precondition to be able to delete/remove a deck
@kprist kprist self-assigned this Nov 1, 2018
@kprist kprist removed their request for review November 1, 2018 10:10
@kprist
Copy link
Member

kprist commented Nov 13, 2018

@abijames I updated the branch with the changes we discussed during amsterdam plenary, so in order to delete a subdeck user simply removes it, then they can delete it after loading it from "My decks". So, no option is given whether to simply remove or also delete. I've also moved the delete button to the right so it's more separated from save/cancel buttons.

Also, provided a fix for the tranfer ownership dialog not closing sometimes. But I would like @TBoonX to look at that as well, as I commented out some code I didn't know much about.

}

unmountTrap() {
this.handleClose();
Copy link
Member

Choose a reason for hiding this comment

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

@TBoonX I'm not sure about this. Could you review this piece of code ?

@abijames
Copy link
Contributor

abijames commented Nov 23, 2018

I tested this on athena server and have feedback to improve the UI. Functionally it all worked well except for 2 issues:

  1. I transferred ownership of a deck. A fork of the deck appeared in the search results and still listed me as the original author. This caused 2 problems
    1.1 when you clicked on the original deck the author changed to the new owner
    1.2 when you click on the link for the original author in the forked deck you get taken to a My Decks list which no longer contains the original deck
  2. when I transferred a published deck, it become unlisted - is this correct?

Suggested UI changes and accessibility issues:

  1. On deck edit the Delete button should be named "Delete Deck" and it could also have the trash can icon
  2. When attempting to delete a deck with a sub-deck, change the message to: "You can only delete decks with no sub-decks. Please remove the sub-decks first and try again."
  3. Message when deleting a deck
    3.1 Does this need to include the deck ID? We don't show this to the user at any other time
    3.2 can you make the buttons Green for OK
    3.3 Change message on this dialog to: "Do you want to delete the deck "[deck name]"? This action can not be reverted."
  4. After deleting a deck a message appears say "Success" with a confirmation button. Change the button to "OK".

Transfer ownership:

  1. You can't select another owner with the keyboard. Have a look at attach deck modal for a keyboard accessible implementation. At the moment when you tab you go to the link for the authors user name.
  2. There needs to be a more visible indication of which editor you've selected. Very unclear when there is more than editor to choose.
  3. Change the text in the message to: "Other users have rights to edit this deck. You can transfer the deck to another editor or delete the deck for everyone."
  4. Change the text in the buttons to [Transfer deck] [Delete deck] [Cancel].
  5. If you select delete deck then you should see the warning modal as you do when you delete a deck without other editors.

Copy link
Contributor

@abijames abijames left a comment

Choose a reason for hiding this comment

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

See comments in thread

@abijames
Copy link
Contributor

Tested on Athena server. Have striked out my comments that have been addressed.

@kprist
Copy link
Member

kprist commented Nov 29, 2018

@abijames regarding the issues you've had:

I transferred ownership of a deck. A fork of the deck appeared in the search results and still listed me as the original author.

Your transferring does not affect a fork. If that fork was owned by you, it will remain, unless you transfer ownership of that as well.

1.1 when you clicked on the original deck the author changed to the new owner

This is to be the expected, why is it an issue?

1.2 when you click on the link for the original author in the forked deck you get taken to a My Decks list which no longer contains the original deck

Also correct, transferring ownership removes the deck from the original author's "My Decks". Although I'm not sure where that link is, is it in the search results?

  1. when I transferred a published deck, it become unlisted - is this correct?

This is intended, yes, so as to allow the new deck owner to review this deck before letting this get published as their own.

@abijames
Copy link
Contributor

abijames commented Nov 29, 2018

@kprist, thanks for comments. Re (1) my corner is the lack of consistency. Opening the deck from search results to the landing page then the owner of the deck changes. It would be better if the new owner was shown in the search results instead

@kprist
Copy link
Member

kprist commented Nov 29, 2018

I see, if it's the same deck and their metadata change, that is a bug, I think the search indexer indeed fails to update the deck owner.

@kprist
Copy link
Member

kprist commented Nov 29, 2018

@abijames I've resolved most of your comments in the athena deployment. Only thing missing should be

If you select delete deck then you should see the warning modal as you do when you delete a deck without other editors.

@kprist
Copy link
Member

kprist commented Nov 29, 2018

@abijames I also verified tranferred owners are properly updated with the search service. There was an issue with some updates in deck service master not in this branch that prevented proper search index updates. Please re-check.

Keep in mind that after transferring a deck, it should be removed from search index (as it is also unlisted). The new owner needs to re-publish it it order to be discoverable.

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

Successfully merging this pull request may close these issues.

3 participants