Closed ildyria closed 8 months ago
I have a few restraints with regard to such change. Don't get me wrong, I think it is a way forward and Lychee definitely need improvements in that regards. However I fear that this approach would only work if you have only few users. If you have a significant number of user then the sharing table actually becomes unusable, so I am not in favor of such radical GUI change.
I do not use the password functionality so I don't really give a F about it, I tend prefer sharing hidden albums rather than password protected ones.
However I do agree with @kamil4 with regard to password protections. You can also consider the following use case as a wedding photographer: you do not want to have to create a user for each and every client you have, lock their account, assign them access rights to specified album. This is just too many clicks for what could be solved in a single dialog box.
However, I do not think creating a null
user is a good idea in such table, reason being that it breaks the foreign key constraints.
In the case of a public user the following attributes:
Attribute | Useful | misc |
---|---|---|
is_public |
✔️ | |
requires_link |
✔️ | |
password |
✔️ | |
is_share_button_visible |
💭 | Why do we need that button as the user can just copy the URL... |
grants_full_photo |
✔️ | |
is_downloadable |
✔️ | |
can_upload |
❌ | |
can_move |
❌ | |
can_delete |
❌ | |
can_admin |
❌ |
In the case of logged in user: | Attribute | Useful | misc |
---|---|---|---|
is_public |
❌ | ||
requires_link |
❌ | ||
password |
❌ | ||
is_share_button_visible |
💭 | Why do we need that button as the user can just copy the URL... | |
grants_full_photo |
✔️ | ||
is_downloadable |
✔️ | ||
can_upload |
✔️ | Logged user can upload pictures to said album (also moveTo as a target) | |
can_move |
✔️ | Logged user can upload pictures from said album. | |
can_delete |
✔️ | Logged user can delete pictures from said album. | |
can_admin |
✔️ | Logged user can modify properties from said album. |
Not that is_uploadable
does not mean anything for albums. I recently got complaints that there was preferences about wording such as "unlocked" vs. "isUnlocked"... In this specific case, I would be strongly in favor of can_upload
and can_delete
where the access right is considered from the User rather than the properties of the album.
I assume this would also simplify the SQL queries, because the conditions become more consistent.
To me adding more tables where the results need to be merged per album per user is not making things "simple" in SQL. A logged in user should still be allowed to see public albums. I fail to see how adding such tables will make things easier.
Furthermore, if we extend on access rights, I would be more in favor for an hybrid approach:
is_share_button_visible
is_public
, requires_link
, password
, grants_full_photo
, is_downloadable
stays on the album.Create a new table user_base_album_rights
Which is the point of view of what the user can do rather than album has as options:
id
,album_id
,user_id
,can_download
,can_upload
,can_move
,can_delete
,can_admin
,has_full_photo
.has_full_photo
and can_download
obviously have precedence on the album properties.
Does this mean users lose the right to edit the photos they have just uploaded, because ownership gets transferred? If there is a general is_editable attribute, does this mean that user are also allowed to delete photos? These are all very interesting questions and I assume that we have to give up the concept of single-ownership albums in the long run.
It can be seen in two ways:
while it makes things a bit more complex, I would be in favor of the later.
For the moment, I would like to keep anything which has to do something with write access out of the discussion. I believe we all agree that this needs extra attention and a lot of detail questions needs to be solved. So in a first step I would like to concentrate on read access, i.e. what the "protection policy" dialog and the "share" dialog already allow today. We should take one step after the other.
Re @kamil4:
@nagmat84 wrote: Actually, I would like to drop the password-support completely. [...] one can always create a "dummy" user, lock the user such that no one can change its password and then share those user's credentials with the people. [...]
@kamil4 replied:
But from users' point of view, that alternative is pretty clunky. With a password-protected album, when my parents in their seventies want to see the pictures of the grandkids, they just click on a single URL to the password-protected album and when they do, a dialog box pops up asking for a password and as soon as they do, pictures show up. With multi-user, that's currently not the case. The URL needs to be to the top-level gallery, where they first need to click on the little arrow in the top-left, provide a login and password, then click on the album that shows up. It's a lot more... friction. No wonder we had this very complaint from a user recently -- that they would like an option to at least go straight to the login/password dialog box.
Totally agreed when in comes to UX. The current situation is awful. But this has not to be this way. IMHO, it should be possible to login with username and password from any page and the frontend should automatically show a login dialog if it gets a 401 response.
I remember to have already proposed that somewhere else. I guess the current difference in UX which we currently see is due to the unfortunate history that error messages in the past used to only differentiate between "API not found" and "server error". But now, the client gets proper, parseable JSON error objects. Hence, I imagine the following: A user goes to a URL, the backend throws a 401 error, and the frontend shows the login dialog (for username+password).
This can already be implemented now and is an issue of the front-end only. This should actually be an own PR and completely independent of the matter we discuss here, because it has a great benefit for UX on its own.
Re @ildyria
However I do agree with @kamil4 with regard to password protections. You can also consider the following use case as a wedding photographer: you do not want to have to create a user for each and every client you have, lock their account, assign them access rights to specified album. This is just too many clicks for what could be solved in a single dialog box.
That was not what I meant. I was thinking of a single dummy user account which mimics the behavior of a shared password except that the user account also has a username. I am not suggesting to create one user account for each person. If one would do so, then there would also be no need to lock the account, because the account would only be used by a single person anyway.
However, I do not think creating a null user is a good idea in such table, reason being that it breaks the foreign key constraints.
Eh? No. That is exactly makes this solution so appealing. Obviously, you don't want to have a "technical dummy" user in the users
table which fictitiously represents the anonymous user. Let alone, that none of the entries in the users
table makes sense for the anonymous user. Hence, you define a foreign constraint on the album_user
table from album_user.user_id
to users.id
. album_user.user_id
is a nullable column, users.id
is not nullable, because every regular user must have an ID. For every non-null value in album_user.user_id
the foreign constraint is enforced by the DBMS and hence the ID must exist in users.id
. However, for any pair _(albumid, null) in album_user
the foreign constraint is not enforced, because null
values are exempt from foreign key constraints. This is a good thing because this allows to add _(albumid, null) to album_user
, without having a null
entry in users
.
However I fear that this approach would only work if you have only few users. If you have a significant number of user then the sharing table actually becomes unusable, so I am not in favor of such radical GUI change.
I don't take that argument, because this essentially implies that user-object ACL can never be implemented. But this is the way, how any modern file system architecture or any other multi-user system works nowadays. And this is what users expect.
Of course there are always ways how to implement such a feature in unusable ways. For example, lets take our unfortunate dialog for the album tree as an anti-role model. This dialog presents the whole album tree at once in an expanded representation without any chance to collapse sub-trees or search for specific albums by key input. Obviously, this is unusable, if there are thousands of albums. This is the way how things should not be done. Similarly, one could present an seemingly unsorted list of all users at once (sorted by some accidental DB internal key). This will surely be unusable. But again this has not to be this way. I think such a dialog would be fine, if it only shows the users with whom an album is actually shared. If one wants to add a user one has to pick this user from an extra list which must be searchable be key input.
If you have a significant number of user then the sharing table actually becomes unusable.
I agree with @nagmat84 here. It should be perfectly fine if it starts with just one row (the "public" row); more rows are added explicitly when the owner wants to share an album with those users. On the other hand, I do like how the current dialog provides one-line clarifications underneath each option; this is something harder to duplicate in a table layout as seen with "Users", where we keep getting asked what is a "Restricted account", for instance.
You can also consider the following use case as a wedding photographer: you do not want to have to create a user for each and every client you have, lock their account, assign them access rights to specified album. This is just too many clicks for what could be solved in a single dialog box.
That's an interesting perspective in the sense that it's opposite (but complementary) to what I meant. I complained about the burden on the viewer; you on the burden on the uploader. I'm honestly not convinced by the argument about the uploader, as uploading is a one-off operation (so additional overheads are easier to accept), whereas viewing will often repeat many times. Why wouldn't I want to create an account for every client? I think that's exactly what I would want to do! But sure, it's more clicks than either hiding the album or password-protecting it.
is_share_button_visible: Why do we need that button as the user can just copy the URL...
??? That button has been there since forever, and has only just been expanded with the QR code. Now you want to remove it? What I personally don't understand is how this became a per-album config in the first place. I can see how not everybody may be interested in that button, but wouldn't a global config have been sufficient in that case? I see that it was merged in #403. The server side looks OK, but the front end is definitely acting up, hiding the button even for the album owner, which is something we never do with other related options such as is_downloadable
. I would call that part of the implementation broken.
has_full_photo
andcan_download
obviously have precedence on the album properties.
But why have this duplication between album properties and user_base_album_rights
at all though? It will be confusing and error-prone.
IMHO, it should be possible to login with username and password from any page and the frontend should automatically show a login dialog if it gets a 401 response. This can already be implemented now and is an issue of the front-end only. This should actually be an own PR and completely independent of the matter we discuss here, because it has a great benefit for UX on its own.
Agreed. The 401 case should be relatively easy. There is a practical difficulty of allowing to log in from any page, in that "since forever" the space where the login button is located in the top-level albums view is instead in the album view occupied by the back button. Any suggestions how to solve it nicely without breaking everybody's finger memory in the process? I think the suggestion we've had of automatically presenting a login dialog if there are no public albums to display is also a good one.
On the other hand, I do like how the current dialog provides one-line clarifications underneath each option; this is something harder to duplicate in a table layout as seen with "Users", where we keep getting asked what is a "Restricted account", for instance.
Agreed.
Now you want to remove it?
Noooo! I am wondering if the option whether to show it or not is necessary. :)
There is a practical difficulty of allowing to log in from any page, in that "since forever" the space where the login button is located in the top-level albums view is instead in the album view occupied by the back button. Any suggestions how to solve it nicely without breaking everybody's finger memory in the process? I think the suggestion we've had of automatically presenting a login dialog if there are no public albums to display is also a good one.
You can actually hit the L
key anywhere and it will make the login dialog pop. :)
Okay, I think I am starting to see the way @nagmat84 wants to implement it.
However I don't really like that we have a table user_base_album
without a model attached to it and would be more in favor to add a model AlbumAccessRights
or something of similar naming and make it a base_album 1 ->* album_access_right
relationship.
The model AlbumAccessRights
having the following attributes:
Column | Guest | User | Comment |
---|---|---|---|
id |
✔️ | ✔️ | id the the row obviously |
album_id |
✔️ | ✔️ | id of the base album |
user_id |
❌ | ✔️ | nullable - link towards user |
requires_link |
✔️ | ❌ | Only used in the guest user case |
password |
✔️ | ❌ | Only used in the guest user case |
is_share_button_visible |
💭 | 💭 | Why do we need that button as the user can just copy the URL... |
grants_full_photo |
✔️ | ✔️ | Access to full size |
is_downloadable |
✔️ | ✔️ | While for a Photo it is not really useful, for albums there is the rationale that you do not necessarily want to allow your users to download all the pictures in one go. |
✔️ & ❌ representing the usefulness of the attribute.
I also can see how this can later be extended to support write access etc.
@nagmat84 can you confirm this is what you have in mind or are we not on the same wavelength yet ?
You can actually hit the
L
key anywhere and it will make the login dialog pop. :)
:exploding_head:
Is that something you added as part of U2F support? Because it's not documented at https://lycheeorg.github.io/docs/keyboard.html, and I know that back when I was updating that page, I went through the source code to make sure that all the shortcuts were covered...
Anyway, having slept on it, I think we should add the log-in button to the right of the "back" button. That way it will be there without shifting existing buttons.
Is that something you added as part of U2F support? Because it's not documented at
Don't remember, that's too old. I was just lazy to put my mouse in the top left corner ... :'D
Okay, I think I am starting to see the way @nagmat84 wants to implement it. However I don't really like that we have a table
user_base_album
without a model attached to it and would be more in favor to add a modelAlbumAccessRights
or something of similar naming and make it abase_album 1 ->* album_access_right
relationship.
Well we already have the table user_base_album
today without a model attached to it, because it implements the (m:n)-relationship between shared base albums and users. This is nothing new. Adding more attributes to it is also fine. There only two different implementation approaches you can take.
One can either consider the table user_base_album
to model an attributed (m:n)-relationship. In theory, Eloquent supports that. But I don't now how much PITA an attributed (m:n)-relationship will be. In UML this gives the following picture:
+------+ +-----------+
| User | (0..*)-----+---- (0..*) | BaseAlbum |
+------+ | +-----------+
|
+------------------------------+
| # requires_direct_link: bool |
| # grants_full_photo: bool |
| ... |
+------------------------------+
Or one can break up the (n:m)-relationship into two (1:n)-relationships like this
+------+ +------------------------------+ +-----------+
| User | (0..1)-----(0..*) | Share | (0..*)-----(0..*) | BaseAlbum |
+------+ +------------------------------+ +-----------+
| # requires_direct_link: bool |
| # grants_full_photo: bool |
| ... |
+------------------------------+
On the SQL level the different approaches will even result into isomorphic table structures. I probably would have gone for the latter approach anyway, because I fear that Laravel/Eloquent will be a PITA if we tried the first approach. Moreover, the second approach also gives @ildyria his model ;-)
The model
AlbumAccessRights
having the following attributes: Column Guest User Commentid
heavy_check_mark heavy_check_mark id the the row obviouslyalbum_id
heavy_check_mark heavy_check_mark id of thebase album
user_id
x heavy_check_mark nullable - link towards userrequires_link
heavy_check_mark x Only used in the guest user casepassword
heavy_check_mark x Only used in the guest user caseis_share_button_visible
thought_balloon thought_balloon Why do we need that button as the user can just copy the URL...grants_full_photo
heavy_check_mark heavy_check_mark Access to full sizeis_downloadable
heavy_check_mark heavy_check_mark While for a Photo it is not really useful, for albums there is the rationale that you do not necessarily want to allow your users to download all the pictures in one go.heavy_check_mark & x representing the usefulness of the attribute.
I am not 100% sure how I am supposed to interpret that table. Of course the table can only have one set of attributes for both the guest and authenticated users. That is why I would keep the password
column in base_album
. It does not make much sense for any other user and hence it should not be repeated for every user even if it is null
"presque partout" for efficiency reasons.
I also agree on is_share_button_visible
. Maybe we should really convert this into a global configuration option.
I also can see how this can later be extended to support write access etc.
@nagmat84 can you confirm this is what you have in mind or are we not on the same wavelength yet ?
Yes
@ildyria and @kamil4 I would like to pull out the discussion about the login dialog intro an extra issue. As I haven't found my old suggestion, I created a new enhancement issue https://github.com/LycheeOrg/Lychee-front/issues/312. I guess I would start with working on that anyway, because it is rather independent of the matter discussed here and a worthy PR on its own.
Just a little heads-up. This is the direction into which the dialog is developing:
At the moment it is only a hard-coded frontend UI. I took care of tooltips, such that users have a chance to understand what the symbol means and the drop-down menu at the bottom allows to add new users.
While I tried to code this UI, I remembered that I have one open PR for the modal dialog https://github.com/LycheeOrg/basicModal/pull/3 and I a demo branch https://github.com/LycheeOrg/basicModal/tree/lychee_demo_revised which shows how the dialogs are going to look. In theory they should still look the same, but as some of the margin/paddings, etc. have been unified to make it a lot easier to code dialogs, the one or other dialog looks slightly different.
Back-end wise, I am convinced. Front-end wise, I disagree with merging the two : guest user / share user.
My rationale is are follows:
I am not opposed to changes, I am just trying to keep it simple and easily usable/accessible for the user.
That being said, I do like the presentation of the upper half part, I just think it must not be merged with the guest user side. That is why I am more in favor of a sharing modal or a double tabs: one for guest, one for sharing with users.
FYI with livewire, I do plan to remove some of the multiple modals that we have. That one is one that will for sure disappear. :)
My rationale is are follows:
- it is a break in UI compared to what we currently have (not that it is a bad thing per say) and thus may confuse users: this complicates the simple sharing window with guest users.
Au contraire. It makes matter much clearer. In particular, it is much more obvious which option affects what in which way. At the moment we have two dialogs, which options that "somehow" interact with each other in sometimes intuitive ways. Just see the discussion in https://github.com/LycheeOrg/Lychee/issues/1478.
If we keep the things separate, then the share dialog will always be the third wheel, because it will always stay the unbeloved child.
- I would believe most users of Lychee do not use the user functionalities (I may be wrong).
It is extremely obvious, that user asks for such features. Of course, user won't use them, if you keep them hidden in some obscure dialog and if you make them unusable. But that is a self-fulfilling prophecy: If a feature is not found in the first place or unintuitive/unclear to use, then user won't use it. But taking that as an argument to not improve it, will keep it in that state.
- I would rather keep those separate in order to have more flexibility later when we implement write access for example.
We can simply add more columns at the right. At the moment there are only two columns (for download and full photo), there can be more.
- just because we merge them in the back-end does not means that we need to merge them in the front-end.
Correct, the frontend design should not be governed by backend decisions on the relational DB model. But in the specific case at hand, the reason of merging it, is another one and is the common root cause for changing both the frontend and backend: It is simply the right way to think about it. Keeping the sharing dialog for anonymous users and for authenticated users separately, introduces an artificial boundary between both. This will always lead to problems of the kind that some feature is blindfoldly implemented for the one aspect without taking the other aspect into consideration.
- adding more users to the shared list changes the size of the window (and thus lower the guest accessibility setting and may induce a scroll).
Nope. At the moment I intent to keep the entire dialog at a fix height and reserve, four/five rows for the body of the table. If more users are added to the table, the body but only the body of the table becomes scrollable.
I am not opposed to changes, I am just trying to keep it simple and easily usable/accessible for the user.
Agreed, but easy access in this case means that user can in particular find it and gain an understanding on how the various options work together. This isn't achieved by splitting things apart which logically depend on each other.
I actually agree with both of you. Or, to make my position more clear, I think @ildyria's concerns are valid, but that they can (and probably should) be addressed within a single dialog, because I also agree with @nagmat84 that having all visibility settings in one place is a plus, though only if the dialog is easy to use.
The above screenshot is a little unfortunate because it doesn't show how sharing with anonymous users is to be achieved. Is there still a big "Public" slider? If not, what replaces it? Do we instead need to add some pseudo-user via the GUI? If so, I can see how that would make it more logically consistent and better optimized in terms of screen real estate usage, but perhaps it is better to keep the current style for selecting options for the anonymous users and only use the table for sharing with regular users?
Either way, I think that the options that apply to anonymous users should be the most prominent ones because I agree with @ildyria that they are used the most. So they should be on top (so that if the dialog box becomes too tall and gets a scrollbar, they are still visible by default) or, if one went with tabs instead, they should be in the tab that is displayed when the dialog box opens.
The above screenshot is a little unfortunate because it doesn't show how sharing with anonymous users is to be achieved. Is there still a big "Public" slider? If not, what replaces it? Do we instead need to add some pseudo-user via the GUI? If so, I can see how that would make it more logically consistent and better optimized in terms of screen real estate usage,
Exactly. The anonymous user is part the first entry in the drop-down list and the top entry of the table, if added. Sorry, that I picked an unfortunate screenshot.
The idea is that the options which are meaningful for the anonymous user and a known user are presented in the same table. Thats also why the special options for the anonymous user are reduced to two.
And I am still unsure if the the option whether a direct link is required should be an option for the anonymous user only (as it is now) or be an option for each sharing.
Either way, I think that the options that apply to anonymous users should be the most prominent ones [...]. So they should be on top
That's not possible with the approach talen here. Because to make the additional, special options at the bottom useful (and not greyed out) the use has to select the anonymous user in the table above first.
The slider for "is public" which you are missing is essentially replaced by the fact whether the anonymous user is part of the sharing table or not.
However, logically this means that the table must come top, because users have to select the anonymous user first.
(so that if the dialog box becomes too tall and gets a scrollbar, they are still visible by default)
That's why the table itself is made to have a fixed height and become scrollable, if necessary.
I will post new screenshots soon, then it will become more clearer.
That's not possible with the approach taken here. Because to make the additional, special options at the bottom useful (and not greyed out) the use has to select the anonymous user in the table above first.
However, logically this means that the table must come top, because users have to select the anonymous user first.
I strongly disagree. I believe that there should not be a fusion of anonymous user and normal user. This is the case in the background but I believe the user do not need to be aware of this.
One of the reason is that the meaning of the tick box under the table and the disabling of them is somewhat lost because no longer "attached" to the guest user. One proof is that you had to clarify it: "These options only applies if the album is is shared with anonymous users."
The user of Lychee should (I would make it a must) not need use a drop down menu to make an album public.
But in the specific case at hand, the reason of merging it, is another one and is the common root cause for changing both the frontend and backend: It is simply the right way to think about it. Keeping the sharing dialog for anonymous users and for authenticated users separately, introduces an artificial boundary between both.
I agree, but we MUST lower the cognitive burden of the user to use the interface. Adding this virtual barrier simplifies the thoughts process when not using User. I really don't think that merging the User functionality with public sharing is the good way to go. Yes, once you understand how it works it obviously makes sense, but there is an entry barrier which I am not comfortable to give to the user.
This will always lead to problems of the kind that some feature is blindfoldly implemented for the one aspect without taking the other aspect into consideration.
The back-end somewhat makes sure that it does not happen. :|
And I am still unsure if the the option whether a direct link is required should be an option for the anonymous user only (as it is now) or be an option for each sharing.
I don't think the direct link required is meaningful in the case of user sharing. :)
The reason why I am strongly against this merge is because:
Here is what I would be quite happier with. It keeps the Public section obvious while also making the sharing part more prominent and merge the 2 in on modal.
I also added fictive option such as Upload, Delete, Manage, etc. and the inheritance parameter for easier propagation to give an idea of the direction I was thinking of.
┌──────────────────────────────────────────────────────┐
│ │
│ ┌───┐ │
│ │o │ :Inherit from parent │
│ └───┘ │
│------------------------------------------------------│
│ ┌───┐ │
│ │o │ :Public: │
│ └───┘ │
│ [ ] Original │
│ [ ] Hidden │
│ [ ] Downloadable │
│ [ ] Password Protected │
│ Password: [____________________] │
│------------------------------------------------------│
│ │
│ Share with │
│ │
│ Orig Dwnld Upld Del Manag │
│ User 1 [ ] [ ] [ ] [ ] [ ] [Del] │
│ User 2 [ ] [ ] [ ] [ ] [ ] [Del] │
│ [User 3 v] [ ] [ ] [ ] [ ] [ ] [Add] │
│ │
│ │
│------------------------------------------------------│
│ ┌───┐ │
│ │o │ :Sensitive: │
│ └───┘ │
│------------------------------------------------------│
│ │
│ ┌───────────────┐ [ +Save & Refresh v] │
│ │ Cancel │ [ +Save Only ] │
│ └───────────────┘ [ +Save & Propagate ] │
│ │
└──────────────────────────────────────────────────────┘
@nagmat84 What you are saying is logically correct but I share @ildyria's concerns about the UX. In my view, a successful interface should "keep simple things simple". Making an album public is one of the most common operations in Lychee, right after creating an album and uploading pictures. Folding that functionality into the user sharing UI, while logical on the face of it, feels like a step backwards in terms of ease-of-use. And we can't even fold it completely, because there are options that only apply to the anonymous users (requires_link, password) and presumably there will in the future be options that only apply to individual users (editing). These prominent exceptions make me think that we should keep them separate.
Regarding the requires_link
for albums shared with users, I think that the same argument applies as you used to convince me about the password protection. The user is already authenticated and the album owner made an explicit decision to share the album with them -- there's no need for additional obfuscation/verification.
@ildyria's outline makes me concerned about the size of the resulting dialog box though. The current visibility settings box doesn't fit vertically on my 13" laptop screen; I'm honestly beginning to wonder if the new one will fit on my 27" external screen?
Let's move out of the PR and discuss this here. As follows is the log of previous discussions.
TL;DR: Proposition by @nagmat84
grants_full_photo
,requires_link
,is_downloadable
,is_share_button_visible
frombase_albums
touser_base_album
null
@nagmat84
@kamil4
@nagmat84
@kamil4