Skip to content

Commit c4fae73

Browse files
authored
Merge pull request #1851 from dotanm/backend/download-select-all
2 parents a2fe60e + 25d049b commit c4fae73

5 files changed

Lines changed: 252 additions & 45 deletions

File tree

apps/backend/api/tests/test_zip_list_photos_view_v2.py

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,148 @@ def test_download(self, patched_shutil):
3434
patched_shutil.return_value.free = 0
3535
response_3 = self.client.post("/api/photos/download", data=datadict)
3636
self.assertEqual(response_3.status_code, 507)
37+
38+
39+
class ZipListPhotosV2SelectAllTest(TestCase):
40+
"""The download endpoint also accepts the select_all + query payload
41+
shape that the other bulk mutations (favorite/hide/public/delete)
42+
already support, so that the frontend can offer Download when the
43+
user has done a server-side "Select All" without enumerating every
44+
image hash. The async zip task isn't exercised in these tests; we
45+
inspect the photo set handed to create_download_job instead."""
46+
47+
def setUp(self):
48+
self.client = APIClient()
49+
self.user = create_test_user()
50+
self.other_user = create_test_user()
51+
self.client.force_authenticate(user=self.user)
52+
53+
disk_patcher = patch("shutil.disk_usage")
54+
patched_disk = disk_patcher.start()
55+
patched_disk.return_value.free = 500000000
56+
self.addCleanup(disk_patcher.stop)
57+
58+
# create_download_job enqueues a django-q AsyncTask; stub it out and
59+
# capture the photos that would have been zipped instead.
60+
job_patcher = patch("api.views.views.create_download_job")
61+
self.mock_create_job = job_patcher.start()
62+
self.mock_create_job.return_value = "fake-job-id"
63+
self.addCleanup(job_patcher.stop)
64+
65+
def _queued_hashes(self):
66+
self.assertTrue(
67+
self.mock_create_job.called,
68+
msg="create_download_job was never reached — the view returned early",
69+
)
70+
photos = self.mock_create_job.call_args.kwargs["photos"]
71+
return {photo.image_hash for photo in photos}
72+
73+
def test_select_all_with_empty_query_includes_all_user_photos(self):
74+
photos = create_test_photos(number_of_photos=3, owner=self.user, size=100)
75+
76+
response = self.client.post(
77+
"/api/photos/download",
78+
data={"select_all": True, "query": {}},
79+
format="json",
80+
)
81+
82+
self.assertEqual(response.status_code, 200)
83+
self.assertEqual(self._queued_hashes(), {p.image_hash for p in photos})
84+
85+
def test_select_all_only_includes_requesting_users_photos(self):
86+
mine = create_test_photos(number_of_photos=2, owner=self.user, size=100)
87+
create_test_photos(number_of_photos=2, owner=self.other_user, size=100)
88+
89+
response = self.client.post(
90+
"/api/photos/download",
91+
data={"select_all": True, "query": {}},
92+
format="json",
93+
)
94+
95+
self.assertEqual(response.status_code, 200)
96+
self.assertEqual(self._queued_hashes(), {p.image_hash for p in mine})
97+
98+
def test_select_all_respects_excluded_hashes(self):
99+
photos = create_test_photos(number_of_photos=4, owner=self.user, size=100)
100+
excluded = [photos[0].image_hash, photos[1].image_hash]
101+
102+
response = self.client.post(
103+
"/api/photos/download",
104+
data={
105+
"select_all": True,
106+
"query": {},
107+
"excluded_hashes": excluded,
108+
},
109+
format="json",
110+
)
111+
112+
self.assertEqual(response.status_code, 200)
113+
self.assertEqual(self._queued_hashes(), {p.image_hash for p in photos[2:]})
114+
115+
def test_select_all_applies_query_filters(self):
116+
videos = create_test_photos(
117+
number_of_photos=2, owner=self.user, size=100, video=True
118+
)
119+
create_test_photos(number_of_photos=3, owner=self.user, size=100, video=False)
120+
121+
response = self.client.post(
122+
"/api/photos/download",
123+
data={"select_all": True, "query": {"video": True}},
124+
format="json",
125+
)
126+
127+
self.assertEqual(response.status_code, 200)
128+
self.assertEqual(self._queued_hashes(), {p.image_hash for p in videos})
129+
130+
def test_select_all_with_no_matching_photos_returns_404(self):
131+
# Two videos in the library, but the user's filter says photos-only.
132+
create_test_photos(number_of_photos=2, owner=self.user, size=100, video=True)
133+
134+
response = self.client.post(
135+
"/api/photos/download",
136+
data={"select_all": True, "query": {"photo": True}},
137+
format="json",
138+
)
139+
140+
self.assertEqual(response.status_code, 404)
141+
self.assertFalse(self.mock_create_job.called)
142+
143+
def test_select_all_with_everything_excluded_returns_404(self):
144+
photos = create_test_photos(number_of_photos=2, owner=self.user, size=100)
145+
146+
response = self.client.post(
147+
"/api/photos/download",
148+
data={
149+
"select_all": True,
150+
"query": {},
151+
"excluded_hashes": [p.image_hash for p in photos],
152+
},
153+
format="json",
154+
)
155+
156+
self.assertEqual(response.status_code, 404)
157+
self.assertFalse(self.mock_create_job.called)
158+
159+
def test_missing_image_hashes_still_rejected_when_select_all_falsy(self):
160+
# Without select_all the original contract is preserved.
161+
response = self.client.post(
162+
"/api/photos/download", data={"select_all": False}, format="json"
163+
)
164+
self.assertEqual(response.status_code, 400)
165+
self.assertFalse(self.mock_create_job.called)
166+
167+
def test_storage_check_runs_in_select_all_mode(self):
168+
# If free storage is smaller than the aggregated photo size, the
169+
# endpoint returns 507 regardless of which payload shape was used.
170+
create_test_photos(number_of_photos=2, owner=self.user, size=10**9)
171+
172+
with patch("shutil.disk_usage") as patched_disk:
173+
patched_disk.return_value.free = 0
174+
response = self.client.post(
175+
"/api/photos/download",
176+
data={"select_all": True, "query": {}},
177+
format="json",
178+
)
179+
180+
self.assertEqual(response.status_code, 507)
181+
self.assertFalse(self.mock_create_job.called)

