FEATURE: replace PM tags dropdown with a dedicated tags page

2018-03-11 10:59:34 +05:30 · 2018-03-11 10:59:34 +05:30 · 24338fbbe8
parent 74ce2220a7
commit 24338fbbe8
23 changed files with 143 additions and 100 deletions
--- a/app/assets/javascripts/discourse/components/tag-list.js.es6
+++ b/app/assets/javascripts/discourse/components/tag-list.js.es6
@ -1,6 +1,7 @@
 export default Ember.Component.extend({
  classNameBindings: [':tag-list', 'categoryClass'],

+  isPrivateMessage: false,
  sortedTags: Ember.computed.sort('tags', 'sortProperties'),

  title: function() {
--- a/app/assets/javascripts/discourse/controllers/user-private-messages-tags.js.es6
+++ b/app/assets/javascripts/discourse/controllers/user-private-messages-tags.js.es6
@ -0,0 +1,13 @@
+export default Ember.Controller.extend({
+  sortProperties: ['count:desc', 'id'],
+
+  actions: {
+    sortByCount() {
+      this.set('sortProperties', ['count:desc', 'id']);
+    },
+
+    sortById() {
+      this.set('sortProperties', ['id']);
+    }
+  }
+});
--- a/app/assets/javascripts/discourse/controllers/user-private-messages.js.es6
+++ b/app/assets/javascripts/discourse/controllers/user-private-messages.js.es6
@ -12,7 +12,7 @@ export default Ember.Controller.extend({
  currentPath: Em.computed.alias('application.currentPath'),
  selected: Em.computed.alias('userTopicsList.selected'),
  bulkSelectEnabled: Em.computed.alias('userTopicsList.bulkSelectEnabled'),
-  pmTags: Em.computed.alias('userTopicsList.model.topic_list.pm_tags'),
+  showToggleBulkSelect: true,
  pmTaggingEnabled: Ember.computed.alias('site.can_tag_pms'),
  tagId: null,

--- a/app/assets/javascripts/discourse/lib/render-tag.js.es6
+++ b/app/assets/javascripts/discourse/lib/render-tag.js.es6
@ -6,7 +6,7 @@ export default function renderTag(tag, params) {
  let path;
  if (tagName === "a" && !params.noHref) {
    const current_user = Discourse.User.current();
-    path = params.isPrivateMessage ? `/u/${current_user.username}/messages/tag/${tag}` : `/tags/${tag}`;
+    path = params.isPrivateMessage ? `/u/${current_user.username}/messages/tags/${tag}` : `/tags/${tag}`;
  }
  const href = path ? ` href='${Discourse.getURL(path)}' ` : "";

--- a/app/assets/javascripts/discourse/routes/app-route-map.js.es6
+++ b/app/assets/javascripts/discourse/routes/app-route-map.js.es6
@ -96,7 +96,8 @@ export default function() {
      this.route('archive');
      this.route('group', { path: 'group/:name'});
      this.route('groupArchive', { path: 'group/:name/archive'});
-      this.route('tag', { path: 'tag/:id'});
+      this.route('tags');
+      this.route('tagsShow', { path: 'tags/:id'});
    });

    this.route('preferences', { resetNamespace: true }, function() {
--- a/app/assets/javascripts/discourse/routes/build-private-messages-route.js.es6
+++ b/app/assets/javascripts/discourse/routes/build-private-messages-route.js.es6
@ -35,8 +35,12 @@ export default (viewName, path, channel) => {
        selected: []
      });

-      this.controllerFor("user-private-messages").set("archive", false);
-      this.controllerFor("user-private-messages").set("pmView", viewName);
+      this.controllerFor("user-private-messages").setProperties({
+        archive: false,
+        pmView: viewName,
+        showToggleBulkSelect: true
+      });
+
      this.searchService.set('contextType', 'private_messages');
    },

--- a/app/assets/javascripts/discourse/routes/user-private-messages-tags-show.js.es6
+++ b/app/assets/javascripts/discourse/routes/user-private-messages-tags-show.js.es6
@ -2,10 +2,9 @@ import createPMRoute from "discourse/routes/build-private-messages-route";

 export default createPMRoute('tags', 'private-messages-tags').extend({
    model(params) {
-      this.controllerFor('user-private-messages').set('tagId', params.id);
      const username = this.modelFor("user").get("username_lower");
      return this.store.findFiltered("topicList", {
-        filter: `topics/private-messages-tag/${username}/${params.id}`
+        filter: `topics/private-messages-tags/${username}/${params.id}`
      });
    }
 });
--- a/app/assets/javascripts/discourse/routes/user-private-messages-tags.js.es6
+++ b/app/assets/javascripts/discourse/routes/user-private-messages-tags.js.es6
@ -0,0 +1,22 @@
+import { ajax } from 'discourse/lib/ajax';
+import { popupAjaxError } from 'discourse/lib/ajax-error';
+
+export default Discourse.Route.extend({
+  model() {
+    return ajax('/tags/personal_messages').then(result => {
+      return result.tags.map(tag => Ember.Object.create(tag));
+    }).catch(popupAjaxError);
+  },
+
+  titleToken() {
+    return [I18n.t("tagging.tags"), I18n.t("user.private_messages")];
+  },
+
+  setupController(controller, model) {
+    this.controllerFor('user-private-messages-tags').setProperties({
+      model,
+      sortProperties: this.siteSettings.tags_sort_alphabetically ? ['id'] : ['count:desc', 'id']
+    });
+    this.controllerFor("user-private-messages").set("showToggleBulkSelect", false);
+  }
+});
--- a/app/assets/javascripts/discourse/templates/components/tag-list.hbs
+++ b/app/assets/javascripts/discourse/templates/components/tag-list.hbs
@ -10,7 +10,7 @@
 {{#each sortedTags as |tag|}}
  <div class='tag-box'>
    {{#if tag.count}}
-      {{discourse-tag tag.id}} <span class='tag-count'>x {{tag.count}}</span>
+      {{discourse-tag tag.id isPrivateMessage=isPrivateMessage}} <span class='tag-count'>x {{tag.count}}</span>
    {{else}}
      {{discourse-tag tag.id}}
    {{/if}}
--- a/app/assets/javascripts/discourse/templates/user-private-messages-tags.hbs
+++ b/app/assets/javascripts/discourse/templates/user-private-messages-tags.hbs
@ -0,0 +1,17 @@
+<div class="list-controls">
+  <div class="container">
+    <h2>{{i18n "tagging.tags"}}</h2>
+  </div>
+</div>
+
+<div class='tag-sort-options'>
+  {{i18n "tagging.sort_by"}}
+  <a {{action "sortByCount"}}>{{i18n "tagging.sort_by_count"}}</a>
+  <a {{action "sortById"}}>{{i18n "tagging.sort_by_name"}}</a>
+</div>
+
+<hr/>
+
+{{#if model}}
+  {{tag-list tags=model sortProperties=sortProperties titleKey="tagging.all_tags" isPrivateMessage=true}}
+{{/if}}
--- a/app/assets/javascripts/discourse/templates/user/messages.hbs
+++ b/app/assets/javascripts/discourse/templates/user/messages.hbs
@ -23,29 +23,39 @@
    </li>
    {{plugin-outlet name="user-messages-nav" connectorTagName='li' args=(hash model=model)}}
    {{#each model.groups as |group|}}
-    {{#if group.has_messages}}
-    <li>
-      {{#link-to 'userPrivateMessages.group' group.name}}
-        {{d-icon "group" class="glyph"}}
-        {{capitalize-string group.name}}
-      {{/link-to}}
-    </li>
-    <li class='archive'>
-      {{#link-to 'userPrivateMessages.groupArchive' group.name}}
-        {{i18n 'user.messages.archive'}}
+      {{#if group.has_messages}}
+        <li>
+          {{#link-to 'userPrivateMessages.group' group.name}}
+            {{d-icon "group" class="glyph"}}
+            {{capitalize-string group.name}}
+          {{/link-to}}
+        </li>
+        <li class='archive'>
+          {{#link-to 'userPrivateMessages.groupArchive' group.name}}
+            {{i18n 'user.messages.archive'}}
+          {{/link-to}}
+        </li>
+      {{/if}}
+    {{/each}}
+
+    {{#if pmTaggingEnabled}}
+      <li class="noGlyph">
+      {{#link-to 'userPrivateMessages.tags' model}}
+        {{i18n 'user.messages.tags'}}
      {{/link-to}}
    </li>
    {{/if}}
-    {{/each}}
  {{/mobile-nav}}
 {{/d-section}}

 <section class='user-right messages'>

 <div class="clearfix list-actions">
-    <button {{action "toggleBulkSelect"}} class="btn bulk-select" title="{{i18n "user.messages.bulk_select"}}">
-      {{d-icon "list"}}
-    </button>
+    {{#if showToggleBulkSelect}}
+      <button {{action "toggleBulkSelect"}} class="btn bulk-select" title="{{i18n "user.messages.bulk_select"}}">
+        {{d-icon "list"}}
+      </button>
+    {{/if}}

    {{#if site.mobileView}}
      {{#if showNewPM}}
@ -74,10 +84,6 @@
    {{#if isGroup}}
      {{group-notifications-button value=group.group_user.notification_level group=group user=model}}
    {{/if}}
-
-    {{#if pmTaggingEnabled}}
-      {{pm-tag-drop pmTags=pmTags tagId=tagId}}
-    {{/if}}
 </div>

 {{outlet}}
--- a/app/assets/javascripts/select-kit/components/pm-tag-drop.js.es6
+++ b/app/assets/javascripts/select-kit/components/pm-tag-drop.js.es6
@ -1,19 +0,0 @@
-import TagDropComponent from "select-kit/components/tag-drop";
-import DiscourseURL from "discourse/lib/url";
-import { default as computed } from "ember-addons/ember-computed-decorators";
-
-export default TagDropComponent.extend({
-  @computed
-  allTagsUrl() {
-    return `/u/${this.currentUser.username}/messages/`;
-  },
-
-  content: Ember.computed.alias("pmTags"),
-
-  actions: {
-    onSelect(tagId) {
-      const url = `/u/${this.currentUser.username}/messages/tag/${tagId}`;
-      DiscourseURL.routeTo(url);
-    }
-  }
-});
--- a/app/controllers/list_controller.rb
+++ b/app/controllers/list_controller.rb
@ -134,7 +134,6 @@ class ListController < ApplicationController
  def self.generate_message_route(action)
    define_method("#{action}") do
      list_opts = build_topic_list_options
-      list_opts[:show_pm_tags] = true if guardian.can_tag_pms?
      target_user = fetch_user_from_params({ include_inactive: current_user.try(:staff?) }, [:user_stat, :user_option])
      guardian.ensure_can_see_private_messages!(target_user.id)
      list = generate_list_for(action.to_s, target_user, list_opts)
--- a/app/controllers/tags_controller.rb
+++ b/app/controllers/tags_controller.rb
@ -19,7 +19,7 @@ class TagsController < ::ApplicationController
  skip_before_action :check_xhr, only: [:tag_feed, :show, :index]

  before_action :set_category_from_params, except: [:index, :update, :destroy,
-    :tag_feed, :search, :notifications, :update_notifications]
+    :tag_feed, :search, :notifications, :update_notifications, :personal_messages]

  def index
    @description_meta = I18n.t("tags.title")
@ -191,6 +191,13 @@ class TagsController < ::ApplicationController
    render json: { valid: valid_tags }
  end

+  def personal_messages
+    guardian.ensure_can_tag_pms!
+    pm_tags = Tag.pm_tags(guardian: guardian)
+
+    render json: { tags: pm_tags }
+  end
+
  private

    def ensure_tags_enabled
--- a/app/models/tag.rb
+++ b/app/models/tag.rb
@ -63,8 +63,8 @@ class Tag < ActiveRecord::Base
    return [] unless (guardian || Guardian.new).can_tag_pms?
    limit = limit_arg || SiteSetting.max_tags_in_filter_list

-    tag_names = Tag.exec_sql <<~SQL
-      SELECT tags.name AS tag_name
+    tag_names_with_counts = Tag.exec_sql <<~SQL
+      SELECT tags.name, COUNT(topics.id) AS topic_count
      FROM tags
      INNER JOIN topic_tags ON tags.id = topic_tags.tag_id
      INNER JOIN topics ON topics.id = topic_tags.topic_id
@ -74,7 +74,7 @@ class Tag < ActiveRecord::Base
      LIMIT #{limit}
    SQL

-    tag_names.values.flatten
+    tag_names_with_counts.map { |t| { id: t['name'], text: t['name'], count: t['topic_count'] } }
  end

  def self.include_tags?
--- a/app/models/topic_list.rb
+++ b/app/models/topic_list.rb
@ -35,7 +35,6 @@ class TopicList
                :for_period,
                :per_page,
                :top_tags,
-                :pm_tags,
                :current_user,
                :tags

@ -60,15 +59,6 @@ class TopicList
    Tag.top_tags(opts)
  end

-  def pm_tags
-    guardian = Guardian.new(@current_user)
-    if @opts[:show_pm_tags] && guardian.can_tag_pms?
-      Tag.pm_tags(guardian: guardian)
-    else
-      []
-    end
-  end
-
  def preload_key
    if @category
      "topic_list_#{@category.url.sub(/^\//, '')}/l/#{@filter}"
--- a/app/serializers/topic_list_serializer.rb
+++ b/app/serializers/topic_list_serializer.rb
@ -8,7 +8,6 @@ class TopicListSerializer < ApplicationSerializer
             :for_period,
             :per_page,
             :top_tags,
-             :pm_tags,
             :tags

  has_many :topics, serializer: TopicListItemSerializer, embed: :objects
@ -30,10 +29,6 @@ class TopicListSerializer < ApplicationSerializer
    Tag.include_tags?
  end

-  def include_pm_tags?
-    scope.can_tag_pms?
-  end
-
  def include_tags?
    SiteSetting.tagging_enabled && object.tags.present?
  end
--- a/config/routes.rb
+++ b/config/routes.rb
@ -368,7 +368,7 @@ Discourse::Application.routes.draw do
    get "#{root_path}/:username/messages/:filter" => "user_actions#private_messages", constraints: { username: RouteFormat.username }
    get "#{root_path}/:username/messages/group/:group_name" => "user_actions#private_messages", constraints: { username: RouteFormat.username, group_name: RouteFormat.username }
    get "#{root_path}/:username/messages/group/:group_name/archive" => "user_actions#private_messages", constraints: { username: RouteFormat.username, group_name: RouteFormat.username }
-    get "#{root_path}/:username/messages/tag/:tag_id" => "user_actions#private_messages", constraints: StaffConstraint.new
+    get "#{root_path}/:username/messages/tags/:tag_id" => "user_actions#private_messages", constraints: StaffConstraint.new
    get "#{root_path}/:username.json" => "users#show", constraints: { username: RouteFormat.username }, defaults: { format: :json }
    get({ "#{root_path}/:username" => "users#show", constraints: { username: RouteFormat.username, format: /(json|html)/ } }.merge(index == 1 ? { as: 'user' } : {}))
    put "#{root_path}/:username" => "users#update", constraints: { username: RouteFormat.username }, defaults: { format: :json }
@ -607,7 +607,7 @@ Discourse::Application.routes.draw do
    get "private-messages-sent/:username" => "list#private_messages_sent", as: "topics_private_messages_sent"
    get "private-messages-archive/:username" => "list#private_messages_archive", as: "topics_private_messages_archive"
    get "private-messages-unread/:username" => "list#private_messages_unread", as: "topics_private_messages_unread"
-    get "private-messages-tag/:username/:tag_id.json" => "list#private_messages_tag", as: "topics_private_messages_tag", constraints: StaffConstraint.new
+    get "private-messages-tags/:username/:tag_id.json" => "list#private_messages_tag", as: "topics_private_messages_tag", constraints: StaffConstraint.new

    scope "/private-messages-group/:username", group_name: RouteFormat.username do
      get ":group_name.json" => "list#private_messages_group", as: "topics_private_messages_group"
@ -732,6 +732,7 @@ Discourse::Application.routes.draw do
    get '/filter/list' => 'tags#index'
    get '/filter/search' => 'tags#search'
    get '/check' => 'tags#check_hashtag'
+    get '/personal_messages' => 'tags#personal_messages'
    constraints(tag_id: /[^\/]+?/, format: /json|rss/) do
      get '/:tag_id.rss' => 'tags#tag_feed'
      get '/:tag_id' => 'tags#show', as: 'tag_show'
--- a/lib/topic_query.rb
+++ b/lib/topic_query.rb
@ -32,8 +32,7 @@ class TopicQuery
         match_all_tags
         no_subcategories
         slow_platform
-         no_tags
-         show_pm_tags)
+         no_tags)
  end

  def self.valid_options
--- a/spec/models/tag_spec.rb
+++ b/spec/models/tag_spec.rb
@ -98,9 +98,8 @@ describe Tag do

  describe '#pm_tags' do
    before do
-      @private_tags = []
      personal_message = Fabricate(:private_message_topic)
-      2.times { |i| @private_tags << Fabricate(:tag, topics: [personal_message]) }
+      2.times { |i| Fabricate(:tag, topics: [personal_message], name: "tag-#{i}") }
    end

    it "returns nothing if user is not a staff" do
@ -114,7 +113,10 @@ describe Tag do

    it "returns all pm tags if user is a staff and pm tagging is enabled" do
      SiteSetting.allow_staff_to_tag_pms = true
-      expect(described_class.pm_tags(guardian: Guardian.new(Fabricate(:admin)))).to match_array(@private_tags.map(&:name))
+      tags = described_class.pm_tags(guardian: Guardian.new(Fabricate(:admin)))
+      expect(tags.length).to eq(2)
+      expect(tags[0][:id]).to eq("tag-0")
+      expect(tags[1][:text]).to eq("tag-1")
    end
  end

--- a/spec/models/topic_list_spec.rb
+++ b/spec/models/topic_list_spec.rb
@ -78,28 +78,4 @@ describe TopicList do
      end
    end
  end
-
-  describe '#pm_tags' do
-    let(:admin) { Fabricate(:admin) }
-    let(:personal_message) { Fabricate(:private_message_topic) }
-
-    before do
-      SiteSetting.tagging_enabled = true
-      SiteSetting.allow_staff_to_tag_pms = true
-      @private_tags = []
-      2.times { |i| @private_tags << Fabricate(:tag, topics: [personal_message]) }
-    end
-
-    context 'when viewed as normal user' do
-      it 'returns no tags' do
-        expect(TopicList.new('liked', personal_message.user, [personal_message], show_pm_tags: true).pm_tags).to be_empty
-      end
-    end
-
-    context 'when viewed as admin' do
-      it 'returns pm tags' do
-        expect(TopicList.new('liked', admin, [personal_message], show_pm_tags: true).pm_tags).to match_array(@private_tags.map(&:name))
-      end
-    end
-  end
 end
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@ -96,17 +96,15 @@ RSpec.describe ListController do

    it 'should fail for non-staff users' do
      sign_in(user)
-      get "/topics/private-messages-tag/#{user.username}/#{tag.name}.json"
+      get "/topics/private-messages-tags/#{user.username}/#{tag.name}.json"
      expect(response.status).to eq(404)
    end

    it 'should be success for staff users' do
      [moderator, admin].each do |user|
        sign_in(user)
-        get "/topics/private-messages-tag/#{user.username}/#{tag.name}.json"
+        get "/topics/private-messages-tags/#{user.username}/#{tag.name}.json"
        expect(response).to be_success
-        data = JSON.parse(response.body)
-        expect(data["topic_list"]["pm_tags"].length).to eq(1)
      end
    end
  end
--- a/spec/requests/tags_controller_spec.rb
+++ b/spec/requests/tags_controller_spec.rb
@ -53,4 +53,36 @@ describe TagsController do
      expect(tag["value"]).to eq('test')
    end
  end
+
+  describe '#personal_messages' do
+    before do
+      SiteSetting.allow_staff_to_tag_pms = true
+      personal_message = Fabricate(:private_message_topic)
+      Fabricate(:tag, topics: [personal_message], name: 'test')
+    end
+
+    context "as a normal user" do
+      it "should return the right response" do
+        get "/tags/personal_messages.json"
+
+        expect(response).not_to be_success
+      end
+    end
+
+    context "as an admin" do
+      before do
+        admin = Fabricate(:admin)
+        sign_in(admin)
+      end
+
+      it "should return the right response" do
+        get "/tags/personal_messages.json"
+
+        expect(response).to be_success
+
+        tag = JSON.parse(response.body)['tags']
+        expect(tag[0]["id"]).to eq('test')
+      end
+    end
+  end
 end