FIX: bookmark topic was not working intuitively

- explicitly call out "clear bookmarks"
- correct keyboard shortcuts
- properly remove bookmarks when toggeling

app/assets/javascripts/discourse/lib/keyboard_shortcuts.js

@@ -17,7 +17,6 @@ var PATH_BINDINGS = {
     },
 
     CLICK_BINDINGS = {
-      'f': '#topic-footer-buttons button.bookmark',                             // bookmark topic
       'm m': 'div.notification-options li[data-id="0"] a',                      // mark topic as muted
       'm r': 'div.notification-options li[data-id="1"] a',                      // mark topic as regular
       'm t': 'div.notification-options li[data-id="2"] a',                      // mark topic as tracking
@@ -50,7 +49,8 @@ var PATH_BINDINGS = {
       'command+f': 'showBuiltinSearch',
       '?': 'showHelpModal',                                                     // open keyboard shortcut help
       'q': 'quoteReply',
-      'b': 'toggleBookmark'
+      'b': 'toggleBookmark',
+      'f': 'toggleBookmarkTopic'
     };
 
 
@@ -70,6 +70,16 @@ Discourse.KeyboardShortcuts = Ember.Object.createWithMixins({
     this.sendToTopicListItemView('toggleBookmark');
   },
 
+  toggleBookmarkTopic: function(){
+    var topic = this.currentTopic();
+    // BIG hack, need a cleaner way
+    if(topic && $('.posts-wrapper').length > 0) {
+      topic.toggleBookmark();
+    } else {
+      this.sendToTopicListItemView('toggleBookmark');
+    }
+  },
+
   quoteReply: function(){
     $('.topic-post.selected button.create').click();
     // lazy but should work for now
@@ -170,6 +180,16 @@ Discourse.KeyboardShortcuts = Ember.Object.createWithMixins({
     }
   },
 
+  currentTopic: function(){
+    var topicController = this.container.lookup('controller:topic');
+    if(topicController) {
+      var topic = topicController.get('model');
+      if(topic){
+        return topic;
+      }
+    }
+  },
+
   sendToSelectedPost: function(action){
     var container = this.container;
     // TODO: We should keep track of the post without a CSS class

app/assets/javascripts/discourse/models/_post.js

@@ -413,15 +413,27 @@ Discourse.Post = Discourse.Model.extend({
   },
 
   toggleBookmark: function() {
-    var self = this;
+    var self = this,
+        bookmarkedTopic;
 
     this.toggleProperty("bookmarked");
-    if (this.get("post_number") === 1) { this.toggleProperty("topic.bookmarked"); }
 
-    return Discourse.Post.updateBookmark(this.get('id'), this.get('bookmarked')).catch(function() {
+    if(this.get("bookmarked") && !this.get("topic.bookmarked")) {
-      self.toggleProperty("bookmarked");
+      this.set("topic.bookmarked", true);
-      if (this.get("post_number") === 1) { this.toggleProperty("topic.bookmarked"); }
+      bookmarkedTopic = true;
-    });
+    }
+
+    // need to wait to hear back from server (stuff may not be loaded)
+
+    return Discourse.Post.updateBookmark(this.get('id'), this.get('bookmarked'))
+      .then(function(result){
+        self.set("topic.bookmarked", result.topic_bookmarked);
+      })
+      .catch(function(e) {
+        self.toggleProperty("bookmarked");
+        if (bookmarkedTopic) {self.set("topic.bookmarked", false); }
+        throw e;
+      });
   }
 });
 

app/assets/javascripts/discourse/models/topic.js.es6

@@ -183,18 +183,48 @@ const Topic = Discourse.Model.extend({
   }.property('word_count'),
 
   toggleBookmark() {
-    const self = this, firstPost = this.get("postStream.posts")[0];
+    const self = this,
+          stream = this.get('postStream'),
+          posts = Em.get(stream, 'posts'),
+          firstPost = posts &&
+                      posts[0] &&
+                      posts[0].get('post_number') === 1 &&
+                      posts[0],
+          bookmark = !self.get('bookmarked');
+
+    var path = bookmark ? '/bookmark' : '/remove_bookmarks';
+    var unbookmarkedPosts = [],
+        bookmarkedPost;
 
     this.toggleProperty('bookmarked');
-    if (this.get("postStream.firstPostPresent")) { firstPost.toggleProperty("bookmarked"); }
 
+    if (bookmark && firstPost) {
+      firstPost.set('bookmarked', true);
+      bookmarkedPost = firstPost;
+    }
 
-    return Discourse.ajax('/t/' + this.get('id') + '/bookmark', {
+    if (!bookmark && posts) {
+      posts.forEach(function(post){
+          if(post.get('bookmarked')){
+            post.set('bookmarked', false);
+            unbookmarkedPosts.push(post);
+          }
+      });
+    }
+
+    return Discourse.ajax('/t/' + this.get('id') + path, {
       type: 'PUT',
-      data: { bookmarked: self.get('bookmarked') },
     }).catch(function(error) {
+
       self.toggleProperty('bookmarked');
-      if (self.get("postStream.firstPostPresent")) { firstPost.toggleProperty('bookmarked'); }
+
+      if(bookmarkedPost) {
+        bookmarkedPost.set('bookmarked', false);
+      }
+
+      unbookmarkedPosts.forEach(function(p){
+        p.set('bookmarked', true);
+      });
 
       let showGenericError = true;
       if (error && error.responseText) {
@@ -207,6 +237,8 @@ const Topic = Discourse.Model.extend({
       if(showGenericError){
         bootbox.alert(I18n.t('generic_error'));
       }
+
+      throw error;
     });
   },
 

app/assets/javascripts/discourse/views/bookmark-button.js.es6

@@ -2,9 +2,12 @@ import ButtonView from 'discourse/views/button';
 
 export default ButtonView.extend({
   classNames: ['bookmark'],
-  textKey: 'bookmarked.title',
   attributeBindings: ['disabled'],
 
+  textKey: function() {
+    return this.get('controller.bookmarked') ? 'bookmarked.clear_bookmarks' : 'bookmarked.title';
+  }.property('controller.bookmarked'),
+
   rerenderTriggers: ['controller.bookmarked'],
 
   helpKey: function() {

app/controllers/posts_controller.rb

@@ -285,7 +285,9 @@ class PostsController < ApplicationController
       PostAction.remove_act(current_user, post, PostActionType.types[:bookmark])
     end
 
-    render nothing: true
+    tu = TopicUser.get(post.topic, current_user)
+
+    render_json_dump(topic_bookmarked: tu.bookmarked)
   end
 
   def wiki

app/controllers/topics_controller.rb

@@ -217,15 +217,24 @@ class TopicsController < ApplicationController
     render nothing: true
   end
 
+  def remove_bookmarks
+    topic = Topic.find(params[:topic_id].to_i)
+
+    PostAction.joins(:post)
+              .where(user_id: current_user.id)
+              .where('topic_id = ?', topic.id).each do |pa|
+
+      PostAction.remove_act(current_user, pa.post, PostActionType.types[:bookmark])
+    end
+
+    render nothing: true
+  end
+
   def bookmark
-    topic = Topic.find_by(id: params[:topic_id])
+    topic = Topic.find(params[:topic_id].to_i)
     first_post = topic.ordered_posts.first
 
-    if params[:bookmarked] == "true"
+    PostAction.act(current_user, first_post, PostActionType.types[:bookmark])
-      PostAction.act(current_user, first_post, PostActionType.types[:bookmark])
-    else
-      PostAction.remove_act(current_user, first_post, PostActionType.types[:bookmark])
-    end
 
     render nothing: true
   end

config/locales/client.en.yml

@@ -167,9 +167,10 @@ en:
 
     bookmarked:
       title: "Bookmark"
+      clear_bookmarks: "Clear Bookmarks"
       help:
-        bookmark: "Click to bookmark this topic"
+        bookmark: "Click to bookmark the first post on this topic"
-        unbookmark: "Click to remove bookmark to this topic"
+        unbookmark: "Click to remove all bookmarks in this topic"
 
     bookmarks:
       not_logged_in: "sorry, you must be logged in to bookmark posts"
@@ -2287,7 +2288,7 @@ en:
         dismiss_topics: '<b>x</b>, <b>t</b> Dismiss Topics'
       actions:
         title: 'Actions'
-        bookmark_topic: '<b>f</b> Bookmark topic'
+        bookmark_topic: '<b>f</b> Toggle bookmark topic'
         pin_unpin_topic: '<b>shift</b>+<b>p</b> Pin/Unpin topic'
         share_topic: '<b>shift</b>+<b>s</b> Share topic'
         share_post: '<b>s</b> Share post'

config/routes.rb

@@ -426,6 +426,7 @@ Discourse::Application.routes.draw do
   post "t/:topic_id/change-owner" => "topics#change_post_owners", constraints: {topic_id: /\d+/}
   delete "t/:topic_id/timings" => "topics#destroy_timings", constraints: {topic_id: /\d+/}
   put "t/:topic_id/bookmark" => "topics#bookmark", constraints: {topic_id: /\d+/}
+  put "t/:topic_id/remove_bookmarks" => "topics#remove_bookmarks", constraints: {topic_id: /\d+/}
 
   post "t/:topic_id/notifications" => "topics#set_notifications" , constraints: {topic_id: /\d+/}
 

spec/controllers/topics_controller_spec.rb

@@ -790,7 +790,7 @@ describe TopicsController do
       it "works correctly" do
         group = Fabricate(:group)
         topic = Fabricate(:topic)
-        admin = log_in(:admin)
+        _admin = log_in(:admin)
 
         xhr :post, :invite, topic_id: topic.id, email: 'hiro@from.heros', group_ids: "#{group.id}"
 
@@ -857,7 +857,7 @@ describe TopicsController do
     end
 
     it 'needs you to be an admin or mod' do
-      user = log_in
+      log_in
       xhr :put, :autoclose, topic_id: 99, auto_close_time: '24', auto_close_based_on_last_post: false
       expect(response).to be_forbidden
     end
@@ -959,6 +959,25 @@ describe TopicsController do
     end
   end
 
+  describe 'remove_bookmarks' do
+    it "should remove bookmarks properly from non first post" do
+      bookmark = PostActionType.types[:bookmark]
+      user = log_in
+
+      post = create_post
+      post2 = create_post(topic_id: post.topic_id)
+
+      PostAction.act(user, post2, bookmark)
+
+      xhr :put, :bookmark, topic_id: post.topic_id
+      PostAction.where(user_id: user.id, post_action_type: bookmark).count.should == 2
+
+      xhr :put, :remove_bookmarks, topic_id: post.topic_id
+      PostAction.where(user_id: user.id, post_action_type: bookmark).count.should == 0
+
+    end
+  end
+
 
   describe 'reset_new' do
     it 'needs you to be logged in' do