From 4c18928a931c8dc149fa3e0bd7de8ce4f6242715 Mon Sep 17 00:00:00 2001 From: Christian Schmidt Date: Wed, 19 Jul 2023 09:02:30 +0200 Subject: [PATCH] Wrong count in response when removing favourite/reblog (#24365) Co-authored-by: Claire --- .../api/v1/statuses/favourites_controller.rb | 5 ++- .../api/v1/statuses/reblogs_controller.rb | 7 +++- app/javascript/mastodon/reducers/statuses.js | 21 ++++++++-- .../status_relationships_presenter.rb | 3 +- app/serializers/rest/status_serializer.rb | 38 +++++++++++++------ .../v1/statuses/reblogs_controller_spec.rb | 6 +++ .../api/v1/statuses/favourites_spec.rb | 12 ++++++ 7 files changed, 73 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/v1/statuses/favourites_controller.rb b/app/controllers/api/v1/statuses/favourites_controller.rb index 2e21ce6a0..f3428e3df 100644 --- a/app/controllers/api/v1/statuses/favourites_controller.rb +++ b/app/controllers/api/v1/statuses/favourites_controller.rb @@ -17,13 +17,16 @@ class Api::V1::Statuses::FavouritesController < Api::BaseController if fav @status = fav.status + count = [@status.favourites_count - 1, 0].max UnfavouriteWorker.perform_async(current_account.id, @status.id) else @status = Status.find(params[:status_id]) + count = @status.favourites_count authorize @status, :show? end - render json: @status, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_account.id, favourites_map: { @status.id => false }) + relationships = StatusRelationshipsPresenter.new([@status], current_account.id, favourites_map: { @status.id => false }, attributes_map: { @status.id => { favourites_count: count } }) + render json: @status, serializer: REST::StatusSerializer, relationships: relationships rescue Mastodon::NotPermittedError not_found end diff --git a/app/controllers/api/v1/statuses/reblogs_controller.rb b/app/controllers/api/v1/statuses/reblogs_controller.rb index e3769437b..3ca623117 100644 --- a/app/controllers/api/v1/statuses/reblogs_controller.rb +++ b/app/controllers/api/v1/statuses/reblogs_controller.rb @@ -24,15 +24,18 @@ class Api::V1::Statuses::ReblogsController < Api::BaseController if @status authorize @status, :unreblog? + @reblog = @status.reblog + count = [@reblog.reblogs_count - 1, 0].max @status.discard RemovalWorker.perform_async(@status.id) - @reblog = @status.reblog else @reblog = Status.find(params[:status_id]) + count = @reblog.reblogs_count authorize @reblog, :show? end - render json: @reblog, serializer: REST::StatusSerializer, relationships: StatusRelationshipsPresenter.new([@status], current_account.id, reblogs_map: { @reblog.id => false }) + relationships = StatusRelationshipsPresenter.new([@status], current_account.id, reblogs_map: { @reblog.id => false }, attributes_map: { @reblog.id => { reblogs_count: count } }) + render json: @reblog, serializer: REST::StatusSerializer, relationships: relationships rescue Mastodon::NotPermittedError not_found end diff --git a/app/javascript/mastodon/reducers/statuses.js b/app/javascript/mastodon/reducers/statuses.js index 3c3d3d711..683fe848f 100644 --- a/app/javascript/mastodon/reducers/statuses.js +++ b/app/javascript/mastodon/reducers/statuses.js @@ -5,11 +5,16 @@ import { normalizeStatusTranslation } from '../actions/importer/normalizer'; import { REBLOG_REQUEST, REBLOG_FAIL, + UNREBLOG_REQUEST, + UNREBLOG_FAIL, FAVOURITE_REQUEST, FAVOURITE_FAIL, - UNFAVOURITE_SUCCESS, + UNFAVOURITE_REQUEST, + UNFAVOURITE_FAIL, BOOKMARK_REQUEST, BOOKMARK_FAIL, + UNBOOKMARK_REQUEST, + UNBOOKMARK_FAIL, } from '../actions/interactions'; import { STATUS_MUTE_SUCCESS, @@ -72,18 +77,28 @@ export default function statuses(state = initialState, action) { return importStatuses(state, action.statuses); case FAVOURITE_REQUEST: return state.setIn([action.status.get('id'), 'favourited'], true); - case UNFAVOURITE_SUCCESS: - return state.updateIn([action.status.get('id'), 'favourites_count'], x => Math.max(0, x - 1)); case FAVOURITE_FAIL: return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'favourited'], false); + case UNFAVOURITE_REQUEST: + return state.setIn([action.status.get('id'), 'favourited'], false); + case UNFAVOURITE_FAIL: + return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'favourited'], true); case BOOKMARK_REQUEST: return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'bookmarked'], true); case BOOKMARK_FAIL: return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'bookmarked'], false); + case UNBOOKMARK_REQUEST: + return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'bookmarked'], false); + case UNBOOKMARK_FAIL: + return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'bookmarked'], true); case REBLOG_REQUEST: return state.setIn([action.status.get('id'), 'reblogged'], true); case REBLOG_FAIL: return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'reblogged'], false); + case UNREBLOG_REQUEST: + return state.setIn([action.status.get('id'), 'reblogged'], false); + case UNREBLOG_FAIL: + return state.get(action.status.get('id')) === undefined ? state : state.setIn([action.status.get('id'), 'reblogged'], true); case STATUS_MUTE_SUCCESS: return state.setIn([action.id, 'muted'], true); case STATUS_UNMUTE_SUCCESS: diff --git a/app/presenters/status_relationships_presenter.rb b/app/presenters/status_relationships_presenter.rb index 50d1fb31b..5d53040fb 100644 --- a/app/presenters/status_relationships_presenter.rb +++ b/app/presenters/status_relationships_presenter.rb @@ -4,7 +4,7 @@ class StatusRelationshipsPresenter PINNABLE_VISIBILITIES = %w(public unlisted private).freeze attr_reader :reblogs_map, :favourites_map, :mutes_map, :pins_map, - :bookmarks_map, :filters_map + :bookmarks_map, :filters_map, :attributes_map def initialize(statuses, current_account_id = nil, **options) if current_account_id.nil? @@ -26,6 +26,7 @@ class StatusRelationshipsPresenter @bookmarks_map = Status.bookmarks_map(status_ids, current_account_id).merge(options[:bookmarks_map] || {}) @mutes_map = Status.mutes_map(conversation_ids, current_account_id).merge(options[:mutes_map] || {}) @pins_map = Status.pins_map(pinnable_status_ids, current_account_id).merge(options[:pins_map] || {}) + @attributes_map = options[:attributes_map] || {} end end diff --git a/app/serializers/rest/status_serializer.rb b/app/serializers/rest/status_serializer.rb index e0b8f32a6..d32621541 100644 --- a/app/serializers/rest/status_serializer.rb +++ b/app/serializers/rest/status_serializer.rb @@ -81,49 +81,57 @@ class REST::StatusSerializer < ActiveModel::Serializer ActivityPub::TagManager.instance.url_for(object) end + def reblogs_count + relationships&.attributes_map&.dig(object.id, :reblogs_count) || object.reblogs_count + end + + def favourites_count + relationships&.attributes_map&.dig(object.id, :favourites_count) || object.favourites_count + end + def favourited - if instance_options && instance_options[:relationships] - instance_options[:relationships].favourites_map[object.id] || false + if relationships + relationships.favourites_map[object.id] || false else current_user.account.favourited?(object) end end def reblogged - if instance_options && instance_options[:relationships] - instance_options[:relationships].reblogs_map[object.id] || false + if relationships + relationships.reblogs_map[object.id] || false else current_user.account.reblogged?(object) end end def muted - if instance_options && instance_options[:relationships] - instance_options[:relationships].mutes_map[object.conversation_id] || false + if relationships + relationships.mutes_map[object.conversation_id] || false else current_user.account.muting_conversation?(object.conversation) end end def bookmarked - if instance_options && instance_options[:relationships] - instance_options[:relationships].bookmarks_map[object.id] || false + if relationships + relationships.bookmarks_map[object.id] || false else current_user.account.bookmarked?(object) end end def pinned - if instance_options && instance_options[:relationships] - instance_options[:relationships].pins_map[object.id] || false + if relationships + relationships.pins_map[object.id] || false else current_user.account.pinned?(object) end end def filtered - if instance_options && instance_options[:relationships] - instance_options[:relationships].filters_map[object.id] || [] + if relationships + relationships.filters_map[object.id] || [] else current_user.account.status_matches_filters(object) end @@ -144,6 +152,12 @@ class REST::StatusSerializer < ActiveModel::Serializer object.active_mentions.to_a.sort_by(&:id) end + private + + def relationships + instance_options && instance_options[:relationships] + end + class ApplicationSerializer < ActiveModel::Serializer attributes :name, :website diff --git a/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb b/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb index 6eac02b23..16ce95dc2 100644 --- a/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb +++ b/spec/controllers/api/v1/statuses/reblogs_controller_spec.rb @@ -10,6 +10,12 @@ describe Api::V1::Statuses::ReblogsController do let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'write:statuses', application: app) } context 'with an oauth token' do + around do |example| + Sidekiq::Testing.fake! do + example.run + end + end + before do allow(controller).to receive(:doorkeeper_token) { token } end diff --git a/spec/requests/api/v1/statuses/favourites_spec.rb b/spec/requests/api/v1/statuses/favourites_spec.rb index 021b8806e..ac5e86f29 100644 --- a/spec/requests/api/v1/statuses/favourites_spec.rb +++ b/spec/requests/api/v1/statuses/favourites_spec.rb @@ -77,6 +77,12 @@ RSpec.describe 'Favourites' do let(:status) { Fabricate(:status) } + around do |example| + Sidekiq::Testing.fake! do + example.run + end + end + it_behaves_like 'forbidden for wrong scope', 'read read:favourites' context 'with public status' do @@ -88,6 +94,9 @@ RSpec.describe 'Favourites' do subject expect(response).to have_http_status(200) + expect(user.account.favourited?(status)).to be true + + UnfavouriteWorker.drain expect(user.account.favourited?(status)).to be false end @@ -110,6 +119,9 @@ RSpec.describe 'Favourites' do subject expect(response).to have_http_status(200) + expect(user.account.favourited?(status)).to be true + + UnfavouriteWorker.drain expect(user.account.favourited?(status)).to be false end