Fixes #4026 - Knowledge Base on-the-fly updating does not work

app/assets/javascripts/app/controllers/knowledge_base/content_controller.coffee

@@ -58,6 +58,10 @@ class App.KnowledgeBaseContentController extends App.Controller
     App.KnowledgeBaseFormController.compareParams(remoteParams, @startingParams)
 
   objectRefreshed: ->
+    if !@object.exists()
+      @parentController.renderNotAvailableAnymore()
+      return
+
     @renderAvailabilityWidgets()
 
     if @remoteDidntChangeSinceStart()

app/controllers/knowledge_bases_controller.rb

@@ -6,7 +6,7 @@ class KnowledgeBasesController < KnowledgeBase::BaseController
   end
 
   def visible_ids
-    render json: KnowledgeBase::InternalAssets.new(current_user).visible_ids
+    render json: calculate_visible_ids
   end
 
   private
@@ -22,6 +22,20 @@ class KnowledgeBasesController < KnowledgeBase::BaseController
     public_assets
   end
 
+  def calculate_visible_ids
+    if KnowledgeBase.granular_permissions? && kb_permissions?
+      return KnowledgeBase::InternalAssets.new(current_user).visible_ids
+    end
+
+    if kb_permission_editor?
+      editor_assets_visible_ids
+    elsif kb_permission_reader?
+      reader_assets_visible_ids
+    else
+      {}
+    end
+  end
+
   def kb_permissions?
     current_user&.permissions?(%w[knowledge_base.editor knowledge_base.reader])
   end
@@ -63,6 +77,13 @@ class KnowledgeBasesController < KnowledgeBase::BaseController
     assets
   end
 
+  def editor_assets_visible_ids
+    {
+      answer_ids:   KnowledgeBase::Answer.pluck(:id),
+      category_ids: KnowledgeBase::Category.pluck(:id)
+    }
+  end
+
   def reader_assets(answer_translation_content_ids)
     assets = [
       KnowledgeBase,
@@ -96,6 +117,13 @@ class KnowledgeBasesController < KnowledgeBase::BaseController
     assets
   end
 
+  def reader_assets_visible_ids
+    {
+      answer_ids:   KnowledgeBase::Answer.internal.pluck(:id),
+      category_ids: KnowledgeBase::Category.pluck(:id)
+    }
+  end
+
   # assets for users who don't have KB permissions
   def public_assets
     return [] if !Setting.get('kb_active_publicly')

app/jobs/checks_kb_client_notification_job.rb

@@ -12,49 +12,39 @@ class ChecksKbClientNotificationJob < ApplicationJob
     object = klass_name.constantize.find_by(id: object_id)
     return if object.blank?
 
-    level = needs_editor?(object) ? 'editor' : '*'
-
     payload = {
       event: 'kb_data_changed',
-      data:  build_data(object, event)
+      data:  build_data(object)
     }
 
-    users_for(level).each { |user| notify(user, payload) }
+    active_users.each do |user|
-  end
+      notify(user, object, payload)
-
-  def build_data(object, event)
-    timestamp = event == :destroy ? Time.zone.now : object.updated_at
-    url       = event == :destroy ? nil           : object.try(:api_url)
-
-    {
-      class:     object.class.to_s,
-      event:     event,
-      id:        object.id,
-      timestamp: timestamp,
-      url:       url
-    }
-  end
-
-  def needs_editor?(object)
-    case object
-    when KnowledgeBase::Answer
-      object.can_be_published_aasm.draft?
-    when KnowledgeBase::Category
-      !object.internal_content?
-    else
-      false
     end
   end
 
-  def notify(user, payload)
+  def build_data(object)
-    PushMessages.send_to(user.id, payload)
+    {
+      class:     object.class.name,
+      id:        object.id,
+      timestamp: object.updated_at,
+      url:       object.try(:api_url)
+    }
   end
 
-  def users_for(permission_suffix)
+  def notify(user, object, payload)
+    return if !user.permissions? 'knowledge_base.*'
+
+    Pundit.authorize user, object, :show?
+
+    PushMessages.send_to(user.id, payload)
+  rescue Pundit::NotAuthorizedError
+    # do nothing if user is not authorized to access
+  end
+
+  def active_users
     Sessions
       .sessions
       .filter_map { |client_id| Sessions.get(client_id)&.dig(:user, 'id') }
       .filter_map { |user_id| User.find_by(id: user_id) }
-      .select { |user| user.permissions? "knowledge_base.#{permission_suffix}" }
   end
 end

app/models/concerns/checks_kb_client_notification.rb

@@ -30,6 +30,10 @@ module ChecksKbClientNotification
   def notify_kb_clients_after
     return if self.class.notify_kb_clients_suspend?
 
+    # do not leak details about deleted items. See ChecksKbClientVisibilityJob
+    # after_commit does not allow on: :touch
+    return if destroyed?
+
     ChecksKbClientNotificationJob.perform_later(self.class.name, id)
   end
 end

spec/jobs/checks_kb_client_notification_job_spec.rb

@@ -0,0 +1,40 @@
+# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe ChecksKbClientNotificationJob, type: :job, performs_jobs: true do
+  include_context 'basic Knowledge Base'
+
+  describe 'pushes to users who have access' do
+    let(:admin) { create(:admin) }
+    let(:agent) { create(:agent) }
+    let(:customer) { create(:customer) }
+
+    before do
+      allow_any_instance_of(described_class) # rubocop:disable RSpec/AnyInstance
+        .to receive(:active_users)
+        .and_return([admin, agent, customer])
+
+      allow(PushMessages).to receive(:send_to)
+
+      described_class.perform_now 'KnowledgeBase::Answer', answer.id
+    end
+
+    shared_examples 'message pushed to given users' do |prefix, **args|
+      context "when answer is #{prefix}" do
+        let(:answer) { send("#{prefix}_answer") }
+
+        args.each do |key, value|
+          it "#{key} #{value ? 'is' : 'not'} notified" do
+            expect(PushMessages).send(value ? :to : :not_to, have_received(:send_to).with(send(key).id, any_args))
+          end
+        end
+      end
+    end
+
+    include_examples 'message pushed to given users', 'published', admin: true, agent: true,  customer: false
+    include_examples 'message pushed to given users', 'internal',  admin: true, agent: true,  customer: false
+    include_examples 'message pushed to given users', 'draft',     admin: true, agent: false, customer: false
+    include_examples 'message pushed to given users', 'archived',  admin: true, agent: false, customer: false
+  end
+end

spec/models/concerns/checks_kb_client_notification_examples.rb

@@ -5,7 +5,7 @@ RSpec.shared_examples 'ChecksKbClientNotification' do
     before { subject }
 
     it 'on creation' do
-      expect(ChecksKbClientNotificationJob).to have_been_enqueued.at_least(:once) # some object have associations that triggers touch job after creation
+      expect(ChecksKbClientNotificationJob).to have_been_enqueued.with(subject.class.name, subject.id)
     end
 
     context 'after initial notifications are cleared' do
@@ -13,17 +13,17 @@ RSpec.shared_examples 'ChecksKbClientNotification' do
 
       it 'on update' do
         subject.update(updated_at: Time.zone.now)
-        expect(ChecksKbClientNotificationJob).to have_been_enqueued.at_least(:once) # some object have associations that triggers touch job after creation
+        expect(ChecksKbClientNotificationJob).to have_been_enqueued.with(subject.class.name, subject.id)
       end
 
       it 'on touch' do
         subject.touch
-        expect(ChecksKbClientNotificationJob).to have_been_enqueued.at_least(:once) # some object have associations that triggers touch job after creation
+        expect(ChecksKbClientNotificationJob).to have_been_enqueued.with(subject.class.name, subject.id)
       end
 
       it 'on destroy' do
         subject.destroy
-        expect(ChecksKbClientNotificationJob).to have_been_enqueued.at_least(:once) # some object have associations that triggers touch job after creation
+        expect(ChecksKbClientNotificationJob).not_to have_been_enqueued.with(subject.class.name, subject.id)
       end
 
       it 'notifications be disabled' do

spec/system/knowledge_base/locale/answer/edit_spec.rb

@@ -124,4 +124,50 @@ RSpec.describe 'Knowledge Base Locale Answer Edit', type: :system do
       end
     end
   end
+
+  context 'deleted by another user' do
+    before do
+      visit "#knowledge_base/#{knowledge_base.id}/locale/#{primary_locale.system_locale.locale}/answer/#{published_answer.id}/edit"
+    end
+
+    it 'shows not available', performs_jobs: true do
+      find(:active_content, text: published_answer.translations.first.title)
+
+      perform_enqueued_jobs do
+        ActiveRecord::Base.transaction do
+          published_answer.destroy
+        end
+      end
+
+      within :active_content do
+        expect(page).to have_text('The page is not available anymore')
+      end
+    end
+  end
+
+  context 'updated by another user' do
+    before do
+      ensure_websocket do
+        visit "#knowledge_base/#{knowledge_base.id}/locale/#{primary_locale.system_locale.locale}/answer/#{published_answer.id}/edit"
+      end
+
+      travel 1.minute
+    end
+
+    it 'shows new content', performs_jobs: true do
+      find(:active_content, text: published_answer.translations.first.title)
+
+      accept_prompt do
+        perform_enqueued_jobs do
+          Transaction.execute do
+            published_answer.translations.first.update! title: 'new title'
+          end
+        end
+      end
+
+      within :active_content do
+        expect(page).to have_text('new title')
+      end
+    end
+  end
 end

spec/system/knowledge_base/locale/answer/read_spec.rb

@@ -48,4 +48,48 @@ RSpec.describe 'Knowledge Base Locale Answer Read', type: :system, authenticated
       end
     end
   end
+
+  context 'deleted by another user' do
+    before do
+      visit "#knowledge_base/#{knowledge_base.id}/locale/#{locale_name}/answer/#{published_answer.id}"
+    end
+
+    it 'shows not available', performs_jobs: true do
+      find(:active_content, text: published_answer.translations.first.title)
+
+      perform_enqueued_jobs do
+        ActiveRecord::Base.transaction do
+          published_answer.destroy
+        end
+      end
+
+      within :active_content do
+        expect(page).to have_text('The page is not available anymore')
+      end
+    end
+  end
+
+  context 'updated by another user' do
+    before do
+      ensure_websocket do
+        visit "#knowledge_base/#{knowledge_base.id}/locale/#{locale_name}/answer/#{published_answer.id}"
+      end
+
+      travel 1.minute
+    end
+
+    it 'shows new content', performs_jobs: true do
+      find(:active_content, text: published_answer.translations.first.title)
+
+      perform_enqueued_jobs do
+        Transaction.execute do
+          published_answer.translations.first.update! title: 'new title'
+        end
+      end
+
+      within :active_content do
+        expect(page).to have_text('new title')
+      end
+    end
+  end
 end