apps/backend/api/views/views.py

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,20 +1002,10 @@ class ZipListPhotosView_V2(APIView):
10021002
def post(self, request):
10031003
import shutil
10041004

1005+
from api.views.photo_filters import build_photo_queryset
1006+
10051007
free_storage = shutil.disk_usage("/").free
10061008
data = request.data
1007-
image_hashes = data.get("image_hashes")
1008-
if not image_hashes:
1009-
return Response(data={"error": "image_hashes required"}, status=400)
1010-
1011-
# DRF may provide list values (QueryDict) or a single string
1012-
if isinstance(image_hashes, str):
1013-
image_hashes = [image_hashes]
1014-
elif isinstance(image_hashes, (list, tuple)):
1015-
# QueryDict -> dict() would produce list values, request.data keeps list too
1016-
pass
1017-
else:
1018-
image_hashes = list(image_hashes)
10191009

10201010
include_stacked = data.get("include_stacked_photos", False)
10211011
if isinstance(include_stacked, (list, tuple)):
@@ -1030,8 +1020,39 @@ def post(self, request):
10301020
include_stacked = bool(include_stacked)
10311021

10321022
photo_query = Photo.objects.filter(owner=self.request.user)
1033-
# Filter photos based on image hashes
1034-
photos = photo_query.filter(image_hash__in=image_hashes)
1023+
1024+
# Two payload shapes are accepted, mirroring the other bulk mutations
1025+
# (SetPhotosDeleted, SetFavoritePhotos, SetPhotosHidden, SetPhotosPublic):
1026+
# an explicit `image_hashes` list, or `select_all=True` plus a `query`
1027+
# describing the photoset the user is looking at, with optional
1028+
# `excluded_hashes` for items they unchecked.
1029+
if data.get("select_all"):
1030+
query_params = data.get("query") or {}
1031+
if isinstance(query_params, (list, tuple)):
1032+
query_params = query_params[0] if query_params else {}
1033+
excluded_hashes = data.get("excluded_hashes") or []
1034+
if isinstance(excluded_hashes, str):
1035+
excluded_hashes = [excluded_hashes]
1036+
1037+
photos = build_photo_queryset(self.request.user, query_params)
1038+
if excluded_hashes:
1039+
photos = photos.exclude(image_hash__in=excluded_hashes)
1040+
else:
1041+
image_hashes = data.get("image_hashes")
1042+
if not image_hashes:
1043+
return Response(data={"error": "image_hashes required"}, status=400)
1044+
1045+
# DRF may provide list values (QueryDict) or a single string
1046+
if isinstance(image_hashes, str):
1047+
image_hashes = [image_hashes]
1048+
elif isinstance(image_hashes, (list, tuple)):
1049+
# QueryDict -> dict() would produce list values, request.data keeps list too
1050+
pass
1051+
else:
1052+
image_hashes = list(image_hashes)
1053+
1054+
photos = photo_query.filter(image_hash__in=image_hashes)
1055+
10351056
if not photos.exists():
10361057
return Response(data={"error": "No photos found"}, status=404)
10371058

