NCEAS / metacatui

MetacatUI: A client-side web interface for DataONE data repositories
https://nceas.github.io/metacatui
Apache License 2.0
42 stars 26 forks source link

Permissions View MVP #1232

Closed laurenwalker closed 3 years ago

laurenwalker commented 4 years ago

The MVP features:

Features to be added after the MVP:

vchendrix commented 4 years ago

Does this include support for manging the access policy for data packages?

laurenwalker commented 4 years ago

Yes

mbjones commented 4 years ago

The inability to share permissions within a lab group for datasets is a blocker to effective use of the tool. Another group using the KNB is struggling with this now. I increased the priority on this to Critical.

laurenwalker commented 4 years ago

I'll try to get it worked in as soon as we can. May have to reprioritize some other high priority features if this is truly critical.

amoeba commented 4 years ago

Just a little update here: @laurenwalker and I chatted on Slack this week about putting together a wireframe for this and I spent some time today working one up. See https://www.figma.com/file/rNgsCNoNFPRtErX5tLKudg/Permissions-View-MVP?node-id=1%3A2 (it's interactive if you press Play).

Some high-level things:

@laurenwalker @csjx do you wanna touch base next week an iterate on this over a video call? I can find a shared time slot but let me know if you have any preferences.

mbjones commented 4 years ago

I think that using the Share layout and design from the portal settings as closely as possible would help reinforce usage patterns and make it easier to learn. Plus, it is largely doing the same thing, but just being applied to a different object (either a data package or a single data object). So, couldn't we turn the nicely designed Share section from the portal into a reusable modal that could be called up in place of your modals?

share-modal

I agree with your 'default to the whole package' approach. It would be good if it were clearer that the package-level rules will also apply to objects unless an object-specific policy is added.

amoeba commented 4 years ago

So, couldn't we turn the nicely designed Share section from the portal into a reusable modal that could be called up in place of your modals?

Yep, that's exactly what I was thinking. My design just differs in some ways for fun but these UIs need to be 100% identical IMO.

It would be good if it were clearer that the package-level rules will also apply to objects unless an object-specific policy is added.

Good point. I think we might be able to use a combination of iconography and text to make it more clear. Though I think having the rules apply to the entire package will be very intuitive to people.

amoeba commented 4 years ago

Just made some updates along the lines of my latest. Shared icons are used to link things visually and text is used to make it clear what's being changed.

laurenwalker commented 4 years ago

Thanks Bryce, this looks really good. I agree with Matt that we should reuse the design and language we already have implemented (e.g. you changed "Is owner" to "Manage" but I think we want to keep "Is owner".) We've gone through several design reviews in our Portal Editor meetings of that view already so we shouldn't change anything unless it's obvious something isn't working well right now.

I agree with this and it's what we have done in the portal editor. Note that the name of the view/button is configured in the AppModel (it defaults to "Sharing options"), so we will want to use that in the view:

https://github.com/NCEAS/metacatui/blob/2d5b18d100f3d4a0067ecfad034fc5b786efdb3c/src/js/models/AppModel.js#L756

amoeba commented 4 years ago

Hey @laurenwalker , thanks for taking a look. Apologies about the confusion over how much mockups differed from the established UI. That was just for fun.

In your wireframes I see that the Share icons only show up when the row is hovered on. I like how this keeps things uncluttered but I wonder if people will miss it? I guess I'd have to see it in action.

Right. I'm usually not a fan of hiding UI behind hover and I think we might prefer to have it always who.

I'm wondering if we should remove the "Public" toggle that displays directly in each row since we have the Public toggle in the AccessPolicyView now. I think it could be confusing to have the toggle in two places.

I think that might look and work great to remove the per-DataItem checkboxes, yeah. Having the functionality in two places may be confusing as you say and hiding it behind a click and a modal should be fine because it should be an uncommon action.

I'll get started on implementing the core of this functionality this week.

laurenwalker commented 4 years ago

Sounds great, thanks

laurenwalker commented 3 years ago

Any updates on the progress of this?

amoeba commented 3 years ago

None yet, but I have time set aside for this this week.

amoeba commented 3 years ago

Initial implementation done in https://github.com/NCEAS/metacatui/commit/99af8230ab43b723a37db396031414f0b7a5a651. I started by getting public/private toggling working for the entire package and building out the modal dialog which also has a public/private toggle. Works but has some UI tweaks to hammer out. Biggest thing to start on next week is hooking in the existing AccessPolicyView so individual permissions can be edited.

amoeba commented 3 years ago

Basic and not-quite complete implementation done as of https://github.com/NCEAS/metacatui/commit/6dddbbe347c573663311000624fcdc91bbe57d24.

What we have:

There are a few things I wanna tweak still on the above:

I'll take a look at these tomorrow. @laurenwalker do you wanna take a peek and let me know how it's looking like we talked about on our dev call today? All my changes are synced with origin/feature-permissions-view-mvp.

mbjones commented 3 years ago

awesome, @amoeba ! When it makes sense, can you drop some screenshots in here of it as-implemented for a quick review?

laurenwalker commented 3 years ago

Thanks Bryce, I will take a look soon

amoeba commented 3 years ago

Hey @mbjones, now works:

Shot showing the new Public and Share columns. Clicking the Public toggle toggles public-read on and off and animates like an iOS toggle.

Screen Shot 2020-12-04 at 1 55 01 PM

Shot showing the modal dialog that pops up when you click the Share button, shown in context with the data package view behind it:

Screen Shot 2020-12-04 at 1 55 12 PM

GIF showing toggling the public/private toggle and opening the Share modal:

2020-12-04 13 56 35

laurenwalker commented 3 years ago

Looks really great so far. My thoughts from my initial review, to be discussed with team and addressed for MVP:

amoeba commented 3 years ago

Knocked out a few of these just now:

I'll work on this tomorrow:

amoeba commented 3 years ago

Added a method where any updates to the metadata's access policy get applied to the resource map too. Includes basic error handling. Committed in f219fd77d638b493f368068f5440ef0717755930. Here's my commit message:

I'm kind of inventing a system here but I expect we'll want to change it in future iterations. Rather than refactor the AccessPolicyView to support editing access policies for more than one object or building a new View that can do this, I just built a toggle on the AccessPolicyView to broadcast any changes broadcasting the change and handing off control to the DataPackage class to handle saving and error handling.

We effectively pre-flight the broadcasted change to the resource map by guarding the UI behind an access check so it's very likely the sysmeta update on the resource map will succeed (barring normal request errors). That's why I thought I could get with with what is basically a background request and with a basic error UI. Let me know what you think @laurenwalker.