Add ACL selection to upload

[owi92]

This adds the ACL selection from #911 to the upload page. With this, users can choose which users and/or groups to give access to their uploads.

There are still some open tasks do be done for this to be production ready, but the core feature of this PR should be fully functioning in the current iteration/deployment.

Todos:

• list all possible groups and add good labels + translations for these (not hardcoded)
• list all users known to tobira (not hardcoded)
• maybe add ui test for acl selection?

[github-actions]

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

[LukasKalbertodt]

Okay, first review, purely from the deployment, not looking at the code. I really like it already! I'm confident we will have a really good ACL UI in Tobira! Though, as usual my long list of thoughts and ideas:

(Edit: these are all for the upload page, as I somehow assumed this PR would not include the access management for existing videos. I might have further thoughts on that later.)

• I think there should be a heading, like "Access"
• More space above "save and finish" button
• Maybe also add quite a bit of space above the footer -> avoids large dropdown lists reaching beyond the footer
• Maybe special case "yourself", i.e. write "You" in italic instead of "Morgan Yu"? Or "You (Morgan Yu)".
• On screen width around 900px and 700px, the "users" table is shown below the other (good), but neither stretch to full page width. Maybe stretch to full width? Or center align it then?
• Remove note "Videos uploaded here will be public".
• In dark mode, the drop down menu has a bug (I'd say): the colored background
  
• I would use "Read & Write" as label/title for the second permission level instead of just "Write".
  • And then also make the selection thingy (the trigger, not the floating) have a min-width such that both "read" and "read & write" have the same width.
• I would add line-height: 1.4 to the descriptions of the two permission levels.
• I would make the button like 300px instead of 200 wide, otherwise the description spans quite a few lines.
• I would maybe rephrase the permission level descriptions a bit. GitHub uses incomplete sentences "Can read and clone this repository". I wouldn't mind that instead of the "users with X-access are able to". Shortens it a bit. Unless you'd say that it makes it less clear. My random suggestion:
  • Read: "Can watch the video and view metadata"
  • Read & Write: "Can watch & edit the video. Can also view & modify metadata and other details."
• Maybe the searchable selects should have the label "Select groups to add" instead of just "select groups"? Or some other formulation that implies the user can add groups that way?

Also: Not related to this PR but considering that this page will grow longer and longer (the "access" section you added, and more metadata in the future), maybe we want to make the progress bar (without cancel button, I'd say) sticky so that they can always be seen at the top? Dunno how hard that is. Mh apparently fairly simple (one only needs to reduce the distance of the cancel button then):

position: sticky;
top: 0;
padding-top: 12px;
padding-bottom: 16px;
background-color: var(--color-neutral05);

[LukasKalbertodt]

Okay, a first code review. I have reviewed all files fully, except the big one Access.tsx. But I need a break right now, and I already collected a few comments, thus me submitting this review already. It will likely also be easier for me to review later, in particular when you pulled out the "general stuff" from Access.tsx into a new file.

[LukasKalbertodt]

Mh question: do we need this reset button? I am not sure I have ever used a reset button in my life. Like... I can just navigate away from the page. I am not sure whether normal people use these kinds of buttons.

[owi92]

I used the button quite often when testing this, but I understand that this doesn't qualify as a regular use case.
I still think it doesn't hurt having, unless you feel that it clutters the page (which I don't). We do allow pasting-in potentially long lists of usernames and people might accidently choose the wrong list, in which case a reset button is certainly convenient. It won't happen often and they can just navigate away as you said, or refresh the page, but wouldn't you rather provide a button for that? 🤷

[LukasKalbertodt]

This one should be greyed out as long as nothing has changed. (Ideally, it is also greyed out if a user changes something, then undoes that and arrives at the initial ACL again; but that's just a nice to have)

[dagraf]

I appreciate Lukas' comments here (#927 (comment)). I would like to add or make slightly different suggestions:

* I think there should be a heading, like "Access"

I agree. Opencast is using "Access policy"/"Zugriffsrechte". In the option to edit ACLs Tobira uses "Manage access"/"Zugangsbeschränkung". I would suggest to use everywhere the same wording: "Access policy"/"Zugriffsrechte"

* Maybe special case "yourself", i.e. write "You" in italic instead of "Morgan Yu"? Or "_You_ (Morgan Yu)".

I like this idea. No strong opinion on the wording. Maybe "Yourself (Name Surename)"

* I would use "Read & Write" as label/title for the second permission level instead of just "Write".

I agree.

* I would make the button like 300px instead of 200 wide, otherwise the description spans quite a few lines.

I agree. Like this the difference between the wideness of the button and the description would be smaller, too.

* I would maybe rephrase the permission level descriptions a bit. GitHub uses incomplete sentences "Can read and clone this repository". I wouldn't mind that instead of the "users with X-access are able to". Shortens it a bit. Unless you'd say that it makes it less clear. My random suggestion:
  
  * Read: "Can watch the video and view metadata"
  * Read & Write: "Can watch & edit the video. Can also view & modify metadata and other details."

I strongly agree. But I think there is no need to mention that metadata can be viewed/edited. My suggestion: Read = "Can watch the video"; Write: "Can watch & edit the video"

* Maybe the searchable selects should have the label "Select groups to add" instead of just "select groups"? Or some other formulation that implies the user can add groups that way?

I agree. My suggestion: "Add groups" and "Add users"

[LukasKalbertodt]

I agree. Opencast is using "Access policy"/"Zugriffsrechte". In the option to edit ACLs Tobira uses "Manage access"/"Zugangsbeschränkung". I would suggest to use everywhere the same wording: "Access policy"/"Zugriffsrechte"

Sounds useful!

I strongly agree. But I think there is no need to mention that metadata can be viewed/edited. My suggestion: Read = "Can watch the video"; Write: "Can watch & edit the video"

I prefer the more descriptive/detailed description. Sometimes users actually wonder about stuff like that. Your suggestion adds very little information beyond what the label "Read"/"Read & Write" already suggests.

[LukasKalbertodt]

I've finally reviewed all code. Well, I still skimmed some code in ui/Access.tsx. I find this PR a bit challenging to review, simply due to its size and amount of features. Not assigning blame, just excusing my suboptimal-reviews :D

So, apart from the inline comments, there is one main thing I've been thinking about: the state mangement. A few comments below actually deal with that as well. Note that those were written BEFORE what I'm writing here now. My understand of how it's currently done:

• The ACLs are stored in a useState(initialAcl) directly inside AclSelector. Whenever the user changes something, that is updated.
• Parent components can get access to the current ACLs by using a custom ref. That's what the upload component does. It retrieves the ACL when the submit button is pressed.
• But that custom ref is only useful for handler functions, not for the main rendering function, as nothing is rerendered when stuff changes. But you needed that for disabling the buttons in the "manage video access" route. So there you allowed passing children to AclSelector, which you do with those buttons. They then can get access to some ACL selector context, giving them access to the current selected ACL.

Especially the last part I would not change. This feels backwards and hacky IMO.

As noted by one comment below, react hook form would probably like to be notified about changed data every time it is changed, not only on submit. So as I suggested there, I would add an onChange event handler to AclSelector. That would mean that parent components of AclSelector have a duplicate state mangement: the upload component has it through react-hook-form, and the "manage" components would have a useState. That way we should completely get rid of the useImperativeHandle ref.

That will work and is better than the current situation IMO. However, then we have the ACL state twice, which is not ideal. So one could also think about getting rid of all useState calls inside AclSelector that store ACLs. We would rename the intialAcl to just acl and the component would then always just render the ACLs that were given. By calling onChange, the parent component's state is changed, which means the acl prop will change, which means AclSelector will be re-rendered. I'm not yet sure if I prefer that, but it should certainly be possible.

[LukasKalbertodt]

We might want to rename to isStrictSubset, as mathematically a set is its own subset, but not its own strict subset. This is important to ensure the "reflexive" property of the sort comparator below.

[LukasKalbertodt]

This does not satisfy the "transitive" property that's required (see docs):

More formally, the comparator is expected to have the following properties, in order to ensure proper sort behavior:

[...]

• Transitive: If compareFn(a, b) and compareFn(b, c) are both positive, zero, or negative, then compareFn(a, c) has the same positivity as the previous two.

Assuming that isSubset implements a correct strict set subset check, then the following inputs fail transitivity:

• a: { 🐑 🐭 }
• b: { 🐔 }
• c: { 🐑 }

compareFn(a, b) and compareFn(b, c) both return 0 as there is no subset relationship. But compareFn(a, c) returns non-0 as there is a subset.

In that case, the behavior of sort is "implementation defined": https://stackoverflow.com/a/40902509/2408867
Which is not great as we in theory cannot expect any reasonable output. In fact, we cannot even expect that subsets appear after their supersets. That said, the worst that could happen is a weird sort order of the entries, which is not a catastrophe. And I strongly suspect that the result will always be roughly what we expect. So maybe we can just keep it as is?

How could we even fix it? I don't think we can't use sort. Sort simply requires a total ordering, meaning that any two elements must be comparable. But in our case, { 🐑 🐭 } and { 🐔 } cannot be compared! Your code returns 0 (meaning: equal) here, but that's not true of course. We simply cannot establish an ordering between the two elements.

So I think we need to use a topological sort, as that's the right tool for the job. Whether to implement that ourselves (I would :D) or use a library is up to you. Though we shouldn't pull in a super heavy general graph processing library or something like that.

[oas777]

Not sure whether this is the time or the place to mention this, but "user and password" are an authentication we need at ETH... I presume this would also be dealt with under "Access policy"?

[LukasKalbertodt]

I found a few more things unfortunately. But we already talked about me taking it from here basically. So I would suggest we do it that way. Then I will fix these last things, we get the PR merged, and then I will deal with the configurable groups in another PR.

[LukasKalbertodt]

Why is that necessary 🤔 I feel like we shouldn't need an effect here.

I think:

• The prop initialAcl should be renamed somehow (acl, but that clashes with something else...). Because it's not initial. It's always the current ACL that's displayed.
• Further, I think you can completely remove the two useState<MultiValue<...>> calls and just rename initialSelections to selections and same with initialOptions. Again, those are directly derived from the acls coming in. And whenever those change, they are changed as well.
• And then you should be able to remove this useEffect
• And then you can also remove all setSelection and setOptions calls from the handlers below. That should just work I think.

[owi92]

Further, I think you can completely remove the two useState<MultiValue<...>>

I was under the impression that those are needed for react select, but I guess if you say it can be done without them, then it can be done without them.