Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix downloading shared photos from shared album #344

Merged
merged 8 commits into from
Aug 11, 2024

Conversation

alexismarquis
Copy link
Contributor

@alexismarquis alexismarquis commented Jul 13, 2024

Breaking Change
This will extend the classes SynoPhotosAlbum and SynoPhotosItem by an editional property called passphrase which should set to None if the item or album is not shared by others or set to the corresponding passphrase when it is shared.


Context :
Shared album have a passphrase field that is used in the public sharing link. It is also required by the download and thumbnail api methods.
See home-assistant/core#120817 for more detail.

This PR adds passphrase handling, fixing downloading photos from different users in shared albums.
Additionally, it addresses a problem I encountered with HEIC photos that return an application/octet-stream content type.

Fixes #299

@alexismarquis alexismarquis requested a review from mib1185 as a code owner July 13, 2024 14:19
@alexismarquis
Copy link
Contributor Author

@mib1185 could you take a look at this ?

Copy link
Owner

@mib1185 mib1185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexismarquis

first thanks for this contribution 👍
There are some formatting issues (see failing CI) which needs to be solved first (easiest way is to use the VSCode dev container). Further there are some typing issues (mypy issues on the CI) which should be solved by the comments below.

From technical point of view, i cannot really verify it, since i do not use the photos station in such way - maybe @lodesmets (sorry for pinging you again 🙈) could also have a look?

As soon as you've done all changes, please press the "Ready for review" button, thx 👍

src/synology_dsm/api/photos/model.py Outdated Show resolved Hide resolved
src/synology_dsm/api/photos/model.py Outdated Show resolved Hide resolved
src/synology_dsm/api/photos/model.py Show resolved Hide resolved
@mib1185 mib1185 marked this pull request as draft August 9, 2024 19:23
@alexismarquis alexismarquis marked this pull request as ready for review August 10, 2024 18:18
@alexismarquis
Copy link
Contributor Author

Hi, thanks for the feedback, requested changes has been done.

@lodesmets
Copy link
Contributor

Hey, @alexismarquis, @mib1185

Looks good. I tested it, And it still seems to work.
I also looked over the code, and it all seems great

Copy link
Owner

@mib1185 mib1185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexismarquis
again many thanks, we are close to be ready for merge, just some small comments.

@lodesmets thanks for your testing and review efforts 👍

src/synology_dsm/api/photos/__init__.py Show resolved Hide resolved
src/synology_dsm/api/photos/__init__.py Outdated Show resolved Hide resolved
src/synology_dsm/api/photos/__init__.py Outdated Show resolved Hide resolved
src/synology_dsm/api/photos/__init__.py Show resolved Hide resolved
src/synology_dsm/api/photos/__init__.py Outdated Show resolved Hide resolved
src/synology_dsm/api/photos/__init__.py Outdated Show resolved Hide resolved
src/synology_dsm/api/photos/__init__.py Outdated Show resolved Hide resolved
Copy link
Owner

@mib1185 mib1185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @alexismarquis 👍

@mib1185 mib1185 merged commit 1d83979 into mib1185:master Aug 11, 2024
6 checks passed
@mib1185 mib1185 added the breaking Breaking Change label Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to download photo in shared album, if uploaded by other person
5 participants