FIX: Allow user to remove bookmark from posts as long as bookmark is present.

https://meta.discourse.org/t/bookmark-issue-when-access-to-topic-is-lost-pms/51993

app/controllers/posts_controller.rb

@@ -377,17 +377,19 @@ class PostsController < ApplicationController
   end
 
   def bookmark
-    post = find_post_from_params
-
     if params[:bookmarked] == "true"
+      post = find_post_from_params
       PostAction.act(current_user, post, PostActionType.types[:bookmark])
     else
+      post_action = PostAction.find_by(post_id: params[:post_id], user_id: current_user.id)
+      post = post_action.post
+      raise Discourse::InvalidParameters unless post_action
+
       PostAction.remove_act(current_user, post, PostActionType.types[:bookmark])
     end
 
-    tu = TopicUser.get(post.topic, current_user)
-
-    render_json_dump(topic_bookmarked: tu.try(:bookmarked))
+    topic_user = TopicUser.get(post.topic, current_user)
+    render_json_dump(topic_bookmarked: topic_user.try(:bookmarked))
   end
 
   def wiki

spec/controllers/posts_controller_spec.rb

@@ -426,7 +426,8 @@ describe PostsController do
     include_examples 'action requires login', :put, :bookmark, post_id: 2
 
     describe 'when logged in' do
-      let(:post) { Fabricate(:post, user: log_in) }
+      let(:user) { log_in }
+      let(:post) { Fabricate(:post, user: user) }
       let(:private_message) { Fabricate(:private_message_post) }
 
       it "raises an error if the user doesn't have permission to see the post" do
@@ -436,13 +437,39 @@ describe PostsController do
       end
 
       it 'creates a bookmark' do
-        PostAction.expects(:act).with(post.user, post, PostActionType.types[:bookmark])
         xhr :put, :bookmark, post_id: post.id, bookmarked: 'true'
+
+        post_action = PostAction.find_by(user:user, post: post)
+
+        expect(post_action.post_action_type_id).to eq(PostActionType.types[:bookmark])
       end
 
-      it 'removes a bookmark' do
-        PostAction.expects(:remove_act).with(post.user, post, PostActionType.types[:bookmark])
-        xhr :put, :bookmark, post_id: post.id
+      context "removing a bookmark" do
+        let(:post_action) { PostAction.act(user, post, PostActionType.types[:bookmark]) }
+        let(:admin) { Fabricate(:admin) }
+
+        it 'should be able to remove a bookmark' do
+          post_action
+          xhr :put, :bookmark, post_id: post.id
+
+          expect(PostAction.find_by(id: post_action.id)).to eq(nil)
+        end
+
+        describe "when user doesn't have permission to see bookmarked post" do
+          it "should still be able to remove a bookmark" do
+            post_action
+            post = post_action.post
+            topic = post.topic
+            topic.convert_to_private_message(admin)
+            topic.remove_allowed_user(admin, user.username)
+
+            expect(Guardian.new(user).can_see_post?(post.reload)).to eq(false)
+
+            xhr :put, :bookmark, post_id: post.id
+
+            expect(PostAction.find_by(id: post_action.id)).to eq(nil)
+          end
+        end
       end
 
     end