From 09c042aa10f2be4824fcb8e96c8a8ae6d324d12f Mon Sep 17 00:00:00 2001 From: ThibG Date: Thu, 7 Mar 2019 15:52:38 +0100 Subject: [PATCH 1/5] Handle StaleObjectError when retrieving polls (#10208) --- .../activitypub/fetch_remote_poll_service.rb | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/app/services/activitypub/fetch_remote_poll_service.rb b/app/services/activitypub/fetch_remote_poll_service.rb index 1dd587d73a0..4f9814fcd7e 100644 --- a/app/services/activitypub/fetch_remote_poll_service.rb +++ b/app/services/activitypub/fetch_remote_poll_service.rb @@ -32,12 +32,17 @@ class ActivityPub::FetchRemotePollService < BaseService # votes, so we need to remove them poll.votes.delete_all if latest_options != poll.options - poll.update!( - last_fetched_at: Time.now.utc, - expires_at: expires_at, - options: latest_options, - cached_tallies: items.map { |item| item.dig('replies', 'totalItems') || 0 } - ) + begin + poll.update!( + last_fetched_at: Time.now.utc, + expires_at: expires_at, + options: latest_options, + cached_tallies: items.map { |item| item.dig('replies', 'totalItems') || 0 } + ) + rescue ActiveRecord::StaleObjectError + poll.reload + retry + end end private From be1c634b2b4372a525d304d2ff830392f04c5cc5 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 7 Mar 2019 22:17:52 +0100 Subject: [PATCH 2/5] Fix public timelines being broken by new toots when they are not mounted (#10131) --- app/javascript/mastodon/actions/compose.js | 4 +++- app/javascript/mastodon/actions/streaming.js | 6 ++++++ app/javascript/mastodon/actions/timelines.js | 8 ++++++++ app/javascript/mastodon/reducers/timelines.js | 9 +++++---- app/javascript/mastodon/stream.js | 8 ++++++-- 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/app/javascript/mastodon/actions/compose.js b/app/javascript/mastodon/actions/compose.js index da7bd3cb0c7..d65d4104826 100644 --- a/app/javascript/mastodon/actions/compose.js +++ b/app/javascript/mastodon/actions/compose.js @@ -158,7 +158,9 @@ export function submitCompose(routerHistory) { // into the columns const insertIfOnline = timelineId => { - if (getState().getIn(['timelines', timelineId, 'items', 0]) !== null) { + const timeline = getState().getIn(['timelines', timelineId]); + + if (timeline && timeline.get('items').size > 0 && timeline.getIn(['items', 0]) !== null && timeline.get('online')) { dispatch(updateTimeline(timelineId, { ...response.data })); } }; diff --git a/app/javascript/mastodon/actions/streaming.js b/app/javascript/mastodon/actions/streaming.js index cd319709d82..c678e939320 100644 --- a/app/javascript/mastodon/actions/streaming.js +++ b/app/javascript/mastodon/actions/streaming.js @@ -3,6 +3,7 @@ import { updateTimeline, deleteFromTimelines, expandHomeTimeline, + connectTimeline, disconnectTimeline, } from './timelines'; import { updateNotifications, expandNotifications } from './notifications'; @@ -16,7 +17,12 @@ export function connectTimelineStream (timelineId, path, pollingRefresh = null, return connectStream (path, pollingRefresh, (dispatch, getState) => { const locale = getState().getIn(['meta', 'locale']); + return { + onConnect() { + dispatch(connectTimeline(timelineId)); + }, + onDisconnect() { dispatch(disconnectTimeline(timelineId)); }, diff --git a/app/javascript/mastodon/actions/timelines.js b/app/javascript/mastodon/actions/timelines.js index 6e7bd027cb7..d92385e9515 100644 --- a/app/javascript/mastodon/actions/timelines.js +++ b/app/javascript/mastodon/actions/timelines.js @@ -12,6 +12,7 @@ export const TIMELINE_EXPAND_FAIL = 'TIMELINE_EXPAND_FAIL'; export const TIMELINE_SCROLL_TOP = 'TIMELINE_SCROLL_TOP'; +export const TIMELINE_CONNECT = 'TIMELINE_CONNECT'; export const TIMELINE_DISCONNECT = 'TIMELINE_DISCONNECT'; export function updateTimeline(timeline, status, accept) { @@ -143,6 +144,13 @@ export function scrollTopTimeline(timeline, top) { }; }; +export function connectTimeline(timeline) { + return { + type: TIMELINE_CONNECT, + timeline, + }; +}; + export function disconnectTimeline(timeline) { return { type: TIMELINE_DISCONNECT, diff --git a/app/javascript/mastodon/reducers/timelines.js b/app/javascript/mastodon/reducers/timelines.js index 38af9cd094f..94b570ecdc7 100644 --- a/app/javascript/mastodon/reducers/timelines.js +++ b/app/javascript/mastodon/reducers/timelines.js @@ -6,6 +6,7 @@ import { TIMELINE_EXPAND_REQUEST, TIMELINE_EXPAND_FAIL, TIMELINE_SCROLL_TOP, + TIMELINE_CONNECT, TIMELINE_DISCONNECT, } from '../actions/timelines'; import { @@ -20,6 +21,7 @@ const initialState = ImmutableMap(); const initialTimeline = ImmutableMap({ unread: 0, + online: false, top: true, isLoading: false, hasMore: true, @@ -142,14 +144,13 @@ export default function timelines(state = initialState, action) { return filterTimeline('home', state, action.relationship, action.statuses); case TIMELINE_SCROLL_TOP: return updateTop(state, action.timeline, action.top); + case TIMELINE_CONNECT: + return state.update(action.timeline, initialTimeline, map => map.set('online', true)); case TIMELINE_DISCONNECT: return state.update( action.timeline, initialTimeline, - map => map.update( - 'items', - items => items.first() ? items.unshift(null) : items - ) + map => map.set('online', false).update('items', items => items.first() ? items.unshift(null) : items) ); default: return state; diff --git a/app/javascript/mastodon/stream.js b/app/javascript/mastodon/stream.js index 9928d0dd768..306a068b7a0 100644 --- a/app/javascript/mastodon/stream.js +++ b/app/javascript/mastodon/stream.js @@ -2,11 +2,11 @@ import WebSocketClient from 'websocket.js'; const randomIntUpTo = max => Math.floor(Math.random() * Math.floor(max)); -export function connectStream(path, pollingRefresh = null, callbacks = () => ({ onDisconnect() {}, onReceive() {} })) { +export function connectStream(path, pollingRefresh = null, callbacks = () => ({ onConnect() {}, onDisconnect() {}, onReceive() {} })) { return (dispatch, getState) => { const streamingAPIBaseURL = getState().getIn(['meta', 'streaming_api_base_url']); const accessToken = getState().getIn(['meta', 'access_token']); - const { onDisconnect, onReceive } = callbacks(dispatch, getState); + const { onConnect, onDisconnect, onReceive } = callbacks(dispatch, getState); let polling = null; @@ -28,6 +28,8 @@ export function connectStream(path, pollingRefresh = null, callbacks = () => ({ if (pollingRefresh) { clearPolling(); } + + onConnect(); }, disconnected () { @@ -47,6 +49,8 @@ export function connectStream(path, pollingRefresh = null, callbacks = () => ({ clearPolling(); pollingRefresh(dispatch); } + + onConnect(); }, }); From 75cb93676b1dd41d3e47f62466c0c6430691a990 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 7 Mar 2019 22:18:05 +0100 Subject: [PATCH 3/5] Fix NaN in Poll component (#10213) --- app/javascript/mastodon/components/poll.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/javascript/mastodon/components/poll.js b/app/javascript/mastodon/components/poll.js index bfff7b60116..a1b297ce750 100644 --- a/app/javascript/mastodon/components/poll.js +++ b/app/javascript/mastodon/components/poll.js @@ -94,7 +94,7 @@ class Poll extends ImmutablePureComponent { renderOption (option, optionIndex) { const { poll, disabled } = this.props; - const percent = (option.get('votes_count') / poll.get('votes_count')) * 100; + const percent = poll.get('votes_count') === 0 ? 0 : (option.get('votes_count') / poll.get('votes_count')) * 100; const leading = poll.get('options').filterNot(other => other.get('title') === option.get('title')).every(other => option.get('votes_count') > other.get('votes_count')); const active = !!this.state.selected[`${optionIndex}`]; const showResults = poll.get('voted') || poll.get('expired'); From 054bbb3da21b2c76374eb921cba862adb8d5a0b3 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 7 Mar 2019 22:53:47 +0100 Subject: [PATCH 4/5] Immediately display poll results to poll author (#10187) * Immediately display poll results to poll author * Refactor Poll#loaded_options and add Poll#voted? to improve DRYness --- app/models/poll.rb | 14 +++++++++----- app/serializers/activitypub/note_serializer.rb | 6 +----- app/serializers/rest/poll_serializer.rb | 12 ++---------- app/views/stream_entries/_poll.html.haml | 6 ++---- 4 files changed, 14 insertions(+), 24 deletions(-) diff --git a/app/models/poll.rb b/app/models/poll.rb index da2e25e7152..14a38026a03 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -41,17 +41,17 @@ class Poll < ApplicationRecord after_commit :reset_parent_cache, on: :update def loaded_options - options.map.with_index { |title, key| Option.new(self, key.to_s, title, cached_tallies[key]) } - end - - def unloaded_options - options.map.with_index { |title, key| Option.new(self, key.to_s, title, nil) } + options.map.with_index { |title, key| Option.new(self, key.to_s, title, show_totals_now? ? cached_tallies[key] : nil) } end def possibly_stale? remote? && last_fetched_before_expiration? && time_passed_since_last_fetch? end + def voted?(account) + account.id == account_id || votes.where(account: account).exists? + end + delegate :local?, to: :account def remote? @@ -95,4 +95,8 @@ class Poll < ApplicationRecord def time_passed_since_last_fetch? last_fetched_at.nil? || last_fetched_at < 1.minute.ago end + + def show_totals_now? + expired? || !hide_totals? + end end diff --git a/app/serializers/activitypub/note_serializer.rb b/app/serializers/activitypub/note_serializer.rb index 3a9e388a5f3..553f333d8ff 100644 --- a/app/serializers/activitypub/note_serializer.rb +++ b/app/serializers/activitypub/note_serializer.rb @@ -122,11 +122,7 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer end def poll_options - if !object.poll.expired? && object.poll.hide_totals? - object.poll.unloaded_options - else - object.poll.loaded_options - end + object.poll.loaded_options end def poll_and_multiple? diff --git a/app/serializers/rest/poll_serializer.rb b/app/serializers/rest/poll_serializer.rb index b02e8ca9344..4dae1c09f43 100644 --- a/app/serializers/rest/poll_serializer.rb +++ b/app/serializers/rest/poll_serializer.rb @@ -4,7 +4,7 @@ class REST::PollSerializer < ActiveModel::Serializer attributes :id, :expires_at, :expired, :multiple, :votes_count - has_many :dynamic_options, key: :options + has_many :loaded_options, key: :options attribute :voted, if: :current_user? @@ -12,20 +12,12 @@ class REST::PollSerializer < ActiveModel::Serializer object.id.to_s end - def dynamic_options - if !object.expired? && object.hide_totals? - object.unloaded_options - else - object.loaded_options - end - end - def expired object.expired? end def voted - object.votes.where(account: current_user.account).exists? + object.voted?(current_user.account) end def current_user? diff --git a/app/views/stream_entries/_poll.html.haml b/app/views/stream_entries/_poll.html.haml index dad04b79c82..d6b2c0cd918 100644 --- a/app/views/stream_entries/_poll.html.haml +++ b/app/views/stream_entries/_poll.html.haml @@ -1,10 +1,8 @@ -- options = (!poll.expired? && poll.hide_totals?) ? poll.unloaded_options : poll.loaded_options -- voted = user_signed_in? && poll.votes.where(account: current_account).exists? -- show_results = voted || poll.expired? +- show_results = (user_signed_in? && poll.voted?(current_account)) || poll.expired? .poll %ul - - options.each do |option| + - poll.loaded_options.each do |option| %li - if show_results - percent = poll.votes_count > 0 ? 100 * option.votes_count / poll.votes_count : 0 From 3aaac4f134eb092baeb0ba5979bdb3abd702a4ee Mon Sep 17 00:00:00 2001 From: ThibG Date: Fri, 8 Mar 2019 00:54:50 +0100 Subject: [PATCH 5/5] Do not allow adding votes to expired polls (#10214) * Do not allow adding votes to expired polls * Only validate expires_at on create --- app/lib/activitypub/activity/create.rb | 1 + app/models/poll.rb | 2 +- app/services/vote_service.rb | 2 ++ spec/lib/activitypub/activity/create_spec.rb | 22 ++++++++++++++++++++ 4 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 87179030c26..7e4e57eadda 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -241,6 +241,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity def poll_vote? return false if replied_to_status.nil? || replied_to_status.poll.nil? || !replied_to_status.local? || !replied_to_status.poll.options.include?(@object['name']) + return true if replied_to_status.poll.expired? replied_to_status.poll.votes.create!(account: @account, choice: replied_to_status.poll.options.index(@object['name']), uri: @object['id']) end diff --git a/app/models/poll.rb b/app/models/poll.rb index 14a38026a03..09f0b65ec4a 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -28,7 +28,7 @@ class Poll < ApplicationRecord validates :options, presence: true validates :expires_at, presence: true, if: :local? - validates_with PollValidator, if: :local? + validates_with PollValidator, on: :create, if: :local? scope :attached, -> { where.not(status_id: nil) } scope :unattached, -> { where(status_id: nil) } diff --git a/app/services/vote_service.rb b/app/services/vote_service.rb index 8bab2810ea1..5b80da03a37 100644 --- a/app/services/vote_service.rb +++ b/app/services/vote_service.rb @@ -11,6 +11,8 @@ class VoteService < BaseService @choices = choices @votes = [] + return if @poll.expired? + ApplicationRecord.transaction do @choices.each do |choice| @votes << @poll.votes.create!(account: @account, choice: choice) diff --git a/spec/lib/activitypub/activity/create_spec.rb b/spec/lib/activitypub/activity/create_spec.rb index 56c7bfc6150..3a1463d958f 100644 --- a/spec/lib/activitypub/activity/create_spec.rb +++ b/spec/lib/activitypub/activity/create_spec.rb @@ -482,6 +482,28 @@ RSpec.describe ActivityPub::Activity::Create do expect(poll.reload.cached_tallies).to eq [1, 0] end end + + context 'when a vote to an expired local poll' do + let(:poll) do + poll = Fabricate.build(:poll, options: %w(Yellow Blue), expires_at: 1.day.ago) + poll.save(validate: false) + poll + end + let!(:local_status) { Fabricate(:status, owned_poll: poll) } + + let(:object_json) do + { + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, + type: 'Note', + name: 'Yellow', + inReplyTo: ActivityPub::TagManager.instance.uri_for(local_status) + } + end + + it 'does not add a vote to the poll' do + expect(poll.votes.first).to be_nil + end + end end context 'when sender is followed by local users' do