Wrong count in response when removing favourite/reblog (mastodon#24365)

Co-authored-by: Claire <[email protected]>

app/controllers/api/v1/statuses/favourites_controller.rb

@@ -17,13 +17,16 @@ def destroy
 
     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

app/controllers/api/v1/statuses/reblogs_controller.rb

@@ -24,15 +24,18 @@ def destroy
 
     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

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:

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 @@ def initialize(statuses, current_account_id = nil, **options)
       @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
 

app/serializers/rest/status_serializer.rb

@@ -81,49 +81,57 @@ def url
     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 @@ def ordered_mentions
     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
 

spec/controllers/api/v1/statuses/reblogs_controller_spec.rb

@@ -10,6 +10,12 @@
   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

spec/requests/api/v1/statuses/favourites_spec.rb

@@ -77,6 +77,12 @@
 
     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 @@
         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 @@
         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