-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Add polls #10111
Add polls #10111
Conversation
863eb2a
to
6e74d83
Compare
How do we see this from the ActivityPub? There is a version of friends.nico as an existing implementation. |
In my understanding, when a poll is attached to a status, the status should have type
Represented here by |
8e99cfb
to
b0d0db6
Compare
b0d0db6
to
c24b774
Compare
a7d0e9f
to
63a1a44
Compare
16ac9ae
to
4ffe700
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like this PR, but I have some concerns. Mainly, we make a few arbitrary choices that may not work well with other implementations:
- as far as I can see, we don't allow for changing votes, that is, neither deleting existing votes, nor overwriting existing ones. If other implementations allow this, we don't have a correct error path to notify them that their vote is invalid. This may lead to inconsistent user experience.
- polls and media being mutually exclusive kind of makes sense with the current UI, but other software might have different interfaces and not have that limit
- the number of replies etc. are quite arbitrary and might also be an issue
An interesting thing is that polls being tied to a Note
, they are only visible to the audience of that Note
. However, the voting code does not seem to enforce that only the audience can vote. It might be worth tightening those restrictions.
Finally, we could send and support Update
of the poll/its collections to update the tallies without the need for fetching changes.
poll.update!( | ||
expires_at: expires_at, | ||
cached_tallies: items.map { |item| item.dig('replies', 'totalItems') || 0 } | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark as earlier, what about fetching collections that aren't inlined?
Furthermore, if the poll has changed since it was initially fetched, this code will produce inconsistent tallies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, if the poll has changed since it was initially fetched, this code will produce inconsistent tallies.
I don't understand what you mean. This is fetched from the origin, for remote polls. The origin has the most correct count of votes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant that if the list of options changed (which nothing in the specs forbids), the updated tallies are going to be garbage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think updating the options of published polls should be allowed. That invalidates the results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could reorder them or add requested options, I don't know. I think we should at least update tallies based on name
, not on the option's position in the serialization.
I'm also thinking that, in the case we can't manage getting tallies from the remote server (because there is nothing standardized for that and the suggested solution is the Also, aren't we missing code to display the poll on public pages? |
@@ -250,6 +255,8 @@ def decrement_count!(key) | |||
before_validation :set_conversation | |||
before_validation :set_local | |||
|
|||
before_save :set_poll_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use a normal association instead of this, now, can't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this throwaway inverse reference to polls so that during eager-loading we can skip queries on the polls table when no statuses have polls, which I believe would be the most common case.
@@ -0,0 +1,5 @@ | |||
class AddPollIdToStatuses < ActiveRecord::Migration[5.2] | |||
def change | |||
add_column :statuses, :poll_id, :bigint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes here, we can use a foreign key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think foreign keys are an increasingly bad idea. It requires having an index on that column, otherwise DELETE or NULLIFY cascades take an incredible amount of time. Any index on statuses would also be massive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think poll_vote?
is still very confusing, fallback for remote implementations not using our replies
scheme should be considered, limits could be tweaked, and we could investigate sending Update
s and fetching the size of replies
collections identified by an URI instead of being inlined, but that's all stuff that can be done later.
Do you have a design for the creation of polls in the web UI, @Gargron ? Want me to create some mocks? |
@mattcoxonline I do not have a design. You can give it a go... |
* Add polls Fix mastodon#1629 * Add tests * Fixes * Change API for creating polls * Use name instead of content for votes * Remove poll validation for remote polls * Add polls to public pages * When updating the poll, update options just in case they were changed * Fix public pages showing both poll and other media
* Add polls Fix mastodon#1629 * Add tests * Fixes * Change API for creating polls * Use name instead of content for votes * Remove poll validation for remote polls * Add polls to public pages * When updating the poll, update options just in case they were changed * Fix public pages showing both poll and other media
Fix #1629
TODO:
TODO in separate PRs: