Playlist multi add - pre selection of selected lists#4026
Playlist multi add - pre selection of selected lists#4026SergioEstevao wants to merge 3 commits into
Conversation
Generated by 🚫 Danger |
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to pre-select playlists that contain all episodes in a multi-episode selection when adding episodes to manual playlists. The change allows the UI to show which playlists already contain all the selected episodes, improving the user experience in the playlist multi-add flow.
Changes:
- Added a new public API method in DataManager to get manual playlist UUIDs for multiple episodes
- Implemented the underlying database query in PlaylistDataManager to find playlists containing all specified episodes
- The ManualPlaylistsChooserViewController now uses this method to pre-select common playlists
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Modules/DataModel/Sources/PocketCastsDataModel/Public/DataManager.swift | Adds public API method manualPlaylistUUIDs(for:) that accepts an array of episode UUIDs |
| Modules/DataModel/Sources/PocketCastsDataModel/Private/Managers/PlaylistDataManager.swift | Implements database query to find playlists containing all episodes from an array using SQL IN clause and HAVING COUNT |
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | ||
| """ | ||
| let resultSet = try db.executeQuery(query, values: []) | ||
| defer { resultSet.close() } | ||
|
|
||
| while resultSet.next() { | ||
| if let uuid = resultSet.string(forColumn: "playlist_uuid") { | ||
| uuids.append(uuid) | ||
| } | ||
| } | ||
| } catch { | ||
| FileLog.shared.addMessage("PlaylistDataManager.manualPlaylistUUIDs error: \(error)") | ||
| } | ||
| } | ||
| return uuids | ||
| } |
There was a problem hiding this comment.
Missing empty array guard. The function should handle empty arrays gracefully by returning early. When an empty array is passed to convertArrayToInString, it produces an empty string wrapped in quotes (''), which results in invalid SQL syntax. Other similar functions in the codebase (e.g., UserEpisodeDataManager.delete at line 546, PlaylistDataManager.deleteEpisodes at line 297) use a guard statement to check for empty arrays before proceeding with database operations.
|
|
||
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") |
There was a problem hiding this comment.
Commented-out code should be removed. The commented line appears to be leftover from development and should be deleted to keep the code clean.
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") |
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); |
There was a problem hiding this comment.
Inconsistent parameter naming. The parameter is named 'episodesUUIDs' (with an 's' before 'UUIDs') which is inconsistent with the existing single-episode method parameter 'episodeUUID' and the public API parameter 'episodeUUIDs'. The parameter should be renamed to 'episodeUUIDs' for consistency.
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | |
| var uuids: [String] = [] | |
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | |
| dbQueue.read { db in | |
| do { | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | |
| func manualPlaylistUUIDs(for episodeUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | |
| var uuids: [String] = [] | |
| //let episodeUUIDs = episodeUUIDs.joined(separator: ",") | |
| dbQueue.read { db in | |
| do { | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodeUUIDs))) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = \(episodeUUIDs.count); |
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | ||
| """ | ||
| let resultSet = try db.executeQuery(query, values: []) | ||
| defer { resultSet.close() } | ||
|
|
||
| while resultSet.next() { | ||
| if let uuid = resultSet.string(forColumn: "playlist_uuid") { | ||
| uuids.append(uuid) | ||
| } | ||
| } | ||
| } catch { | ||
| FileLog.shared.addMessage("PlaylistDataManager.manualPlaylistUUIDs error: \(error)") | ||
| } | ||
| } | ||
| return uuids | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the new function. The PlaylistDataManagerTests file contains comprehensive test coverage for other PlaylistDataManager methods, but this new function that returns playlists containing all episodes from an array lacks test coverage. Consider adding tests to verify the behavior with multiple episodes, empty arrays, episodes that are only in some playlists, and episodes that share common playlists.
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | ||
| """ | ||
| let resultSet = try db.executeQuery(query, values: []) |
There was a problem hiding this comment.
manualPlaylistUUIDs(for episodesUUIDs: [String]) builds a SQL IN clause by interpolating the result of DataHelper.convertArrayToInString(episodesUUIDs) directly into the query string, which is vulnerable to SQL injection if any episodeUuid contains quotes or SQL metacharacters. Since episode.uuid values are populated from server JSON (episodeJson["uuid"]) and then passed here (e.g. via episodes.map { $0.uuid }), an attacker controlling server responses or intercepting traffic could inject arbitrary SQL and read or modify data in the local database. Instead of string-concatenating the IN list, construct the query using ? placeholders for each UUID and pass the episode UUIDs via the values array so the database layer performs proper escaping/binding.
| var uuids: [String] = [] | |
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | |
| dbQueue.read { db in | |
| do { | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | |
| """ | |
| let resultSet = try db.executeQuery(query, values: []) | |
| // If no episode UUIDs are provided, there can be no matching playlists. | |
| guard !episodesUUIDs.isEmpty else { return [] } | |
| var uuids: [String] = [] | |
| dbQueue.read { db in | |
| do { | |
| let placeholders = Array(repeating: "?", count: episodesUUIDs.count).joined(separator: ", ") | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(placeholders)) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = ? | |
| """ | |
| var values: [Any] = episodesUUIDs | |
| values.append(episodesUUIDs.count) | |
| let resultSet = try db.executeQuery(query, values: values) |
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |
|
Version |
| 📘 Part of: # |
|:---:|
Fixes PCIOS-
To test
Checklist
CHANGELOG.mdif necessary.