LibrePhotos / librephotos

A self-hosted open source photo management service. This is the repository of the backend.
MIT License
6.87k stars 298 forks source link

WIP: Allow users to upload media to shared albums #1256

Open the01 opened 4 months ago

the01 commented 4 months ago

Basic working implementation for #911

@derneuere could you please take a look? As a first step, I took the simplest approach to get this feature working without changing the FE, so probably a few adjustments are needed.

I added a SHARED_ALBUM_ALL_ADD setting with an env variable of the same name, so that admins can decide if the basic functionality works for them or they want to wait until the edge cases are also covered.

Do we need to filter here again even though we already did it here?

sonarcloud[bot] commented 4 months ago

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

derneuere commented 4 months ago

The code looks good :) Looking forward to testing it!

You can only add images to a shared album, if you explicitly shared it with a user. I believe we could simplify things further by removing the setting for admin access in this scenario. I don't think, that we need the setting for admin in this case. However, I'm definitely open to discussion on this. If anyone there is a compelling reason to keep the admin setting, I'm all ears!

In general: Let's only keep deployment specific as an environment variable and the rest as a constance variable, which you can change within the program: https://github.com/LibrePhotos/librephotos/blob/3eeefbbc574abfde3cb7082b21bee4acc8917c6c/librephotos/settings/production.py#L128

the01 commented 4 months ago

I am not yet familiar enough with the code base to predict all possible side effects, but one scenario I could think of would be (accidental) deletion of pictures from the album by a sharee (I don't think its an actual word, but the ones the album was shared with). That is why I wanted to include an option to turn the feature off, at least globally.

I saw constance, but I assumed this is stored in the database and would require a migration. For the first test I wanted to limit the permanent/non-revertable changes, hence the env variable. Long term it would be better to have this configurable per album as an additional field on AlbumUser or maybe even per Album&User specified on the relationship table. But I think it would be better to start simple like this, gather some experience and then extend it to more fine-grained control later. (In part since this will require changes to the FE, which is not my strong suit 😆 )

derneuere commented 4 months ago

Yes, that sounds like a viable approach :) 👍