Fixes ended poll voting #5473

ericdecanini · 2022-03-09T11:36:21Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Fixes polls being votable after being ended

Motivation and context

The cause of the issue is found in EventRelationsAggregationProcessor:388.

When voting, the SDK checks that the eventTimestamp is before the closed time and if it is, the vote response can go through, despite the poll having already been ended (determined by the non-null presence of a closed time).

The fix on the SDK was to simply remove that extra condition. I checked that this shouldn't have any side-effects. I tried to approach this differently, i.e. trying to fix the event timestamp (event.originServerTs) set during the poll ending so that it matches the poll closed time but to no avail.

Though I should be able to investigate this with a bit more time if that's the preferrable course of action

I also added an extra layer of protection on the client side to check that the poll is in an ended state.

Screenshots / GIFs

Tests

Set your device system time to 1 hour ahead of current time
In any timeline, create a new poll
Immediately end it
Try to vote on it and see that it cannot be voted

Tested devices

Physical
Emulator
OS version(s): Android 10

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

github-actions · 2022-03-09T12:10:59Z

Unit Test Results

106 files ±0 106 suites ±0 1m 20s ⏱️ +6s
188 tests ±0 188 ✔️ ±0 0 💤 ±0 0 ❌ ±0
622 runs ±0 622 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit edfe81c. ± Comparison against base commit 84b34f7.

♻️ This comment has been updated with latest results.

ouchadam · 2022-03-11T12:37:30Z

...main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt

@@ -385,7 +385,7 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
                }

        val closedTime = existingPollSummary?.closedTime
-        if (closedTime != null && eventTimestamp > closedTime) {
+        if (closedTime != null) {


is there a case where the poll could have a closed timestamp but still be valid to casting votes? cc @onurays

I think we should keep timestamp condition. Even if we prevent it previously, there is always a chance to trigger a poll response to an ended poll from another client or via rest api.

I'll investigate alternative solutions to this before merging

Reverted this change and pushed alternative solution. See below comment for details

bmarty

One remark.

You may have seen it, but this is where the SDK ignore late vote FWIW: https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt#L388

bmarty · 2022-03-11T16:25:23Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollItem.kt

+        }
+    }
+
+    private fun isPollActive(optionViewState: PollOptionViewState) = optionViewState !is PollOptionViewState.PollEnded


There is too much logic here. Better to move that to the controller.
Maybe replace var pollSent: Boolean = false which is unused by something like var canVote: Boolean = false

…-ended-poll # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

ericdecanini · 2022-03-13T19:18:32Z

Update: refactored MessageItemFactory's buildPollItem method and a few other poll-related classes

…-ended-poll # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt # vector/src/main/java/im/vector/app/features/poll/create/CreatePollViewState.kt

ouchadam · 2022-03-15T15:07:47Z

vector/src/main/java/im/vector/app/features/poll/PollState.kt

