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.

Sign up for GitHub

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

Jump to bottom

Settings Sync: Fix Up Next settings #1615

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Settings Sync: Fix Up Next settings #1615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab00f17
Fix UpNext settings
bjtitus Apr 10, 2024
47487ee
Fix Up Next query for podcast settings
bjtitus Apr 12, 2024
885a5f5
Add unit test for Podcast autoAddToUpNext candidates
bjtitus Apr 12, 2024
0703a33
PR feedback
bjtitus Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions ...les/DataModel/Sources/PocketCastsDataModel/Private/Managers/AutoAddQueueDataManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public struct AutoAddCandidatesDataManager {
SELECT
-- Get the Podcast Auto Add Setting
json_extract(podcast.settings, '$.addToUpNextPosition.value') AS \(Constants.autoAddSettingColumnName),
json_extract(podcast.settings, '$.addToUpNext.value') AS \(Constants.autoAddEnabledColumnName),

-- Get the episode UUID
queue.id AS \(Constants.idColumnName),
Expand Down Expand Up @@ -118,12 +119,13 @@ public struct AutoAddCandidatesDataManager {

let setting: Int32
if FeatureFlag.newSettingsStorage.enabled {
let value = resultSet.int(forColumn: Constants.autoAddSettingColumnName)
let position = UpNextPosition(rawValue: value)
switch position {
case .top:
let enabledValue = resultSet.bool(forColumn: Constants.autoAddSettingColumnName)
let positionValue = resultSet.int(forColumn: Constants.autoAddSettingColumnName)
let position = UpNextPosition(rawValue: positionValue)
switch (enabledValue, position) {
case (true, .top):
setting = AutoAddToUpNextSetting.addFirst.rawValue
case .bottom:
case (true, .bottom):
setting = AutoAddToUpNextSetting.addLast.rawValue
default:
setting = AutoAddToUpNextSetting.off.rawValue
Expand Down Expand Up @@ -151,6 +153,7 @@ public struct AutoAddCandidatesDataManager {
private enum Constants {
static let tableName = "AutoAddCandidates"
static let autoAddSettingColumnName = "auto_add_setting"
static let autoAddEnabledColumnName = "auto_add_enabled"
static let settingsColumnName = "settings"
static let episodeColumnName = "episode_uuid"
static let idColumnName = "id"
Expand Down
29 changes: 20 additions & 9 deletions ...les/DataModel/Sources/PocketCastsDataModel/Public/Model/PodcastSettings+OldSettings.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,27 @@ extension PodcastSettings {
}
}
set {
switch newValue {
case .addFirst:
addToUpNext = true
addToUpNextPosition = .top
case .addLast:
addToUpNext = true
addToUpNextPosition = .bottom
case .off:
addToUpNext = false
addToUpNext = newValue.enabled
if let position = newValue.position {
addToUpNextPosition = position
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it matters, but shouldn't addToUpNextPosition be optional? In case it has no position, is nil.

As it stands it will be the previous value that had a position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be optional in the Protobuf settings so I was thinking we shouldn't make it optional in our local model.

}
}
}
}

public extension AutoAddToUpNextSetting {
var enabled: Bool {
return self != .off
}

var position: UpNextPosition? {
switch self {
case .addFirst:
return .top
case .addLast:
return .bottom
case .off:
return nil
}
}
}
11 changes: 11 additions & 0 deletions Modules/DataModel/Tests/PocketCastsDataModelTests/AutoAddCandidatesDataManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ final class AutoAddCandidatesDataManagerTests: XCTestCase {

let dataManager = try setupDatabase()
let newUpNextSetting = AutoAddToUpNextSetting.addFirst
let secondUpNextSetting = AutoAddToUpNextSetting.off

let podcast = Podcast()
podcast.uuid = "1234"
Expand All @@ -58,6 +59,16 @@ final class AutoAddCandidatesDataManagerTests: XCTestCase {
XCTAssertNotNil(matchingCandidate, "Episode should appear in Up Next candidates")
XCTAssertEqual(matchingCandidate?.autoAddToUpNextSetting, newUpNextSetting)

// Disable the setting and ensure it is removed from candidates

podcast.setAutoAddToUpNext(setting: secondUpNextSetting)
dataManager.save(podcast: podcast)

let offCandidates = dataManager.autoAddCandidates.candidates()

let matchingOffCandidate = offCandidates.first(where: { $0.episodeUuid == episode.uuid })
XCTAssertEqual(matchingOffCandidate?.autoAddToUpNextSetting, secondUpNextSetting)

featureFlagMock.reset()
}

Expand Down
5 changes: 4 additions & 1 deletion podcasts/DataManager+Import.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ extension DataManager {
}

if let setting = AutoAddToUpNextSetting(rawValue: podcast.autoAddToUpNext) {
podcast.settings.autoUpNextSetting = setting
podcast.settings.addToUpNext = setting.enabled
if let position = setting.position {
podcast.settings.addToUpNextPosition = position
}
}

save(podcast: podcast, cache: idx == podcasts.endIndex)
Expand Down
Loading