DMPRoadmap / roadmap

DCC/UC3 collaboration for a data management planning tool
MIT License
102 stars 110 forks source link

Co-owners should not be able to delete the plan #242

Closed briri closed 7 years ago

briri commented 7 years ago

This is an issue that apparently exists in the DMPonline_V4 codebase but we should correct it in Roadmap.

If you share your plan with someone else and give them administrator access, they have the ability to delete the plan. The plan should not get deleted in this situation, we should just remove their access to the plan so that it falls off of their 'View Plans' page.

sjDCC commented 7 years ago

I'm happy for this to be addressed in a later release. Doesn't strictly need to be in MVP v.0.3. There's a warning at present which isn't ideal but works as a stopgap.

briri commented 7 years ago

Ah ok. Changing it to MVP instead of MVP-0.3 and moving it off the board for now

stephaniesimms commented 7 years ago

I can't test this @briri because I can't access any new user accounts. The invitation email contains a link that gives me this error (screenshot 1). And when I try to login directly, I get the second error. This is related to #271

I'm also not receiving invitation emails anymore. The email link I used to test this was from an attempt to create an account yesterday when the email was working...

screen shot 2017-06-01 at 2 10 01 pm

account-creation-user-invited-error

stephaniesimms commented 7 years ago

This is working as expected now. The only change is cosmetic: the table formatting needs to be fixed for 3 columns w/balanced widths (screenshot below of current format and corrected format). The table appears w/correct formatting automatically when there's a value in the third column.

roadmap dev screen shot 2017-06-05 at 1 31 26 pm

roadmap stage screen shot 2017-06-05 at 1 36 06 pm

stephaniesimms commented 7 years ago

Table formatting looks great now - standardized 3 column widths. All works as expected.

sjDCC commented 7 years ago

Share page isn't giving me options at the moment so can't test. Have two accounts ready though so just drop me a line when good to go. share

I still see this warning though if I go to delete. It must be generated regardless of whether the plan has been shared or not, so perhaps remove in all cases? warning

FYI, the 'share' action from the dropdown list on 'my plans' page is throwing one of the red error screens. Assume this is because there isn't content in page.

stephaniesimms commented 7 years ago

This is not exactly the behavior we want. Currently, only the plan owner sees the option to "Delete" a plan in the Actions menu but not a co-owner, editor, etc (screenshot 1). Also, if the plan owner does decide to delete the plan they get the warning message that it will be deleted for everyone with whom they shared the plan (screenshot 2).

Desired behavior is for everyone with access to the plan to be able to delete/remove it from their list without affecting other users' access to that plan. If that's not possible from a database pov, then we at least want to allow co-owners, editors, read only to "delete/remove" the plan from their list - this would essentially be allowing them to remove their own access via the My plans table.

screen shot 2017-06-29 at 12 05 16 pm

screen shot 2017-06-29 at 12 05 36 pm

vyruss commented 7 years ago

@stephaniesimms @sjDCC that sounds OK. To avoid confusion, I would suggest using the term "Remove" (or "Leave plan" - in the context of the user leaving this collaboration) for co-owners/editors/readonly. An owner shouldn't be able to leave (only delete). Then we could ask:

Are you sure you wish to remove yourself from (/leave) this plan? If you wish to add it to your plan list later, you will have to request access again.

stephaniesimms commented 7 years ago

@JEK-III can you recommend a different term (not "Delete") for the plan Actions menu for the following use case. See @vyruss suggestion to use "Remove," or maybe "Remove access"?

bhavi commented 7 years ago

@stephaniesimms, it seems reasonable for other users to "Remove" the plan from their Dashboard when they no longer want to a part of the plan. But if an owner wants to delete the plan, wats the point to keep the active for other users?

stephaniesimms commented 7 years ago

@bhavi a Co-owner of a plan is most likely an equal collaborator on the research project and should have the option of maintaining a record even if the primary Owner wants to delete/remove it from their own account. And anyone with access to the plan would potentially want to refer back to it, e.g., to copy parts for related or ongoing projects, to use as a foundation to write a new plan, etc. Admins who provide comments on plans also like to refer back to them. We want to encourage maintaining DMPs as a living record and enable people to recycle them when appropriate.

briri commented 7 years ago

@stephaniesimms the scenario you outlined above could lead to situations where a plan has been removed by everyone involved (creating an orphaned record). There would at that point be no feasible way for someone to reference it again.

Does making the plan's visibility Public further complicate things?

If we use the 'Remove' terminology on the screen for everyone (owners, co-owners, readers, commenters) can we at the very least delete the plan once it no longer has anyone associated with it? (deleting it when the last user clicks remove)

JEK-III commented 7 years ago

In a very quick survey, it looks like "Remove" is the most common term for this situation. Remove is a little more clear when accompanied by a trash icon, but I think it'll be ok on its own. "Remove access" seems confusing and an invitation to think a little more deeply about the mechanics of all this than is really necessary.