+
+package im.vector.app.features.poll
+
+sealed class PollState {


tiny nit pick, if we're not using private base properties/functions in the base sealed class we can use sealed interface instead (which is more flexible and helps avoid duplicated properties!)

sealed interfaces are awesome, I struggle to find cases where a sealed classes would be better 🤔

This has never crossed my mind, but I totally agree! Changes pushed

ouchadam

thanks for all the cleaning up 💯

would be good to double check with @onurays for the original timestamp condition in case there's a scenario we aren't aware of

onurays · 2022-03-15T17:15:02Z

...src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

+            is MessageNoticeContent -> buildNoticeMessageItem(messageContent, informationData, highlight, callback, attributes)
+            is MessageVideoContent -> buildVideoMessageItem(messageContent, informationData, highlight, callback, attributes)
+            is MessageFileContent -> buildFileMessageItem(messageContent, highlight, attributes)
+            is MessageAudioContent -> {


The arrows of when conditions should be at the same column. Could you please check your formatter? (We had this problem on a previous Android Studio).

@bmarty we need to discuss / revert this later.

onurays

Thanks. Statically reviewed, LGTM.

ericdecanini · 2022-03-17T12:51:59Z

...main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt

                }

-        if (existingPollSummary.closedTime != null) {


The source of the bug is here. A local echo being processed before the remote echo meant that the remote echo never reaches past this point. closedTime thus never gets set from the remote and always turns out to be the current time millis of the local device that ended the poll. If the device has time that's out of sync with unix current time, this bug can occur

cc @onurays

I see, thanks for the clarification.

…-ended-poll # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

Adds server side vote blocking for ended polls

e4ce4ab

ericdecanini added 6 commits March 10, 2022 16:28

Removes event timestamp condition for sdk poll voting

080844d

Reverts timestamp condition but changes timing of setting closedTime

628a160

Fixes warning for debugging

df6ae4b

Reverts to fix by removing event timestamp condition

fe3c9cc

Reverts to fix by removing event timestamp condition

6db1c37

Reverts debug isPollActive

6a59007

ericdecanini marked this pull request as ready for review March 10, 2022 20:27

ouchadam reviewed Mar 11, 2022

View reviewed changes

bmarty reviewed Mar 11, 2022

View reviewed changes

ericdecanini added 2 commits March 13, 2022 20:09

Refactors buildPollItem in MessageItemFactory

610c67c

Merge remote-tracking branch 'origin/develop' into bugfix/eric/voting…

f24d8c2

…-ended-poll # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

ericdecanini added 4 commits March 13, 2022 20:22

Adds new line at the end of PollState

86765c9

Fixes import ordering in DefaultNavigator

06e0047

Adds getBestX following merge from develop

98b7d19

ouchadam reviewed Mar 15, 2022

View reviewed changes

ouchadam approved these changes Mar 15, 2022

View reviewed changes

ericdecanini added 2 commits March 15, 2022 18:10

Fixes PollMode import error in TimelineFragment

a2d18d4

Makes PollState a sealed interface

a46901a

onurays reviewed Mar 15, 2022

View reviewed changes

onurays approved these changes Mar 15, 2022

View reviewed changes

ericdecanini added 3 commits March 17, 2022 12:07

Merge branch 'develop' into bugfix/eric/voting-ended-poll

9c8f29e

Fixes crash due to empty constructor in CreatePollViewState

d11fc06

Fixes remote echo of end poll not processing correctly

fbb6f11

ericdecanini requested a review from onurays March 17, 2022 12:48

ericdecanini commented Mar 17, 2022

View reviewed changes

onurays approved these changes Mar 22, 2022

View reviewed changes

ericdecanini added 2 commits March 22, 2022 14:15

Merge remote-tracking branch 'origin/develop' into bugfix/eric/voting…

7449d15

…-ended-poll # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

Merge remote-tracking branch 'origin/develop' into bugfix/eric/voting…

edfe81c

…-ended-poll # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

ericdecanini merged commit 1097436 into develop Mar 24, 2022

ericdecanini deleted the bugfix/eric/voting-ended-poll branch March 24, 2022 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes ended poll voting #5473

Fixes ended poll voting #5473

ericdecanini commented Mar 9, 2022 •

edited

github-actions bot commented Mar 9, 2022 •

edited

ouchadam Mar 11, 2022

onurays Mar 15, 2022 •

edited

ericdecanini Mar 15, 2022

ericdecanini Mar 17, 2022

bmarty left a comment

bmarty Mar 11, 2022

ericdecanini commented Mar 13, 2022

ouchadam Mar 15, 2022

ericdecanini Mar 15, 2022

ouchadam left a comment

onurays Mar 15, 2022

onurays Mar 15, 2022

onurays left a comment

ericdecanini Mar 17, 2022

onurays Mar 22, 2022

Fixes ended poll voting #5473

Fixes ended poll voting #5473

Conversation

ericdecanini commented Mar 9, 2022 • edited

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Mar 9, 2022 • edited

Unit Test Results

Choose a reason for hiding this comment

onurays Mar 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdecanini commented Mar 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurays left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdecanini commented Mar 9, 2022 •

edited

github-actions bot commented Mar 9, 2022 •

edited

onurays Mar 15, 2022 •

edited