From 4a872823e75fe54f752eca12b14a35108b2e871c Mon Sep 17 00:00:00 2001
From: Penar Musaraj <pmusaraj@gmail.com>
Date: Wed, 1 Aug 2018 17:41:27 -0400
Subject: [PATCH] Improvements to user drafts (#6226)

* drafts in user profile: only show to user herself (not to admins), use avatar replying to (instead of topic OP), add keyboard shortcut for drafts, simplify display labels

* use JSON when testing Draft.stream
---
 .../discourse/components/user-stream.js.es6   |  2 +-
 .../discourse/controllers/user.js.es6         |  6 ++--
 .../discourse/lib/keyboard-shortcuts.js.es6   |  1 +
 .../discourse/models/user-draft.js.es6        |  8 ++----
 .../modal/keyboard-shortcuts-help.hbs         |  1 +
 app/controllers/drafts_controller.rb          | 28 +++++++++----------
 app/models/draft.rb                           |  4 ++-
 config/locales/client.en.yml                  |  6 ++--
 config/locales/server.en.yml                  |  2 +-
 lib/guardian/user_guardian.rb                 |  4 ---
 spec/components/guardian_spec.rb              | 18 ------------
 spec/models/draft_spec.rb                     |  8 +++---
 12 files changed, 34 insertions(+), 54 deletions(-)

diff --git a/app/assets/javascripts/discourse/components/user-stream.js.es6 b/app/assets/javascripts/discourse/components/user-stream.js.es6
index 7bd7ff01439..f28c2968494 100644
--- a/app/assets/javascripts/discourse/components/user-stream.js.es6
+++ b/app/assets/javascripts/discourse/components/user-stream.js.es6
@@ -89,7 +89,7 @@ export default Ember.Component.extend(LoadMore, {
       const stream = this.get("stream");
       Draft.clear(draft.draft_key, draft.sequence)
         .then(() => {
-          stream.load(this.site);
+          stream.remove(draft);
         })
         .catch(error => {
           popupAjaxError(error);
diff --git a/app/assets/javascripts/discourse/controllers/user.js.es6 b/app/assets/javascripts/discourse/controllers/user.js.es6
index 06518682d28..96b2a333f23 100644
--- a/app/assets/javascripts/discourse/controllers/user.js.es6
+++ b/app/assets/javascripts/discourse/controllers/user.js.es6
@@ -62,9 +62,9 @@ export default Ember.Controller.extend(CanCheckEmails, {
     return viewingSelf || isAdmin;
   },
 
-  @computed("viewingSelf", "currentUser.admin")
-  showDrafts(viewingSelf, isAdmin) {
-    return viewingSelf || isAdmin;
+  @computed("viewingSelf")
+  showDrafts(viewingSelf) {
+    return viewingSelf;
   },
 
   @computed("viewingSelf", "currentUser.admin")
diff --git a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6
index d9bced4bd41..5e3907da2c9 100644
--- a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6
+++ b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6
@@ -31,6 +31,7 @@ const bindings = {
   "g b": { path: "/bookmarks" },
   "g p": { path: "/my/activity" },
   "g m": { path: "/my/messages" },
+  "g d": { path: "/my/activity/drafts" },
   home: { handler: "goToFirstPost", anonymous: true },
   "command+up": { handler: "goToFirstPost", anonymous: true },
   j: { handler: "selectDown", anonymous: true },
diff --git a/app/assets/javascripts/discourse/models/user-draft.js.es6 b/app/assets/javascripts/discourse/models/user-draft.js.es6
index 2f8d431eb73..5004457c773 100644
--- a/app/assets/javascripts/discourse/models/user-draft.js.es6
+++ b/app/assets/javascripts/discourse/models/user-draft.js.es6
@@ -31,17 +31,15 @@ export default RestModel.extend({
     );
   },
 
-  @computed("draft_key", "post_number")
-  draftType(draftKey, postNumber) {
+  @computed("draft_key")
+  draftType(draftKey) {
     switch (draftKey) {
       case NEW_TOPIC_KEY:
         return I18n.t("drafts.new_topic");
       case NEW_PRIVATE_MESSAGE_KEY:
         return I18n.t("drafts.new_private_message");
       default:
-        return postNumber
-          ? I18n.t("drafts.post_reply", { postNumber })
-          : I18n.t("drafts.topic_reply");
+        return false;
     }
   }
 });
diff --git a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs
index a7b6c0705b8..9d5ae70e07c 100644
--- a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs
+++ b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs
@@ -14,6 +14,7 @@
         {{#if siteSettings.enable_personal_messages}}
           <li>{{{i18n 'keyboard_shortcuts_help.jump_to.messages'}}}</li>
         {{/if}}
+        <li>{{{i18n 'keyboard_shortcuts_help.jump_to.drafts'}}}</li>
       </ul>
       <h4>{{i18n 'keyboard_shortcuts_help.navigation.title'}}</h4>
       <ul>
diff --git a/app/controllers/drafts_controller.rb b/app/controllers/drafts_controller.rb
index 313e8814816..aa8840bd05c 100644
--- a/app/controllers/drafts_controller.rb
+++ b/app/controllers/drafts_controller.rb
@@ -16,29 +16,29 @@ class DraftsController < ApplicationController
       limit: params[:limit]
     }
 
-    guardian.ensure_can_see_drafts!(user)
-    stream = Draft.stream(opts)
-    stream.each do |d|
-      parsed_data = JSON.parse(d.data)
-      if parsed_data
-        if parsed_data['reply']
-          d.raw = parsed_data['reply']
-        end
-        if parsed_data['categoryId'].present? && !d.category_id.present?
-          d.category_id = parsed_data['categoryId']
+    help_key = "user_activity.no_drafts"
+
+    if user == current_user
+      stream = Draft.stream(opts)
+      stream.each do |d|
+        parsed_data = JSON.parse(d.data)
+        if parsed_data
+          if parsed_data['reply']
+            d.raw = parsed_data['reply']
+          end
+          if parsed_data['categoryId'].present? && !d.category_id.present?
+            d.category_id = parsed_data['categoryId']
+          end
         end
       end
-    end
 
-    help_key = "user_activity.no_drafts"
-    if user == current_user
       help_key += ".self"
     else
       help_key += ".others"
     end
 
     render json: {
-      drafts: serialize_data(stream, DraftSerializer),
+      drafts: stream ? serialize_data(stream, DraftSerializer) : [],
       no_results_help: I18n.t(help_key)
     }
 
diff --git a/app/models/draft.rb b/app/models/draft.rb
index 812670feaaf..b9521abe1a2 100644
--- a/app/models/draft.rb
+++ b/app/models/draft.rb
@@ -58,13 +58,15 @@ class Draft < ActiveRecord::Base
         pu.username, pu.name, pu.id user_id, pu.uploaded_avatar_id, pu.username_lower,
         du.username draft_username, NULL as raw, NULL as cooked, NULL as post_number
       FROM drafts d
+      LEFT JOIN LATERAL json_extract_path_text (d.data::json, 'postId') postId ON TRUE
+      LEFT JOIN posts p ON postId :: BIGINT = p.id
       LEFT JOIN topics t ON
         CASE
             WHEN d.draft_key LIKE '%' || '#{EXISTING_TOPIC}' || '%'
               THEN CAST(replace(d.draft_key, '#{EXISTING_TOPIC}', '') AS INT)
             ELSE 0
         END = t.id
-      JOIN users pu on pu.id = COALESCE(t.user_id, d.user_id)
+      JOIN users pu on pu.id = COALESCE(p.user_id, t.user_id, d.user_id)
       JOIN users du on du.id = #{user_id}
       /*where*/
       /*order_by*/
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index ad05f9c7eac..ccc85631817 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -288,12 +288,11 @@ en:
       confirm_clear: "Are you sure you want to clear all the bookmarks from this topic?"
 
     drafts:
-      resume: "Resume Draft"
-      remove: "Remove Draft"
+      resume: "Resume"
+      remove: "Remove"
       new_topic: "New topic draft"
       new_private_message: "New private message draft"
       topic_reply: "Draft reply"
-      post_reply: "Draft reply to #{{postNumber}}"
 
     topic_count_latest:
       one: "See {{count}} new or updated topic"
@@ -2565,6 +2564,7 @@ en:
         bookmarks: '<b>g</b>, <b>b</b> Bookmarks'
         profile: '<b>g</b>, <b>p</b> Profile'
         messages: '<b>g</b>, <b>m</b> Messages'
+        drafts: '<b>g</b>, <b>d</b> Drafts'
       navigation:
         title: 'Navigation'
         jump: '<b>#</b> Go to post #'
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 1d034eca822..2f655bc1012 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -778,7 +778,7 @@ en:
       others: "No replies."
     no_drafts:
       self: "You have no drafts."
-      others: "No drafts."
+      others: "You do not have permission to see drafts for this user."
 
   topic_flag_types:
     spam:
diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb
index b111d5f5112..0aef85c9914 100644
--- a/lib/guardian/user_guardian.rb
+++ b/lib/guardian/user_guardian.rb
@@ -30,10 +30,6 @@ module UserGuardian
     is_me?(user) || is_admin?
   end
 
-  def can_see_drafts?(user)
-    is_me?(user) || is_admin?
-  end
-
   def can_silence_user?(user)
     user && is_staff? && not(user.staff?)
   end
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index 72c1c8db3a5..9b95f66e329 100644
--- a/spec/components/guardian_spec.rb
+++ b/spec/components/guardian_spec.rb
@@ -2350,24 +2350,6 @@ describe Guardian do
     end
   end
 
-  describe "can_see_drafts?" do
-    it "won't allow a non-logged in user to see a user's drafts" do
-      expect(Guardian.new.can_see_drafts?(user)).to be_falsey
-    end
-
-    it "won't allow a user to see another user's drafts" do
-      expect(Guardian.new(coding_horror).can_see_drafts?(user)).to be_falsey
-    end
-
-    it "will allow user to see own drafts" do
-      expect(Guardian.new(user).can_see_drafts?(user)).to be_truthy
-    end
-
-    it "will allow an admin to see a user's drafts" do
-      expect(Guardian.new(admin).can_see_drafts?(user)).to be_truthy
-    end
-  end
-
   describe "can_edit_email?" do
     context 'when allowed in settings' do
       before do
diff --git a/spec/models/draft_spec.rb b/spec/models/draft_spec.rb
index 90fc10270c3..09983a3b8a8 100644
--- a/spec/models/draft_spec.rb
+++ b/spec/models/draft_spec.rb
@@ -73,19 +73,19 @@ describe Draft do
     end
 
     it "should include the correct number of drafts in the stream" do
-      Draft.set(@user, "test", 0, "first")
-      Draft.set(@user, "test2", 0, "second")
+      Draft.set(@user, "test", 0, '{"reply":"hey.","action":"createTopic","title":"Hey"}')
+      Draft.set(@user, "test2", 0, '{"reply":"howdy"}')
       expect(stream.count).to eq(2)
     end
 
     it "should include the right topic id in a draft reply in the stream" do
-      Draft.set(@user, "topic_#{public_topic.id}", 0, "hey")
+      Draft.set(@user, "topic_#{public_topic.id}", 0, '{"reply":"hi"}')
       draft_row = stream.first
       expect(draft_row.topic_id).to eq(public_topic.id)
     end
 
     it "should include the right draft username in the stream" do
-      Draft.set(@user, "topic_#{public_topic.id}", 0, "hey")
+      Draft.set(@user, "topic_#{public_topic.id}", 0, '{"reply":"hey"}')
       draft_row = stream.first
       expect(draft_row.draft_username).to eq(@user.username)
     end