I had the same question as @briri about public plans. If the behavior is that they stick around (which is fine), a user who creates a plan, makes it public, and then remove/deletes it is not going to expect it to still be public. We could have a modal that pops up to say "hey, you made this plan public, do you want it to disappear?" but that might be more effort than it's worth.

stephaniesimms commented 7 years ago

"Remove" sounds good. And yes @briri , we should delete the plan once it no longer has anyone associated with it to avoid orphaned records. This includes removing a public plan - we can handle this situation with a modal as @JEK-III suggested or just update the current message a user receives when they select Delete (screenshot below) with a generic msg that states:

Are you sure you wish to remove this plan? If the plan is not being shared with other users, this action will delete it from the system permanently.

screen shot 2017-07-03 at 1 08 52 pm

vyruss commented 7 years ago

I think there's potential for confusion here. What are our use cases?

stephaniesimms commented 7 years ago

after further discussion w/ @briri it sounds like we can achieve the desired behavior and avoid orphaned records. we should also consider the implications of deleting plans and what admins see in the usage dashboard. we may want to never delete a plan permanently from the database and just remove access as described above. this would enable admins/institutions to track usage accurately over time.

@sjDCC are there any policy/legal reasons that plans should be deleted permanently? if not, we should consider keeping everything. i believe we do this now for the dmptool - @briri can confirm.

to clarify the use cases:

xsrust commented 7 years ago

Reading through the comments, I think the following implementation is sensible:

If we were to remove the role instead of 'de-activating' it, there would be two un-desired effects:

sjDCC commented 7 years ago

Hi @stephaniesimms @xsrust - the comments in your last two posts sound right to me.

AFAIK, we haven't deleted plans from DMPonline either, just made them inactive. In some jurisdictions I think people need to be able to show they've removed all personal data (for this reason I think Benjamin was planning an actual delete button for OPIDoR), but it sounds like unpublishing / deactivating works for both of us.

I like the term 'Remove' too. In effect it's removing it from the users list, and perhaps unpublishing if they're the plan owner, but allowing others to still view/edit etc.

xsrust commented 7 years ago

@sjDCC @stephaniesimms Would you be able to advise on the pop-up confirmation text for removing yourself from a plan? There are two cases, and here is my sample text: 1) You are the creator and the plan is publicly visible "Are you sure you wish to remove yourself from this plan? It will still be available to others. This will also remove your plan from the Public DMP's page" 2) Everything else "Are you sure you wish to remove yourself from this plan? It will still be available to others."

vyruss commented 7 years ago

@xsrust @sjDCC @stephaniesimms I'd say we can omit "yourself from this" and may I suggest:

  1. "Are you sure you wish to remove this public plan? This will remove it from the Public DMPs page but any collaborators will still be able to access it."
  2. "Are you sure you wish to remove this plan? Any collaborators will still be able to access it."
sjDCC commented 7 years ago

Thanks @xsrust. I'd change the wording slightly to say they're removing it from their list as I think the main reason people do this is to clean up their display. I also think we should have different messages whether the plan has been shared or not. It's confusing to say it will still be available to others if it hasn't be shared, though I guess this could mean institutional access so is maybe not such an odd concept?

I would adjust it slightly to say:

You are the creator, it is publicly visible but not shared "Are you sure you wish to remove this plan from your list? As your plan has been published, this action will also remove it from the Public DMPs page"

You are the creator, it has been shared and is also public "Are you sure you wish to remove this plan from your list? It will still be available to others. As your plan has been published, this action will also remove it from the Public DMP's page"

Everything else (which I think means it's always a shared plan) "Are you sure you wish to remove this plan from your list? It will still be available to others."

sjDCC commented 7 years ago

I think you can scrap my suggestions in favour of @vyruss's They're clearer.

The first suggestion works for plans that have been shared or not as 'any' doesn't suggest others can already access it.

xsrust commented 7 years ago

Thanks. This is implemented and I'll submit a PR once @stephaniesimms confirms she's happy with the text that Jimmy submitted above.

stephaniesimms commented 7 years ago

thanks all for the clear comms unraveling this issue. i agree that "collaborators" is a good term and am happy with the text that @vyruss proposed. go forth and PR @xsrust !

sjDCC commented 7 years ago

The revised notifications on this look good, but the actual link throws an error.

error

This happens for public plans and other visibilities. Is this a new ticket or can it be fixed in this one?

Also, I noticed that we've lost the column in the table that says whether a plan has been shared or not. That might be useful to reinstate as there's otherwise no way to tell at a glance. If the error message made people question whether there were collaborators, they'd have to open up the plan and check the 'sharing' tab to see. What do you think @stephaniesimms

xsrust commented 7 years ago

Appologies, I was asked to rename the method from archive to deactivate, and forgot to do so in one of the files. This will be addressed in a PR today

stephaniesimms commented 7 years ago

This is working as expected now. @sjDCC I agree that it's better for users to see which plans have been shared at a glance so I created a new issue to reinstate the "Shared?" column #545 . Closing this issue.