From 1f2951e2531c668b026c82ebc96c91950507a6eb Mon Sep 17 00:00:00 2001 From: Jason Morley Date: Mon, 28 Nov 2022 11:52:45 +0000 Subject: [PATCH] fix: Rules variable are incorrectly updated after cancelling edit on iOS (#509) The iOS app has modal rule editing which takes a copy of the rule models on edit, only updating the rule set on save. Unfortunately, we weren't performing a full deep copy of the rule model, meaning that changes were being applied and written to disk, even on cancel. --- .../Configuration/VariableModel.swift | 13 +++- .../FileawayCore/Model/RuleModel.swift | 18 ++--- .../FileawayCore/Model/RulesModel.swift | 2 +- .../Extensions/XCTestCase.swift | 48 +++++++++++++ .../FileawayCoreTests/RuleModelTests.swift | 71 +++++++++++++++++++ .../FileawayCoreTests/RulesModelTests.swift | 27 +------ ios/Fileaway/Views/Editor/RuleView.swift | 2 +- .../Views/Settings/VariablesTable.swift | 2 +- 8 files changed, 145 insertions(+), 38 deletions(-) create mode 100644 core/Tests/FileawayCoreTests/Extensions/XCTestCase.swift create mode 100644 core/Tests/FileawayCoreTests/RuleModelTests.swift diff --git a/core/Sources/FileawayCore/Configuration/VariableModel.swift b/core/Sources/FileawayCore/Configuration/VariableModel.swift index aac54a8d..2b8b420f 100644 --- a/core/Sources/FileawayCore/Configuration/VariableModel.swift +++ b/core/Sources/FileawayCore/Configuration/VariableModel.swift @@ -47,7 +47,7 @@ public class VariableModel: ObservableObject, Identifiable, Codable, Hashable { return lhs.id == rhs.id } - public var id = UUID() + public let id: UUID @Published public var name: String @Published public var type: VariableType @@ -56,17 +56,26 @@ public class VariableModel: ObservableObject, Identifiable, Codable, Hashable { return HashRainbow.colorForString(name, colors: HashRainbow.NeonColors) } - public init(name: String, type: VariableType) { + public init(id: UUID = UUID(), name: String, type: VariableType) { + self.id = id self.name = name self.type = type } + public convenience init(_ variableModel: VariableModel) { + self.init(id: variableModel.id, + name: variableModel.name, + type: variableModel.type) + } + required public init(from decoder: Decoder) throws { let container = try decoder.container(keyedBy: CodingKeys.self) let name = try container.decode(String.self, forKey: .name) let type = try container.decode(RawType.self, forKey: .type) + id = UUID() + switch type { case .string: self.name = name diff --git a/core/Sources/FileawayCore/Model/RuleModel.swift b/core/Sources/FileawayCore/Model/RuleModel.swift index 16cbc7da..379fb02b 100644 --- a/core/Sources/FileawayCore/Model/RuleModel.swift +++ b/core/Sources/FileawayCore/Model/RuleModel.swift @@ -63,9 +63,9 @@ public class RuleModel: ObservableObject, Identifiable, CustomStringConvertible, } } - public init(id: UUID, rootUrl: URL, name: String, variables: [VariableModel], destination: [ComponentModel]) { + public init(id: UUID, archiveURL: URL, name: String, variables: [VariableModel], destination: [ComponentModel]) { self.id = id - self.archiveURL = rootUrl + self.archiveURL = archiveURL self.name = name self.variables = variables self.destination = destination @@ -81,15 +81,15 @@ public class RuleModel: ObservableObject, Identifiable, CustomStringConvertible, public convenience init(_ ruleModel: RuleModel) { self.init(id: ruleModel.id, - rootUrl: ruleModel.archiveURL, + archiveURL: ruleModel.archiveURL, name: String(ruleModel.name), - variables: ruleModel.variables, + variables: ruleModel.variables.map { VariableModel($0) }, destination: ruleModel.destination.map { ComponentModel($0, variable: nil) }) } public convenience init(id: UUID, ruleModel: RuleModel) { self.init(id: id, - rootUrl: ruleModel.archiveURL, + archiveURL: ruleModel.archiveURL, name: String(ruleModel.name), variables: ruleModel.variables, destination: ruleModel.destination.map { ComponentModel($0, variable: nil) }) @@ -97,7 +97,7 @@ public class RuleModel: ObservableObject, Identifiable, CustomStringConvertible, public convenience init(rootUrl: URL, rule: Rule) { self.init(id: rule.id, - rootUrl: rootUrl, + archiveURL: rootUrl, name: rule.name, variables: rule.variables, destination: rule.destination.map { component in @@ -187,7 +187,7 @@ public class RuleModel: ObservableObject, Identifiable, CustomStringConvertible, return names.count == variables.count && !name.isEmpty } - public func createVariable() { + public func createVariable() -> VariableModel { let names = Set(variables.map { $0.name }) var index = 1 var name = "" @@ -195,7 +195,9 @@ public class RuleModel: ObservableObject, Identifiable, CustomStringConvertible, name = "Variable \(index)" index = index + 1 } while names.contains(name) - self.variables.append(VariableModel(name: name, type: .string)) + let variable = VariableModel(name: name, type: .string) + self.variables.append(variable) + return variable } public static func == (lhs: RuleModel, rhs: RuleModel) -> Bool { diff --git a/core/Sources/FileawayCore/Model/RulesModel.swift b/core/Sources/FileawayCore/Model/RulesModel.swift index 4f9d9031..d4523aae 100644 --- a/core/Sources/FileawayCore/Model/RulesModel.swift +++ b/core/Sources/FileawayCore/Model/RulesModel.swift @@ -94,7 +94,7 @@ public class RulesModel: ObservableObject { public func new() throws -> RuleModel { let name = uniqueRuleName(preferredName: "Rule") let rule = RuleModel(id: UUID(), - rootUrl: archiveURL, + archiveURL: archiveURL, name: name, variables: [VariableModel(name: "Date", type: .date(hasDay: true))], destination: [ diff --git a/core/Tests/FileawayCoreTests/Extensions/XCTestCase.swift b/core/Tests/FileawayCoreTests/Extensions/XCTestCase.swift new file mode 100644 index 00000000..06f967d7 --- /dev/null +++ b/core/Tests/FileawayCoreTests/Extensions/XCTestCase.swift @@ -0,0 +1,48 @@ +// Copyright (c) 2018-2022 InSeven Limited +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +import XCTest + +extension XCTestCase { + + func createTemporaryDirectory() throws -> URL { + + let fileManager = FileManager.default + let directoryURL = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent(UUID().uuidString) + try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true) + + var isDirectory: ObjCBool = false + XCTAssertTrue(fileManager.fileExists(atPath: directoryURL.path, isDirectory: &isDirectory)) + XCTAssert(isDirectory.boolValue) + + addTeardownBlock { + do { + let fileManager = FileManager.default + try fileManager.removeItem(at: directoryURL) + XCTAssertFalse(fileManager.fileExists(atPath: directoryURL.path)) + } catch { + XCTFail("Failed to delete temporary directory with error \(error).") + } + } + + return directoryURL + } + +} diff --git a/core/Tests/FileawayCoreTests/RuleModelTests.swift b/core/Tests/FileawayCoreTests/RuleModelTests.swift new file mode 100644 index 00000000..0a3d7283 --- /dev/null +++ b/core/Tests/FileawayCoreTests/RuleModelTests.swift @@ -0,0 +1,71 @@ +// Copyright (c) 2018-2022 InSeven Limited +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +import XCTest + +@testable import FileawayCore + +class RuleModelTests: XCTestCase { + + func testDeepCopy() throws { + + let archiveURL = try createTemporaryDirectory() + let rule1 = RuleModel(id: UUID(), + archiveURL: archiveURL, + name: "Rule", + variables: [], + destination: []) + _ = rule1.createVariable() + XCTAssertEqual(rule1.variables.count, 1) + XCTAssertEqual(rule1.variables[0].name, "Variable 1") + XCTAssertEqual(rule1.variables[0].type, .string) + + // Ensure that we deep copy a rule so that modifications to the new model do not affect any component + // of the original. + + // Variables. + + let rule2 = RuleModel(rule1) + + // Duplicate variables should have the same identifiers and should be identical on first copy. + XCTAssertEqual(rule1.variables[0].id, rule2.variables[0].id) + XCTAssertEqual(rule1.variables, rule2.variables) + + // Create a new variable and ensure the variable lists now differ. + _ = rule2.createVariable() + XCTAssertEqual(rule1.variables.count, 1) + XCTAssertEqual(rule2.variables.count, 2) + + rule2.variables[0].name = "Description" + XCTAssertEqual(rule1.variables[0].name, "Variable 1") + XCTAssertNotEqual(rule1.variables[0].name, rule2.variables[0].name) + + rule2.variables[0].type = .date(hasDay: true) + XCTAssertEqual(rule1.variables[0].type, .string) + XCTAssertNotEqual(rule1.variables[0].type, rule2.variables[0].type) + + // Destination. + + // TODO: Test RuleModel.destination deep copy behaviour #510 + // https://github.com/inseven/fileaway/issues/510 + + } + +} diff --git a/core/Tests/FileawayCoreTests/RulesModelTests.swift b/core/Tests/FileawayCoreTests/RulesModelTests.swift index 613acfba..d183b1dd 100644 --- a/core/Tests/FileawayCoreTests/RulesModelTests.swift +++ b/core/Tests/FileawayCoreTests/RulesModelTests.swift @@ -26,37 +26,14 @@ import XCTest // https://github.com/inseven/fileaway/issues/500 class RulesModelTests: XCTestCase { - func temporaryDirectoryURL() throws -> URL { - - let fileManager = FileManager.default - let directoryURL = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent(UUID().uuidString) - try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true) - - var isDirectory: ObjCBool = false - XCTAssertTrue(fileManager.fileExists(atPath: directoryURL.path, isDirectory: &isDirectory)) - XCTAssert(isDirectory.boolValue) - - addTeardownBlock { - do { - let fileManager = FileManager.default - try fileManager.removeItem(at: directoryURL) - XCTAssertFalse(fileManager.fileExists(atPath: directoryURL.path)) - } catch { - XCTFail("Failed to delete temporary directory with error \(error).") - } - } - - return directoryURL - } - func emptyModel() throws -> RulesModel { - let archiveURL = try temporaryDirectoryURL() + let archiveURL = try createTemporaryDirectory() let rules = RulesModel(archiveURL: archiveURL) return rules } func testNewRule() throws { - let archiveURL = try temporaryDirectoryURL() + let archiveURL = try createTemporaryDirectory() let rules = RulesModel(archiveURL: archiveURL) XCTAssert(rules.ruleModels.isEmpty) diff --git a/ios/Fileaway/Views/Editor/RuleView.swift b/ios/Fileaway/Views/Editor/RuleView.swift index d44aac92..9b8c6f91 100644 --- a/ios/Fileaway/Views/Editor/RuleView.swift +++ b/ios/Fileaway/Views/Editor/RuleView.swift @@ -65,7 +65,7 @@ struct RuleView: View { if self.editMode == .active { Button { withAnimation { - self.editingRuleModel.createVariable() + _ = self.editingRuleModel.createVariable() } } label: { Text("New Variable...") diff --git a/macos/Fileaway/Views/Settings/VariablesTable.swift b/macos/Fileaway/Views/Settings/VariablesTable.swift index f4bbb780..6fa0e5c5 100644 --- a/macos/Fileaway/Views/Settings/VariablesTable.swift +++ b/macos/Fileaway/Views/Settings/VariablesTable.swift @@ -52,7 +52,7 @@ struct VariablesTable: View { VStack { VStack { Button { - ruleModel.createVariable() + _ = ruleModel.createVariable() } label: { Text("Add") .frame(width: 80)