apps/frontend/src/api_client/jobs/hooks/useDownloadPhotosMutation.ts

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { useMutation } from "@tanstack/react-query";
22
import { notification } from "../../../service/notifications";
33
import { fetchClient } from "../../api";
44
import { serverAddress } from "../../apiClient";
5+
import type { BulkPhotoQuery } from "../../photos/types";
56

67
type StatusResponse = { status: string };
78

@@ -10,11 +11,21 @@ type DownloadResponse = {
1011
job_id: string;
1112
};
1213

13-
type DownloadOptions = {
14+
type IndividualDownloadOptions = {
15+
select_all?: false;
1416
image_hashes: string[];
1517
include_stacked_photos?: boolean;
1618
};
1719

20+
type SelectAllDownloadOptions = {
21+
select_all: true;
22+
query: BulkPhotoQuery;
23+
excluded_hashes?: string[];
24+
include_stacked_photos?: boolean;
25+
};
26+
27+
type DownloadOptions = IndividualDownloadOptions | SelectAllDownloadOptions;
28+
1829
async function startDownloadProcess(options: DownloadOptions) {
1930
const response = await fetchClient.post("/photos/download", options);
2031
return response as DownloadResponse;
@@ -44,19 +55,29 @@ async function downloadFile(filename: string) {
4455
window.URL.revokeObjectURL(downloadUrl);
4556
}
4657

58+
type IndividualMutationArgs = {
59+
select_all?: false;
60+
image_hashes: string[];
61+
userId: number | null;
62+
includeStackedPhotos?: boolean;
63+
};
64+
65+
type SelectAllMutationArgs = {
66+
select_all: true;
67+
query: BulkPhotoQuery;
68+
excluded_hashes?: string[];
69+
userId: number | null;
70+
includeStackedPhotos?: boolean;
71+
};
72+
73+
type DownloadMutationArgs = IndividualMutationArgs | SelectAllMutationArgs;
74+
4775
// Download photos
4876
export const useDownloadPhotosMutation = () =>
4977
useMutation({
50-
mutationFn: async ({
51-
image_hashes,
52-
userId,
53-
includeStackedPhotos = false,
54-
}: {
55-
image_hashes: string[];
56-
userId: number | null;
57-
includeStackedPhotos?: boolean;
58-
}) => {
59-
console.log("[Download] Starting download process", { image_hashes, userId, includeStackedPhotos });
78+
mutationFn: async (args: DownloadMutationArgs) => {
79+
const { userId, includeStackedPhotos = false } = args;
80+
console.log("[Download] Starting download process", { args });
6081
notification.downloadStarting();
6182

6283
if (!userId) {
@@ -65,10 +86,18 @@ export const useDownloadPhotosMutation = () =>
6586
throw new Error("User ID is required for download");
6687
}
6788

68-
const { job_id: jobId, url: filename } = await startDownloadProcess({
69-
image_hashes,
70-
include_stacked_photos: includeStackedPhotos,
71-
});
89+
const downloadRequest: DownloadOptions = args.select_all
90+
? {
91+
select_all: true,
92+
query: args.query,
93+
excluded_hashes: args.excluded_hashes,
94+
include_stacked_photos: includeStackedPhotos,
95+
}
96+
: {
97+
image_hashes: args.image_hashes,
98+
include_stacked_photos: includeStackedPhotos,
99+
};
100+
const { job_id: jobId, url: filename } = await startDownloadProcess(downloadRequest);
72101
console.log("[Download] Job started", { jobId, filename });
73102

74103
const statusInterval = setInterval(async () => {

apps/frontend/src/components/photolist/PhotoListView.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -613,6 +613,7 @@ function PhotoListViewComponent({
613613
selectedItems={selectionState.selectedItems}
614614
selectAllMode={selectionState.selectAllMode}
615615
selectAllQuery={selectionState.selectAllQuery}
616+
totalCount={selectionState.totalCount || numberOfItems || idx2hash.length}
616617
// @ts-ignore
617618
albumID={params ? params.albumID : undefined}
618619
ownerUsername={ownerUsername}

apps/frontend/src/components/photolist/SelectionActions.tsx

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ type Props = {
4545
selectedItems: UserAlbum[];
4646
selectAllMode?: boolean;
4747
selectAllQuery?: BulkPhotoQuery;
48+
totalCount?: number;
4849
updateSelectionState: (state: Partial<SelectionState>) => void;
4950
onSharePhotos: () => void;
5051
setAlbumCover: (actionType: string, photoId?: string) => void;
@@ -73,6 +74,7 @@ export function SelectionActions(props: Readonly<Props>) {
7374
selectedItems,
7475
selectAllMode = false,
7576
selectAllQuery,
77+
totalCount,
7678
updateSelectionState,
7779
onSharePhotos,
7880
onShareAlbum,
@@ -129,20 +131,38 @@ export function SelectionActions(props: Readonly<Props>) {
129131
// Download modal state
130132
const [isDownloadModalOpen, setIsDownloadModalOpen] = useState(false);
131133

134+
// In select-all mode the modal can only show an upper bound, because the
135+
// server-side query is the source of truth and the excluded set may contain
136+
// hashes that aren't currently in `selectedItems` (e.g. items scrolled out of
137+
// virtualised view). Subtracting still gives a reasonable estimate for the UI.
138+
const downloadPhotoCount = selectAllMode
139+
? Math.max(0, (totalCount ?? 0) - getExcludedHashes().length)
140+
: getImageHashes().length;
141+
132142
const handleDownloadConfirm = (options: { includeStackedPhotos: boolean }) => {
133-
downloadPhotoArchive.mutate({
134-
image_hashes: getImageHashes(),
135-
userId,
136-
includeStackedPhotos: options.includeStackedPhotos,
137-
});
143+
if (selectAllMode) {
144+
downloadPhotoArchive.mutate({
145+
select_all: true,
146+
query: selectAllQuery ?? {},
147+
excluded_hashes: getExcludedHashes(),
148+
userId,
149+
includeStackedPhotos: options.includeStackedPhotos,
150+
});
151+
} else {
152+
downloadPhotoArchive.mutate({
153+
image_hashes: getImageHashes(),
154+
userId,
155+
includeStackedPhotos: options.includeStackedPhotos,
156+
});
157+
}
138158
resetSelection();
139159
};
140160

141161
return (
142162
<Group>
143163
<ModalDownloadOptions
144164
isOpen={isDownloadModalOpen}
145-
photoCount={getImageHashes().length}
165+
photoCount={downloadPhotoCount}
146166
onRequestClose={() => setIsDownloadModalOpen(false)}
147167
onConfirm={handleDownloadConfirm}
148168
/>
@@ -331,16 +351,7 @@ export function SelectionActions(props: Readonly<Props>) {
331351

332352
<Menu.Divider />
333353

334-
<Menu.Item
335-
leftSection={<Download />}
336-
disabled={!hasSelection || selectAllMode}
337-
onClick={() => {
338-
// Download doesn't support selectAll mode yet
339-
if (!selectAllMode) {
340-
setIsDownloadModalOpen(true);
341-
}
342-
}}
343-
>
354+
<Menu.Item leftSection={<Download />} disabled={!hasSelection} onClick={() => setIsDownloadModalOpen(true)}>
344355
{` ${t("selectionactions.download")}`}
345356
</Menu.Item>
346357

0 commit comments

Comments
 (0)