From fb464aa45170ed793ad4f97524604489007228b3 Mon Sep 17 00:00:00 2001 From: Nicolas Mauri Date: Thu, 20 Apr 2023 10:58:16 +0200 Subject: [PATCH 1/6] Fix: Refreshing the poll when receiving pollEnd can break the chronological order in the store. --- MatrixSDK/Aggregations/MXAggregations.m | 2 -- changelog.d/pr-1776.bugfix | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) create mode 100644 changelog.d/pr-1776.bugfix diff --git a/MatrixSDK/Aggregations/MXAggregations.m b/MatrixSDK/Aggregations/MXAggregations.m index d94b7b9051..6a5c38b918 100644 --- a/MatrixSDK/Aggregations/MXAggregations.m +++ b/MatrixSDK/Aggregations/MXAggregations.m @@ -315,8 +315,6 @@ - (void)registerListener [self.beaconAggregations handleBeaconWithEvent:event]; } break; - case MXEventTypePollEnd: - [self.aggregatedPollsUpdater refreshPollAfter:event]; default: break; } diff --git a/changelog.d/pr-1776.bugfix b/changelog.d/pr-1776.bugfix new file mode 100644 index 0000000000..29c5b8a8d5 --- /dev/null +++ b/changelog.d/pr-1776.bugfix @@ -0,0 +1 @@ +Poll: Refreshing the poll when receiving pollEnd can break the chronological order in the store. \ No newline at end of file From af3059bb9972447fd529381a8e79efda4292b168 Mon Sep 17 00:00:00 2001 From: Nicolas Mauri Date: Fri, 21 Apr 2023 18:35:32 +0200 Subject: [PATCH 2/6] Fix: PollAggregator will now try to fetch pollStartEvent if it is missing from the store --- MatrixSDK/Room/Polls/PollAggregator.swift | 62 +++++++++++++++++------ 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/MatrixSDK/Room/Polls/PollAggregator.swift b/MatrixSDK/Room/Polls/PollAggregator.swift index 85dadc1a74..4034e98f1f 100644 --- a/MatrixSDK/Room/Polls/PollAggregator.swift +++ b/MatrixSDK/Room/Polls/PollAggregator.swift @@ -88,10 +88,10 @@ public class PollAggregator { throw PollAggregatorError.invalidPollStartEvent } - try self.init(session: session, room: room, pollStartEventId: pollStartEventId, delegate: delegate) + self.init(session: session, room: room, pollStartEventId: pollStartEventId, delegate: delegate) } - public init(session: MXSession, room: MXRoom, pollStartEventId: String, delegate: PollAggregatorDelegate? = nil) throws { + public init(session: MXSession, room: MXRoom, pollStartEventId: String, delegate: PollAggregatorDelegate? = nil) { self.session = session self.room = room self.pollStartEventId = pollStartEventId @@ -100,9 +100,9 @@ public class PollAggregator { NotificationCenter.default.addObserver(self, selector: #selector(handleRoomDataFlush), name: .mxRoomDidFlushData, object: self.room) setupEditListener() - try buildPollStartContent() + buildPollStartContent() } - + private func setupEditListener() { editEventsListener = session.aggregations.listenToEditsUpdate(inRoom: self.room.roomId) { [weak self] event in guard let self = self, @@ -111,20 +111,18 @@ public class PollAggregator { return } - do { - try self.buildPollStartContent() - } catch { - self.delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) - } + self.buildPollStartContent() } } - private func buildPollStartContent() throws { + private func buildPollStartContent() { guard let event = session.store.event(withEventId: pollStartEventId, inRoom: room.roomId), let eventContent = MXEventContentPollStart(fromJSON: event.content), eventContent.answerOptions.count >= Constants.minAnswerOptionCount else { - throw PollAggregatorError.invalidPollStartEvent + // Build a temporary poll without a start event which will be fetched on next reload + buildPollWithoutStartEvent() + return } pollStartedEvent = event @@ -137,8 +135,23 @@ public class PollAggregator { events: events, currentUserIdentifier: session.myUserId, hasBeenEdited: hasBeenEdited) + } + + + /// Creates a temporary poll without having the start event yet + private func buildPollWithoutStartEvent() { + let tempPoll = Poll() + tempPoll.id = pollStartEventId + tempPoll.startDate = Date(timeIntervalSince1970: 0) + tempPoll.hasBeenEdited = false + tempPoll.hasDecryptionError = false + tempPoll.text = "" + tempPoll.maxAllowedSelections = 1 + tempPoll.kind = .undisclosed + tempPoll.answerOptions = [] + tempPoll.isClosed = false - reloadPollData() + poll = tempPoll } @objc private func handleRoomDataFlush(sender: Notification) { @@ -149,14 +162,33 @@ public class PollAggregator { reloadPollData() } - private func reloadPollData() { + public func reloadPollData() { delegate?.pollAggregatorDidStartLoading(self) session.aggregations.referenceEvents(forEvent: pollStartEventId, inRoom: room.roomId, from: nil, limit: -1) { [weak self] response in - guard let self = self else { + guard let self else { return } + guard let event = response.originalEvent else { + // we didn't found the start event + self.delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) + return + } + + // if we don't already have a pollStartedEvent, update it + if pollStartedEvent == nil { + if let pollStartEventContent = MXEventContentPollStart(fromJSON: event.content) { + pollStartedEvent = event + self.pollStartEventContent = pollStartEventContent + hasBeenEdited = (event.unsignedData.relations?.replace != nil) + } else { + // we were not able to decrypt the start event content + self.delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) + return + } + } + self.events.removeAll() self.events.append(contentsOf: response.chunk) @@ -179,7 +211,7 @@ public class PollAggregator { currentUserIdentifier: self.session.myUserId, hasBeenEdited: self.hasBeenEdited) } as Any - + self.poll = self.pollBuilder.build(pollStartEventContent: self.pollStartEventContent, pollStartEvent: self.pollStartedEvent, events: self.events, From a0f2cf9254eb4eaad4a84ac9edf1976cf0990132 Mon Sep 17 00:00:00 2001 From: Nicolas Mauri Date: Wed, 26 Apr 2023 17:46:24 +0200 Subject: [PATCH 3/6] PollAggregator: poll is now optional --- MatrixSDK/Room/Polls/PollAggregator.swift | 52 +++++++---------------- 1 file changed, 16 insertions(+), 36 deletions(-) diff --git a/MatrixSDK/Room/Polls/PollAggregator.swift b/MatrixSDK/Room/Polls/PollAggregator.swift index 4034e98f1f..23778305d1 100644 --- a/MatrixSDK/Room/Polls/PollAggregator.swift +++ b/MatrixSDK/Room/Polls/PollAggregator.swift @@ -54,7 +54,7 @@ public class PollAggregator { private var events: [MXEvent] = [] private var hasBeenEdited = false - public private(set) var poll: PollProtocol! { + public private(set) var poll: PollProtocol? { didSet { delegate?.pollAggregatorDidUpdateData(self) } @@ -100,7 +100,9 @@ public class PollAggregator { NotificationCenter.default.addObserver(self, selector: #selector(handleRoomDataFlush), name: .mxRoomDidFlushData, object: self.room) setupEditListener() - buildPollStartContent() + try? buildPollStartContent() + + reloadPollData() } private func setupEditListener() { @@ -111,18 +113,16 @@ public class PollAggregator { return } - self.buildPollStartContent() + try? self.buildPollStartContent() } } - private func buildPollStartContent() { + private func buildPollStartContent() throws { guard let event = session.store.event(withEventId: pollStartEventId, inRoom: room.roomId), let eventContent = MXEventContentPollStart(fromJSON: event.content), eventContent.answerOptions.count >= Constants.minAnswerOptionCount else { - // Build a temporary poll without a start event which will be fetched on next reload - buildPollWithoutStartEvent() - return + throw PollAggregatorError.invalidPollStartEvent } pollStartedEvent = event @@ -136,24 +136,7 @@ public class PollAggregator { currentUserIdentifier: session.myUserId, hasBeenEdited: hasBeenEdited) } - - - /// Creates a temporary poll without having the start event yet - private func buildPollWithoutStartEvent() { - let tempPoll = Poll() - tempPoll.id = pollStartEventId - tempPoll.startDate = Date(timeIntervalSince1970: 0) - tempPoll.hasBeenEdited = false - tempPoll.hasDecryptionError = false - tempPoll.text = "" - tempPoll.maxAllowedSelections = 1 - tempPoll.kind = .undisclosed - tempPoll.answerOptions = [] - tempPoll.isClosed = false - poll = tempPoll - } - @objc private func handleRoomDataFlush(sender: Notification) { guard let room = sender.object as? MXRoom, room == self.room else { return @@ -162,7 +145,7 @@ public class PollAggregator { reloadPollData() } - public func reloadPollData() { + private func reloadPollData() { delegate?.pollAggregatorDidStartLoading(self) session.aggregations.referenceEvents(forEvent: pollStartEventId, inRoom: room.roomId, from: nil, limit: -1) { [weak self] response in @@ -176,17 +159,14 @@ public class PollAggregator { return } - // if we don't already have a pollStartedEvent, update it - if pollStartedEvent == nil { - if let pollStartEventContent = MXEventContentPollStart(fromJSON: event.content) { - pollStartedEvent = event - self.pollStartEventContent = pollStartEventContent - hasBeenEdited = (event.unsignedData.relations?.replace != nil) - } else { - // we were not able to decrypt the start event content - self.delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) - return - } + if let pollStartEventContent = MXEventContentPollStart(fromJSON: event.content), pollStartEventContent.answerOptions.count >= Constants.minAnswerOptionCount { + pollStartedEvent = event + self.pollStartEventContent = pollStartEventContent + hasBeenEdited = (event.unsignedData.relations?.replace != nil) + } else { + // we were not able to decrypt the start event content + self.delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) + return } self.events.removeAll() From 99227c7500e948cd00a6e9ed84c4bbfcffacf169 Mon Sep 17 00:00:00 2001 From: Nicolas Mauri Date: Thu, 27 Apr 2023 15:01:27 +0200 Subject: [PATCH 4/6] Fix: PollAggregator - code refactoring --- MatrixSDK/Room/Polls/PollAggregator.swift | 55 +++++++++++------------ 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/MatrixSDK/Room/Polls/PollAggregator.swift b/MatrixSDK/Room/Polls/PollAggregator.swift index 23778305d1..5b886a3e53 100644 --- a/MatrixSDK/Room/Polls/PollAggregator.swift +++ b/MatrixSDK/Room/Polls/PollAggregator.swift @@ -100,11 +100,11 @@ public class PollAggregator { NotificationCenter.default.addObserver(self, selector: #selector(handleRoomDataFlush), name: .mxRoomDidFlushData, object: self.room) setupEditListener() - try? buildPollStartContent() + buildPollStartContent() reloadPollData() } - + private func setupEditListener() { editEventsListener = session.aggregations.listenToEditsUpdate(inRoom: self.room.roomId) { [weak self] event in guard let self = self, @@ -113,30 +113,37 @@ public class PollAggregator { return } - try? self.buildPollStartContent() + self.buildPollStartContent() } } - private func buildPollStartContent() throws { - guard let event = session.store.event(withEventId: pollStartEventId, inRoom: room.roomId), - let eventContent = MXEventContentPollStart(fromJSON: event.content), - eventContent.answerOptions.count >= Constants.minAnswerOptionCount + private func buildPollStartContent() { + let event = session.store.event(withEventId: pollStartEventId, inRoom: room.roomId) + tryUpdatePollStartedEvent(with: event) + if let pollStartedEvent { + poll = pollBuilder.build(pollStartEventContent: pollStartEventContent, + pollStartEvent: pollStartedEvent, + events: events, + currentUserIdentifier: session.myUserId, + hasBeenEdited: hasBeenEdited) + } + } + + private func tryUpdatePollStartedEvent(with event: MXEvent?) { + guard + let event = event, + let eventContent = MXEventContentPollStart(fromJSON: event.content), + eventContent.answerOptions.count >= Constants.minAnswerOptionCount else { - throw PollAggregatorError.invalidPollStartEvent + delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) + return } pollStartedEvent = event pollStartEventContent = eventContent - hasBeenEdited = (event.unsignedData.relations?.replace != nil) - - poll = pollBuilder.build(pollStartEventContent: eventContent, - pollStartEvent: pollStartedEvent, - events: events, - currentUserIdentifier: session.myUserId, - hasBeenEdited: hasBeenEdited) } - + @objc private func handleRoomDataFlush(sender: Notification) { guard let room = sender.object as? MXRoom, room == self.room else { return @@ -153,24 +160,12 @@ public class PollAggregator { return } - guard let event = response.originalEvent else { - // we didn't found the start event - self.delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) - return - } - - if let pollStartEventContent = MXEventContentPollStart(fromJSON: event.content), pollStartEventContent.answerOptions.count >= Constants.minAnswerOptionCount { - pollStartedEvent = event - self.pollStartEventContent = pollStartEventContent - hasBeenEdited = (event.unsignedData.relations?.replace != nil) - } else { - // we were not able to decrypt the start event content - self.delegate?.pollAggregator(self, didFailWithError: PollAggregatorError.invalidPollStartEvent) + self.tryUpdatePollStartedEvent(with: response.originalEvent) + if self.pollStartedEvent == nil { return } self.events.removeAll() - self.events.append(contentsOf: response.chunk) let eventTypes = [kMXEventTypeStringPollResponse, kMXEventTypeStringPollResponseMSC3381, kMXEventTypeStringPollEnd, kMXEventTypeStringPollEndMSC3381] From 45ca4338042445af433b8f0c323315010ef9783a Mon Sep 17 00:00:00 2001 From: Nicolas Mauri Date: Thu, 27 Apr 2023 15:22:51 +0200 Subject: [PATCH 5/6] Fix: compilation issue --- MatrixSDK/Room/Polls/PollAggregator.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MatrixSDK/Room/Polls/PollAggregator.swift b/MatrixSDK/Room/Polls/PollAggregator.swift index 5b886a3e53..63ded7ae1d 100644 --- a/MatrixSDK/Room/Polls/PollAggregator.swift +++ b/MatrixSDK/Room/Polls/PollAggregator.swift @@ -120,7 +120,7 @@ public class PollAggregator { private func buildPollStartContent() { let event = session.store.event(withEventId: pollStartEventId, inRoom: room.roomId) tryUpdatePollStartedEvent(with: event) - if let pollStartedEvent { + if let pollStartedEvent = pollStartedEvent { poll = pollBuilder.build(pollStartEventContent: pollStartEventContent, pollStartEvent: pollStartedEvent, events: events, @@ -156,7 +156,7 @@ public class PollAggregator { delegate?.pollAggregatorDidStartLoading(self) session.aggregations.referenceEvents(forEvent: pollStartEventId, inRoom: room.roomId, from: nil, limit: -1) { [weak self] response in - guard let self else { + guard let self = self else { return } From fb064c4fd09535be7dbd610397b24e7d13c42860 Mon Sep 17 00:00:00 2001 From: Nicolas Mauri Date: Thu, 27 Apr 2023 15:52:41 +0200 Subject: [PATCH 6/6] Fix: UnitTests --- MatrixSDKTests/MXCryptoSecretShareTests.m | 2 +- MatrixSDKTests/MXPollAggregatorTests.swift | 34 +++++++++++----------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/MatrixSDKTests/MXCryptoSecretShareTests.m b/MatrixSDKTests/MXCryptoSecretShareTests.m index f5763032a9..63f07c624b 100644 --- a/MatrixSDKTests/MXCryptoSecretShareTests.m +++ b/MatrixSDKTests/MXCryptoSecretShareTests.m @@ -114,7 +114,7 @@ - (void)testSecretShare // -> She gets the secret XCTAssertEqualObjects(sharedSecret, secret); [expectation fulfill]; - + return YES; } failure:^(NSError * _Nonnull error) { XCTFail(@"The operation should not fail - NSError: %@", error); [expectation fulfill]; diff --git a/MatrixSDKTests/MXPollAggregatorTests.swift b/MatrixSDKTests/MXPollAggregatorTests.swift index 9862243eda..0f7f0825f4 100644 --- a/MatrixSDKTests/MXPollAggregatorTests.swift +++ b/MatrixSDKTests/MXPollAggregatorTests.swift @@ -40,12 +40,12 @@ class MXPollAggregatorTest: XCTestCase { func testAggregations() { self.createScenarioForBobAndAlice { bobSession, aliceSession, bobRoom, aliceRoom, pollStartEvent, expectation in self.delegate = PollAggregatorBlockWrapper(dataUpdateCallback: { pollAggregator in - XCTAssertEqual(self.pollAggregator.poll.answerOptions.first!.count, 2) - XCTAssertEqual(self.pollAggregator.poll.answerOptions.last!.count, 0) + XCTAssertEqual(self.pollAggregator.poll?.answerOptions.first!.count, 2) + XCTAssertEqual(self.pollAggregator.poll?.answerOptions.last!.count, 0) expectation.fulfill() }) - self.pollAggregator = try! PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) + self.pollAggregator = PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) let dispatchGroup = DispatchGroup() @@ -74,14 +74,14 @@ class MXPollAggregatorTest: XCTestCase { func testSessionPausing() { self.createScenarioForBobAndAlice { bobSession, aliceSession, bobRoom, aliceRoom, pollStartEvent, expectation in let delegate = PollAggregatorBlockWrapper(dataUpdateCallback: { aggregator in - XCTAssertEqual(aggregator.poll.answerOptions.first!.count, 2) - XCTAssertEqual(aggregator.poll.answerOptions.last!.count, 0) + XCTAssertEqual(aggregator.poll?.answerOptions.first!.count, 2) + XCTAssertEqual(aggregator.poll?.answerOptions.last!.count, 0) }) - self.pollAggregator = try! PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) + self.pollAggregator = PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) - XCTAssertEqual(self.pollAggregator.poll.answerOptions.first!.count, 1) // One from Alice - XCTAssertEqual(self.pollAggregator.poll.answerOptions.last!.count, 0) + XCTAssertEqual(self.pollAggregator.poll?.answerOptions.first!.count, 1) // One from Alice + XCTAssertEqual(self.pollAggregator.poll?.answerOptions.last!.count, 0) bobSession.pause() @@ -99,16 +99,16 @@ class MXPollAggregatorTest: XCTestCase { func testGappySync() { self.createScenarioForBobAndAlice { bobSession, aliceSession, bobRoom, aliceRoom, pollStartEvent, expectation in - self.pollAggregator = try! PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) + self.pollAggregator = PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) self.delegate = PollAggregatorBlockWrapper(dataUpdateCallback: { aggregator in - XCTAssertEqual(aggregator.poll.answerOptions.first!.count, 2) // One from Bob and one from Alice - XCTAssertEqual(aggregator.poll.answerOptions.last!.count, 1) // One from Alice + XCTAssertEqual(aggregator.poll?.answerOptions.first!.count, 2) // One from Bob and one from Alice + XCTAssertEqual(aggregator.poll?.answerOptions.last!.count, 1) // One from Alice expectation.fulfill() }) - XCTAssertEqual(self.pollAggregator.poll.answerOptions.first!.count, 1) // One from Alice - XCTAssertEqual(self.pollAggregator.poll.answerOptions.last!.count, 0) + XCTAssertEqual(self.pollAggregator.poll?.answerOptions.first!.count, 1) // One from Alice + XCTAssertEqual(self.pollAggregator.poll?.answerOptions.last!.count, 0) bobSession.pause() @@ -135,7 +135,7 @@ class MXPollAggregatorTest: XCTestCase { func testEditing() { self.createScenarioForBobAndAlice { bobSession, aliceSession, bobRoom, aliceRoom, pollStartEvent, expectation in - self.pollAggregator = try! PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) + self.pollAggregator = PollAggregator(session: bobSession, room: bobRoom, pollStartEventId: pollStartEvent.eventId) self.delegate = PollAggregatorBlockWrapper(dataUpdateCallback: { aggregator in defer { @@ -144,9 +144,9 @@ class MXPollAggregatorTest: XCTestCase { guard self.isFirstDelegateUpdate else { return } - XCTAssertEqual(aggregator.poll.text, "Some other question") - XCTAssertEqual(aggregator.poll.answerOptions.count, 2) - XCTAssertTrue(aggregator.poll.hasBeenEdited) + XCTAssertEqual(aggregator.poll?.text, "Some other question") + XCTAssertEqual(aggregator.poll?.answerOptions.count, 2) + XCTAssertEqual(aggregator.poll?.hasBeenEdited, true) expectation.fulfill() })