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.

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

Poll model #3036

Merged
merged 30 commits into from
Jan 26, 2023
Merged

Poll model #3036

merged 30 commits into from
Jan 26, 2023

Conversation

kerryarchibald
Copy link
Contributor

@kerryarchibald kerryarchibald commented Jan 9, 2023

  • Adds a Poll model
  • Creates and add polls to room state
  • Use relations API to aggregate poll responses

Next PR will address paging responses from /relations

POC react-sdk PR matrix-org/matrix-react-sdk#9877

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
src/models/room.ts Show resolved Hide resolved
src/models/relations.ts Show resolved Hide resolved
src/models/poll.ts Show resolved Hide resolved
src/models/poll.ts Show resolved Hide resolved
Comment on lines +171 to +173
if (event.getTs() > pollEndTimestamp) {
this.responses?.removeEvent(event);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For the read receipt code (defining what has been read or not) it usually prefers looking at the order in the DAG and as a last result will use the timestamp.

Could we take a similar approach here?
You have functions like compareEventOrdering in EventTimelineSet that could be good to use.

getEventReadUpTo is a good place to look at the logic in place that favours ordering first, and timestamp as a fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand why this is probably more correct, but since the polls spec specifically mentions the poll end event timestamp, and comparing events by origin_server_ts, I think it is best to stay with strict timestamp comparison.

Only a user's most recent vote (by origin_server_ts) is accepted, even if that event is invalid or redacted. Votes with timestamps after the poll has closed are ignored, as if they never happened.

The m.poll.end's origin_server_ts determines when the poll closes exactly: if no valid end event is received, the poll is still open. If the poll is closed, only votes sent on or before that timestamp are considered, even if those votes are from before the start event. This is to handle clock drift over federation as gracefully as possible.

https://github.com/matrix-org/matrix-spec-proposals/blob/travis/msc/polls/proposals/3381-polls.md

src/models/poll.ts Outdated Show resolved Hide resolved
src/models/poll.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Look like a good start 👍
A few comments that probably need to be looked into, the biggest one being how we compare events with each others

spec/unit/room.spec.ts Show resolved Hide resolved
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

I checked out all branches and tested polls locally. Looks good 👍

Some minor comments. Mainly about strict typing.

src/models/poll.ts Outdated Show resolved Hide resolved
src/models/poll.ts Outdated Show resolved Hide resolved
src/models/poll.ts Show resolved Hide resolved
src/models/poll.ts Show resolved Hide resolved
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks good to me ✅

this.processAggregatedTimelineEvents(room, events);
}

public processAggregatedTimelineEvents(room?: Room, events?: MatrixEvent[]): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a tsdoc comment to explain what this is supposed to do?

@kerryarchibald kerryarchibald enabled auto-merge (squash) January 26, 2023 01:53
@kerryarchibald kerryarchibald merged commit ef51ee2 into develop Jan 26, 2023
@kerryarchibald kerryarchibald deleted the psg-1014/poll-model branch January 26, 2023 02:07
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Feb 28, 2023
* Element-R: implement encryption of outgoing events ([\matrix-org#3122](matrix-org#3122)).
* Poll model - page /relations results ([\matrix-org#3073](matrix-org#3073)). Contributed by @kerryarchibald.
* Poll model - validate end events ([\matrix-org#3072](matrix-org#3072)). Contributed by @kerryarchibald.
* Handle optional last_known_event_id property in m.predecessor ([\matrix-org#3119](matrix-org#3119)). Contributed by @andybalaam.
* Add support for stable identifier for fixed MAC in SAS verification ([\matrix-org#3101](matrix-org#3101)).
* Provide eventId as well as roomId from Room.findPredecessor ([\matrix-org#3095](matrix-org#3095)). Contributed by @andybalaam.
* MSC3946 Dynamic room predecessors ([\matrix-org#3042](matrix-org#3042)). Contributed by @andybalaam.
* Poll model ([\matrix-org#3036](matrix-org#3036)). Contributed by @kerryarchibald.
* Remove video tracks on video mute without renegotiating ([\matrix-org#3091](matrix-org#3091)).
* Introduces a backwards-compatible API change. `MegolmEncrypter#prepareToEncrypt`'s return type has changed from `void` to `() => void`. ([\matrix-org#3035](matrix-org#3035)). Contributed by @clarkf.
* Stop the ICE disconnected timer on call terminate ([\matrix-org#3147](matrix-org#3147)).
* Clear notifications when we can infer read status from receipts ([\matrix-org#3139](matrix-org#3139)). Fixes element-hq/element-web#23991.
* Messages sent out of order after one message fails ([\matrix-org#3131](matrix-org#3131)). Fixes element-hq/element-web#22885 and element-hq/element-web#18942. Contributed by @justjanne.
* Element-R: fix a bug which prevented encryption working after a reload ([\matrix-org#3126](matrix-org#3126)).
* Element-R: Fix invite processing ([\matrix-org#3121](matrix-org#3121)).
* Don't throw with no `opponentDeviceInfo` ([\matrix-org#3107](matrix-org#3107)).
* Remove flaky megolm test ([\matrix-org#3098](matrix-org#3098)). Contributed by @clarkf.
* Fix "verifyLinks" functionality of getRoomUpgradeHistory ([\matrix-org#3089](matrix-org#3089)). Contributed by @andybalaam.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants