Merge pull request #9776 from opf/fix/39131-wiki-history-change-author-always-set-to-page-creator

Fix/39131 wiki history change author always set to page creator

app/controllers/wiki_controller.rb

@@ -151,8 +151,6 @@ class WikiController < ApplicationController
     end
 
     @content = @page.content_for_version(params[:version])
-    # don't keep previous comment
-    @content.comments = nil
 
     # To prevent StaleObjectError exception when reverting to a previous version
     @content.lock_version = @page.content.lock_version

app/models/permitted_params.rb

@@ -601,9 +601,9 @@ class PermittedParams
           redirect_existing_links
         ),
         wiki_content: %i(
-          comments
           text
           lock_version
+          journal_notes
         ),
         move_to: [:move_to]
       }

app/models/wiki_content.rb

@@ -33,11 +33,6 @@ class WikiContent < ApplicationRecord
 
   belongs_to :page, class_name: 'WikiPage'
   belongs_to :author, class_name: 'User'
-  validates_length_of :comments, maximum: 255, allow_nil: true
-
-  attr_accessor :comments
-
-  before_save :comments_to_journal_notes
 
   acts_as_journalized
 
@@ -63,9 +58,7 @@ class WikiContent < ApplicationRecord
     page.visible?(user)
   end
 
-  def project
-    page.project
-  end
+  delegate :project, to: :page
 
   def attachments
     page.nil? ? [] : page.attachments
@@ -77,14 +70,7 @@ class WikiContent < ApplicationRecord
 
   deprecated_alias :versions, :journals
 
-  # REVIEW
   def version
     last_journal.nil? ? 0 : last_journal.version
   end
-
-  private
-
-  def comments_to_journal_notes
-    add_journal author, comments
-  end
 end

app/services/wiki_pages/set_attributes_service.rb

@@ -72,6 +72,6 @@ class WikiPages::SetAttributesService < ::BaseServices::SetAttributes
   end
 
   def content_attribute?(name)
-    WikiContent.column_names.include?(name) || name.to_s == 'comments'
+    WikiContent.column_names.include?(name) || name.to_s == 'journal_notes'
   end
 end

app/services/wiki_pages/update_service.rb

@@ -31,7 +31,7 @@
 class WikiPages::UpdateService < ::BaseServices::Update
   include Attachments::ReplaceAttachments
 
-  private
+  protected
 
   def persist(service_result)
     service_result = super(service_result)

app/views/user_mailer/wiki_content_added.html.erb

@@ -31,5 +31,5 @@ See COPYRIGHT and LICENSE files for more details.
   <%=raw t(:mail_body_wiki_content_added,
               id: link_to(@wiki_content.page.title, project_wiki_url(@wiki_content.page.project, @wiki_content.page)),
               author: @wiki_content.author) %><br />
-  <em><%= @wiki_content.comments %></em>
+  <em><%= @wiki_content.journals.last.notes %></em>
 </p>

app/views/user_mailer/wiki_content_added.text.erb

@@ -30,6 +30,6 @@ See COPYRIGHT and LICENSE files for more details.
 <%= t(:mail_body_wiki_content_added,
           id: @wiki_content.page.title,
           author: @wiki_content.author) %>
-<%= @wiki_content.comments %>
+<%= @wiki_content.journals.last.notes %>
 
 <%= project_wiki_url(@wiki_content.page.project, @wiki_content.page) %>

app/views/user_mailer/wiki_content_updated.html.erb

@@ -31,7 +31,7 @@ See COPYRIGHT and LICENSE files for more details.
   <%=raw t(:mail_body_wiki_content_updated,
                id: link_to(@wiki_content.page.title, project_wiki_url(@wiki_content.page.project, @wiki_content.page)),
                author: @wiki_content.author) %><br />
-  <em><%= @wiki_content.comments %></em>
+  <em><%= @wiki_content.journals.last.notes %></em>
 </p>
 
 <p><%= t(:label_view_diff) %>: <%= link_to @wiki_diff_url, @wiki_diff_url %></p>

app/views/user_mailer/wiki_content_updated.text.erb

@@ -30,7 +30,7 @@ See COPYRIGHT and LICENSE files for more details.
 <%= t(:mail_body_wiki_content_updated,
           id: @wiki_content.page.title,
           author: @wiki_content.author) %>
-<%= @wiki_content.comments %>
+<%= @wiki_content.journals.last.notes %>
 
 <%= project_wiki_url(@wiki_content.page.project, @wiki_content.page) %>
 

app/views/wiki/_page_form.html.erb

@@ -32,7 +32,7 @@
 </div>
 
 <div class="form--field">
-  <%= f.text_field :comments,
+  <%= f.text_field :journal_notes,
                    class: '-border-on-hover-only -hide-placeholder-on-focus',
                    label_options: { class: 'hidden-for-sighted' },
                    autocomplete: 'off',

modules/meeting/app/controllers/meeting_contents_controller.rb

@@ -138,6 +138,6 @@ class MeetingContentsController < ApplicationController
   end
 
   def content_params
-    params.require(@content_type).permit(:text, :lock_version, :comment)
+    params.require(@content_type).permit(:text, :lock_version, :journal_notes)
   end
 end

modules/meeting/app/controllers/meetings_controller.rb

@@ -68,7 +68,7 @@ class MeetingsController < ApplicationController
     if params[:copied_from_meeting_id].present? && params[:copied_meeting_agenda_text].present?
       @meeting.agenda = MeetingAgenda.new(
         text: params[:copied_meeting_agenda_text],
-        comment: "Copied from Meeting ##{params[:copied_from_meeting_id]}"
+        journal_notes: I18n.t('meeting.copied', id: params[:copied_from_meeting_id])
       )
       @meeting.agenda.author = User.current
     end

modules/meeting/app/models/meeting.rb

@@ -180,7 +180,9 @@ class Meeting < ApplicationRecord
 
       attachments = agenda.attachments.map { |a| [a, a.copy] }
       original_text = String(agenda.text)
-      minutes = create_minutes(text: original_text, comment: 'Minutes created', attachments: attachments.map(&:last))
+      minutes = create_minutes(text: original_text,
+                               journal_notes: I18n.t('events.meeting_minutes_created'),
+                               attachments: attachments.map(&:last))
 
       # substitute attachment references in text to use the respective copied attachments
       updated_text = original_text.gsub(/(?<=\(\/api\/v3\/attachments\/)\d+(?=\/content\))/) do |id|

modules/meeting/app/models/meeting_agenda.rb

@@ -27,16 +27,15 @@
 #++
 
 class MeetingAgenda < MeetingContent
-  # TODO: internationalize the comments
   def lock!(user = User.current)
-    self.comment = 'Agenda closed'
+    self.journal_notes = I18n.t('events.meeting_agenda_closed')
     self.author = user
     self.locked = true
     save
   end
 
   def unlock!(user = User.current)
-    self.comment = 'Agenda opened'
+    self.journal_notes = I18n.t('events.meeting_agenda_opened')
     self.author = user
     self.locked = false
     save

modules/meeting/app/models/meeting_content.rb

@@ -31,13 +31,7 @@ class MeetingContent < ApplicationRecord
   include OpenProject::Journal::AttachmentHelper
 
   belongs_to :meeting
-  belongs_to :author, class_name: 'User', foreign_key: 'author_id'
-
-  attr_accessor :comment
-
-  validates_length_of :comment, maximum: 255, allow_nil: true
-
-  before_save :comment_to_journal_notes
+  belongs_to :author, class_name: 'User'
 
   acts_as_attachable(
     after_remove: :attachments_changed,
@@ -77,13 +71,5 @@ class MeetingContent < ApplicationRecord
   end
 
   # Show the project on activity and search views
-  def project
-    meeting.project
-  end
-
-  private
-
-  def comment_to_journal_notes
-    add_journal(author, comment) unless changes.empty?
-  end
+  delegate :project, to: :meeting
 end

modules/meeting/app/views/meeting_contents/_form.html.erb

@@ -47,7 +47,7 @@ See COPYRIGHT and LICENSE files for more details.
   <%= f.hidden_field :lock_version %>
   <% path = send("preview_#{content_type}_path", content.meeting) %>
 
-  <p><%= f.text_field :comment, :size => 120 %></p>
+  <p><%= f.text_field :journal_notes, label: :comments %></p>
   <p><%= styled_button_tag t(:button_save), class: '-highlight -with-icon icon-checkmark button--save-agenda' %>
   <%= link_to t(:button_cancel), "#", data: { 'content-type': content_type }, class: 'button -with-icon icon-cancel button--cancel-agenda' %>
 <% end %>

modules/meeting/config/locales/en.yml

@@ -75,6 +75,9 @@ en:
   label_time_zone: "Time zone"
   label_start_date: "Start date"
 
+  meeting:
+    copied: "Copied from Meeting #%{id}"
+
   notice_successful_notification: "Notification sent successfully"
   notice_timezone_missing: No time zone is set and %{zone} is assumed. To choose your time zone, please click here.
 

spec/features/wiki/adding_editing_history_spec.rb

@@ -37,6 +37,11 @@ describe 'wiki pages', type: :feature, js: true, with_settings: { journal_aggreg
                       member_in_project: project,
                       member_through_role: role
   end
+  let(:other_user) do
+    FactoryBot.create :user,
+                      member_in_project: project,
+                      member_through_role: role
+  end
   let(:role) do
     FactoryBot.create(:role,
                       permissions: %i[view_wiki_pages
@@ -51,6 +56,12 @@ describe 'wiki pages', type: :feature, js: true, with_settings: { journal_aggreg
   let(:content_second_version) do
     'The new content, second version'
   end
+  let(:content_third_version) do
+    'The new content, third version'
+  end
+  let(:other_user_comment) do
+    'Other users`s comment'
+  end
 
   before do
     login_as user
@@ -120,5 +131,30 @@ describe 'wiki pages', type: :feature, js: true, with_settings: { journal_aggreg
 
     expect(page).to have_selector('.wiki-version--details', text: 'Version 2/2')
     expect(page).to have_selector('.wiki-content', text: content_second_version)
+
+    login_as other_user
+
+    visit project_wiki_path(project, 'new page')
+
+    within '.toolbar-items' do
+      SeleniumHubWaiter.wait
+      click_on "Edit"
+    end
+
+    find('.ck-content').set(content_third_version)
+
+    fill_in 'Journal notes', with: other_user_comment
+
+    SeleniumHubWaiter.wait
+    click_button 'Save'
+
+    within '.toolbar-items' do
+      SeleniumHubWaiter.wait
+      click_on 'More'
+      click_on 'History'
+    end
+
+    expect(page).to have_selector('tr.wiki-page-version:last-of-type .author', text: other_user.name)
+    expect(page).to have_selector('tr.wiki-page-version:last-of-type .comments', text: other_user_comment)
   end
 end

spec/models/permitted_params_spec.rb

@@ -890,7 +890,7 @@ describe PermittedParams, type: :model do
     let (:attribute) { :wiki_content }
 
     describe 'title' do
-      let(:hash) { { 'comments' => 'blubs' } }
+      let(:hash) { { 'journal_notes' => 'blubs' } }
 
       it_behaves_like 'allows params'
     end

spec/models/wiki_content_spec.rb

@@ -29,29 +29,22 @@
 require 'spec_helper'
 
 describe WikiContent, type: :model do
-  let(:wiki) { FactoryBot.create(:wiki) }
-  let(:page) { FactoryBot.create(:wiki_page, wiki: wiki) }
   let(:content) { FactoryBot.create(:wiki_content, page: page, author: author) }
-  let(:author) do
+
+  shared_let(:wiki) { FactoryBot.create(:wiki) }
+  shared_let(:page) { FactoryBot.create(:wiki_page, wiki: wiki) }
+  shared_let(:author) do
     FactoryBot.create(:user,
                       member_in_project: wiki.project,
                       member_with_permissions: [:view_wiki_pages])
   end
-  let(:project_watcher) do
+  shared_let(:project_watcher) do
     FactoryBot.create(:user,
                       member_in_project: wiki.project,
                       member_with_permissions: [:view_wiki_pages])
   end
-  let(:page_watcher) do
-    watcher = FactoryBot.create(:user,
-                                member_in_project: wiki.project,
-                                member_with_permissions: [:view_wiki_pages])
-    page.watcher_users << watcher
-
-    watcher
-  end
 
-  let(:wiki_watcher) do
+  shared_let(:wiki_watcher) do
     watcher = FactoryBot.create(:user,
                                 member_in_project: wiki.project,
                                 member_with_permissions: [:view_wiki_pages])
@@ -60,39 +53,134 @@ describe WikiContent, type: :model do
     watcher
   end
 
-  describe '#save (create)' do
-    let(:content) { FactoryBot.build(:wiki_content, page: page) }
+  describe 'mail sending' do
+    context 'when creating' do
+      let(:content) { FactoryBot.build(:wiki_content, page: page) }
+
+      it 'sends mails to the wiki`s watchers and project all watchers' do
+        expect do
+          perform_enqueued_jobs do
+            User.execute_as(author) do
+              content.save!
+            end
+          end
+        end
+          .to change { ActionMailer::Base.deliveries.size }
+                .by(2)
+      end
+    end
 
-    it 'sends mails to the wiki`s watchers and project all watchers' do
-      wiki_watcher
-      project_watcher
+    context 'when updating',
+            with_settings: { journal_aggregation_time_minutes: 0 } do
+      let(:page_watcher) do
+        watcher = FactoryBot.create(:user,
+                                    member_in_project: wiki.project,
+                                    member_with_permissions: [:view_wiki_pages])
+        page.watcher_users << watcher
 
-      expect do
-        perform_enqueued_jobs do
-          content.save!
+        watcher
+      end
+
+      before do
+        page_watcher
+
+        content.text = 'My new content'
+      end
+
+      it 'sends mails to the watchers, the wiki`s watchers and project all watchers' do
+        expect do
+          perform_enqueued_jobs do
+            User.execute_as(author) do
+              content.save!
+            end
+          end
         end
+          .to change { ActionMailer::Base.deliveries.size }
+                .by(3)
       end
-        .to change { ActionMailer::Base.deliveries.size }
-        .by(2)
     end
   end
 
-  describe '#save (update)' do
-    it 'sends mails to the watchers, the wiki`s watchers and project all watchers',
-       with_settings: { journal_aggregation_time_minutes: 0 } do
-      page_watcher
-      wiki_watcher
-      project_watcher
+  describe '#version',
+           with_settings: { journal_aggregation_time_minutes: 0 } do
+    context 'when updating' do
+      it 'updates the version' do
+        content.text = 'My new content'
 
-      content.text = 'My new content'
+        expect { content.save! }
+          .to change(content, :version)
+                .by(1)
+      end
+    end
 
-      expect do
-        perform_enqueued_jobs do
-          content.save!
-        end
+    context 'when creating' do
+      it 'sets the version to 1' do
+        content.save!
+
+        expect(content.version)
+          .to be 1
       end
-        .to change { ActionMailer::Base.deliveries.size }
-        .by(3)
+    end
+
+    context 'when new' do
+      it 'starts with 0' do
+        content = described_class.new(text: 'a', author: author, page: page)
+
+        expect(content.version)
+          .to be 0
+      end
+    end
+  end
+
+  describe '#author' do
+    it 'sets the author' do
+      expect(content.author)
+        .to eql author
+    end
+  end
+
+  describe '#journals',
+           with_settings: { journal_aggregation_time_minutes: 0 } do
+    context 'when creating' do
+      it 'adds a journal' do
+        expect(content.journals.count)
+          .to be 1
+      end
+
+      it 'journalizes the text' do
+        expect(content.journals.last.data.text)
+          .to eql content.text
+      end
+    end
+
+    context 'when updating' do
+      let(:text) { 'My new content' }
+
+      before do
+        content.text = text
+      end
+
+      it 'adds a journal' do
+        expect { content.save! }
+          .to change(content.journals, :count)
+                .by(1)
+      end
+
+      it 'journalizes the text' do
+        content.save!
+
+        expect(content.journals.last.data.text)
+          .to eql content.text
+      end
+    end
+  end
+
+  describe '#text' do
+    it 'doe not truncate to 64k' do
+      content = described_class.create(text: 'a' * 500.kilobyte, author: author, page: page)
+      content.reload
+      expect(content.text.size)
+        .to eql(500.kilobyte)
     end
   end
 end

spec/services/wiki_pages/set_attributes_service_spec.rb

@@ -65,7 +65,8 @@ describe WikiPages::SetAttributesService, type: :model do
       {
         text: 'some new text',
         title: 'a new title',
-        slug: 'a new slug'
+        slug: 'a new slug',
+        journal_notes: 'the journal notes'
       }
     end
 
@@ -82,7 +83,7 @@ describe WikiPages::SetAttributesService, type: :model do
     subject { instance.call(call_attributes) }
 
     it 'is successful' do
-      expect(subject.success?).to be_truthy
+      expect(subject).to be_success
     end
 
     context 'for an existing wiki page' do
@@ -94,6 +95,9 @@ describe WikiPages::SetAttributesService, type: :model do
 
         expect(wiki_page.content.attributes.slice(*wiki_page.content.changed).symbolize_keys)
           .to eql call_attributes.slice(:text)
+
+        expect(wiki_page.content.journal_notes)
+          .to eql call_attributes[:journal_notes]
       end
 
       it 'does not persist the wiki_page' do
@@ -119,6 +123,19 @@ describe WikiPages::SetAttributesService, type: :model do
           .to eql user
       end
 
+      it 'sets the attributes' do
+        subject
+
+        expect(wiki_page.attributes.slice(*wiki_page.changed).symbolize_keys)
+          .to eql call_attributes.slice(:title, :slug)
+
+        expect(wiki_page.content.attributes.slice(*(wiki_page.content.changed - ['author_id'])).symbolize_keys)
+          .to eql call_attributes.slice(:text)
+
+        expect(wiki_page.content.journal_notes)
+          .to eql call_attributes[:journal_notes]
+      end
+
       it 'marks the content author to be system changed' do
         subject
 

spec_legacy/functional/wiki_controller_spec.rb

@@ -85,68 +85,6 @@ describe WikiController, type: :controller do
     assert_template 'new'
   end
 
-  it 'should create page' do
-    session[:user_id] = 2
-    post :create, params: { project_id: 1,
-                            id: 'New page',
-                            content: { comments: 'Created the page',
-                                       text: "h1. New page\n\nThis is a new page",
-                                       page: { title: 'New page',
-                                               parent_id: '' } } }
-    assert_redirected_to action: 'show', project_id: 'ecookbook', id: 'new-page'
-    page = wiki.find_page('New page')
-    assert !page.new_record?
-    refute_nil page.content
-    assert_equal 'Created the page', page.content.last_journal.notes
-  end
-
-  it 'should update page with failure' do
-    session[:user_id] = 2
-    assert_no_difference 'WikiPage.count' do
-      assert_no_difference 'WikiContent.count' do
-        assert_no_difference 'Journal.count' do
-          put :update, params: { project_id: 1,
-                                 id: 'Another page',
-                                 content: {
-                                   comments: 'a' * 300, # failure here, comment is too long
-                                   text: 'edited',
-                                   lock_version: 1,
-                                   page: { title: 'Another page',
-                                           parent_id: '' }
-                                 } }
-        end
-      end
-    end
-    assert_response :success
-    assert_template 'edit'
-
-    assert_error_tag descendant: { content: /Comment is too long/ }
-    assert_select 'textarea', attributes: { id: 'content_text' }, content: /edited/
-    assert_select 'input', attributes: { id: 'content_lock_version', value: '1' }
-  end
-
-  it 'should history' do
-    FactoryBot.create :wiki_content_journal,
-                      journable_id: 1,
-                      data: FactoryBot.build(:journal_wiki_content_journal,
-                                             text: 'h1. CookBook documentation')
-    FactoryBot.create :wiki_content_journal,
-                      journable_id: 1,
-                      data: FactoryBot.build(:journal_wiki_content_journal,
-                                             text: "h1. CookBook documentation\n\n\nSome updated [[documentation]] here...")
-    FactoryBot.create :wiki_content_journal,
-                      journable_id: 1,
-                      data: FactoryBot.build(:journal_wiki_content_journal,
-                                             text: "h1. CookBook documentation\nSome updated [[documentation]] here...")
-
-    get :history, params: { project_id: 1, id: 'CookBook documentation' }
-    assert_response :success
-    assert_template 'history'
-    refute_nil assigns(:versions)
-    assert_equal 3, assigns(:versions).size
-    assert_select 'input[type=submit][name=commit]'
-  end
-
   it 'should history with one version' do
     FactoryBot.create :wiki_content_journal,
                       journable_id: 2,

spec_legacy/unit/wiki_content_spec.rb

@@ -1,91 +0,0 @@
-#-- encoding: UTF-8
-
-#-- copyright
-# OpenProject is an open source project management software.
-# Copyright (C) 2012-2021 the OpenProject GmbH
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License version 3.
-#
-# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
-# Copyright (C) 2006-2013 Jean-Philippe Lang
-# Copyright (C) 2010-2013 the ChiliProject Team
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License
-# as published by the Free Software Foundation; either version 2
-# of the License, or (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-#
-# See COPYRIGHT and LICENSE files for more details.
-#++
-require_relative '../legacy_spec_helper'
-
-describe WikiContent, type: :model do
-  fixtures :all
-
-  before do
-    @wiki = Wiki.find(1)
-    @page = @wiki.pages.first
-  end
-
-  it 'should create' do
-    page = WikiPage.new(wiki: @wiki, title: 'Page')
-    page.content = WikiContent.new(text: 'Content text', author: User.find(1), comments: 'My comment')
-    assert page.save
-    page.reload
-
-    content = page.content
-    assert_kind_of WikiContent, content
-    assert_equal 1, content.version
-    assert_equal 1, content.versions.length
-    assert_equal 'Content text', content.text
-    assert_equal 'My comment', content.versions.last.notes
-    assert_equal User.find(1), content.author
-    assert_equal content.text, content.versions.last.data.text
-  end
-
-  it 'should update' do
-    content = @page.content
-    version_count = content.version
-    content.text = 'My new content'
-    assert content.save
-    content.reload
-    assert_equal version_count + 1, content.version
-    assert_equal version_count + 1, content.versions.length
-  end
-
-  it 'should fetch history' do
-    wiki_content_journal = FactoryBot.create(:wiki_content_journal,
-                                             journable: @page.content)
-    wiki_content_journal.data.update(page_id: @page.id, text: '')
-
-    assert !@page.content.journals.empty?
-    @page.content.journals.each do |journal|
-      assert_kind_of String, journal.data.text
-    end
-  end
-
-  it 'should large text should not be truncated to 64k' do
-    page = WikiPage.new(wiki: @wiki, title: 'Big page')
-    page.content = WikiContent.new(text: 'a' * 500.kilobyte, author: User.find(1))
-    assert page.save
-    page.reload
-    assert_equal 500.kilobyte, page.content.text.size
-  end
-
-  specify 'new WikiContent is version 0' do
-    page = WikiPage.new(wiki: @wiki, title: 'Page')
-    page.content = WikiContent.new(text: 'Content text', author: User.find(1), comments: 'My comment')
-
-    assert_equal 0, page.content.version
-  end
-end