Merge pull request #10854 from opf/impl-42924-display-duration-activity

Implementation 42924 Display duration in the work package activity

app/models/work_package/journalized.rb

@@ -80,7 +80,7 @@ module WorkPackage::Journalized
     register_journal_formatter(:cost_association) do |value, journable, field|
       association = journable.class.reflect_on_association(field.to_sym)
       if association
-        record = association.class_name.constantize.find_by_id(value.to_i)
+        record = association.class_name.constantize.find_by(id: value.to_i)
         record&.subject
       end
     end
@@ -103,5 +103,6 @@ module WorkPackage::Journalized
                                   :author_id, :responsible_id
     register_on_journal_formatter :datetime, :start_date, :due_date
     register_on_journal_formatter :plaintext, :subject
+    register_on_journal_formatter :day_count, :duration
   end
 end

lib_static/plugins/acts_as_journalized/lib/journal_changes.rb

@@ -37,6 +37,8 @@ module JournalChanges
                  subsequent_journal_data_changes
                end
 
+    @changes.delete(:duration) unless OpenProject::FeatureDecisions.work_packages_duration_field_active?
+
     @changes.merge!(get_association_changes(predecessor, 'attachable', 'attachments', :attachment_id, :filename))
     @changes.merge!(get_association_changes(predecessor, 'customizable', 'custom_fields', :custom_field_id, :value))
   end
@@ -46,7 +48,7 @@ module JournalChanges
   def initial_journal_data_changes
     data
      .journaled_attributes
-     .reject { |_, new_value| new_value.nil? }
+     .compact
      .inject({}) do |result, (attribute, new_value)|
       result[attribute] = [nil, new_value]
       result
@@ -59,8 +61,7 @@ module JournalChanges
 
     normalized_new_data
       .select { |attribute, new_value| no_nil_to_empty_strings?(normalized_old_data, attribute, new_value) }
-      .map { |attribute, new_value| [attribute, [normalized_old_data[attribute], new_value]] }
-      .to_h
+      .to_h { |attribute, new_value| [attribute, [normalized_old_data[attribute], new_value]] }
       .with_indifferent_access
   end
 

lib_static/plugins/acts_as_journalized/lib/journal_formatter.rb

@@ -52,6 +52,7 @@
 require_relative './journal_formatter/base'
 require_relative './journal_formatter/attribute'
 require_relative './journal_formatter/datetime'
+require_relative './journal_formatter/day_count'
 require_relative './journal_formatter/decimal'
 require_relative './journal_formatter/fraction'
 require_relative './journal_formatter/id'
@@ -79,12 +80,13 @@ module JournalFormatter
   end
 
   def self.default_formatters
-    { plaintext: JournalFormatter::Plaintext,
-      datetime: JournalFormatter::Datetime,
-      named_association: JournalFormatter::NamedAssociation,
-      fraction: JournalFormatter::Fraction,
+    { datetime: JournalFormatter::Datetime,
+      day_count: JournalFormatter::DayCount,
       decimal: JournalFormatter::Decimal,
-      id: JournalFormatter::Id }
+      fraction: JournalFormatter::Fraction,
+      id: JournalFormatter::Id,
+      named_association: JournalFormatter::NamedAssociation,
+      plaintext: JournalFormatter::Plaintext }
   end
 
   self.formatters = default_formatters

lib_static/plugins/acts_as_journalized/lib/journal_formatter/day_count.rb

@@ -0,0 +1,35 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2022 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.
+#++
+
+module JournalFormatter
+  class DayCount < Attribute
+    def format_values(values)
+      values.map { |v| "#{v.to_i} #{'day'.pluralize(v.to_i)}" unless v.nil? }
+    end
+  end
+end

spec/lib/journal_formatter/day_count_spec.rb

@@ -0,0 +1,73 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2022 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 File.expand_path("#{File.dirname(__FILE__)}/../../spec_helper.rb")
+
+describe JournalFormatter::DayCount do
+  let(:klass) { described_class }
+  let(:id) { 1 }
+  let(:journal) do
+    instance_double(Journal, journable: WorkPackage.new)
+  end
+  let(:instance) { klass.new(journal) }
+  let(:key) { :duration }
+
+  describe '#render' do
+    describe 'when setting the old value to 1 day, and the new value to 3 days' do
+      let(:expected) do
+        I18n.t(:text_journal_changed_html,
+               label: "<strong>Duration</strong>",
+               old: '<i title="1 day">1 day</i>',
+               new: '<i title="3 days">3 days</i>',
+               linebreak: '')
+      end
+
+      it { expect(instance.render(key, [1, 3])).to eq(expected) }
+    end
+
+    describe 'when setting the initial value to 3 days' do
+      let(:expected) do
+        I18n.t(:text_journal_set_to,
+               label: "<strong>Duration</strong>",
+               value: '<i title="3 days">3 days</i>')
+      end
+
+      it { expect(instance.render(key, [nil, 3])).to eq(expected) }
+    end
+
+    describe 'when deleting the initial value of 3 days' do
+      let(:expected) do
+        I18n.t(:text_journal_deleted,
+               label: "<strong>Duration</strong>",
+               old: '<strike><i title="3 days">3 days</i></strike>')
+      end
+
+      it { expect(instance.render(key, [3, nil])).to eq(expected) }
+    end
+  end
+end

spec/models/mail_handler_spec.rb

@@ -50,7 +50,7 @@ describe MailHandler, type: :model do
     allow(Setting).to receive(:default_language).and_return('en')
   end
 
-  shared_context 'wp_on_given_project' do
+  shared_context 'for wp_on_given_project' do
     let(:permissions) { %i[add_work_packages assign_versions work_package_assigned] }
     let!(:user) do
       create(:user,
@@ -67,7 +67,7 @@ describe MailHandler, type: :model do
     end
   end
 
-  shared_context 'wp_on_given_project_case_insensitive' do
+  shared_context 'for wp_on_given_project_case_insensitive' do
     let(:permissions) { %i[add_work_packages assign_versions] }
     let!(:user) do
       create(:user,
@@ -100,7 +100,7 @@ describe MailHandler, type: :model do
              member_in_project: project,
              member_with_permissions: [:work_package_assigned])
     end
-    let(:submit_options) { { } }
+    let(:submit_options) { {} }
 
     subject do
       submit_email('wp_on_given_project_group_assignment.eml', **submit_options)
@@ -274,11 +274,11 @@ describe MailHandler, type: :model do
     end
 
     context 'when sending a mail not as a reply' do
-      context 'in a given project' do
+      context 'for a given project' do
         let!(:status) { create(:status, name: 'Resolved') }
         let!(:version) { create(:version, name: 'alpha', project:) }
 
-        include_context 'wp_on_given_project' do
+        include_context 'for wp_on_given_project' do
           let(:submit_options) { { allow_override: 'version' } }
         end
 
@@ -380,7 +380,7 @@ describe MailHandler, type: :model do
           end
         end
 
-        include_context 'wp_on_given_project' do
+        include_context 'for wp_on_given_project' do
           let(:submit_options) { { issue: { type: default_type.name } } }
         end
 
@@ -399,7 +399,7 @@ describe MailHandler, type: :model do
           user.locked!
         end
 
-        include_context 'wp_on_given_project'
+        include_context 'for wp_on_given_project'
 
         it 'does not create the work package' do
           expect { subject }
@@ -426,7 +426,7 @@ describe MailHandler, type: :model do
             expect do
               work_package = submit_email('ticket_by_unknown_user.eml', issue: { project: 'onlinestore' }, unknown_user: 'create')
               work_package_created(work_package)
-              expect(work_package.author.active?).to be_truthy
+              expect(work_package.author).to be_active
               expect(work_package.author.mail).to eq('john.doe@somenet.foo')
               expect(work_package.author.firstname).to eq('John')
               expect(work_package.author.lastname).to eq('Doe')
@@ -441,9 +441,9 @@ describe MailHandler, type: :model do
               password = email.body.encoded.match(/\* Password: (\S+)\s?$/)[1]
 
               # Can't log in here since randomly assigned password must be changed
-              found_user = User.find_by_login(login)
+              found_user = User.find_by(login:)
               expect(work_package.author).to eq(found_user)
-              expect(found_user.check_password?(password)).to be_truthy
+              expect(found_user).to be_check_password(password)
             end.to change(User, :count).by(1)
           end
         end
@@ -471,11 +471,11 @@ describe MailHandler, type: :model do
         context 'with unknown_user: \'accept\' and permission check present' do
           let(:expected) do
             'MailHandler: work_package could not be created by Anonymous due to ' \
-            '#["may not be accessed.", ' \
-            '"Type was attempted to be written but is not writable.", ' \
-            '"Project was attempted to be written but is not writable.", ' \
-            '"Subject was attempted to be written but is not writable.", ' \
-            '"Description was attempted to be written but is not writable."]'
+              '#["may not be accessed.", ' \
+              '"Type was attempted to be written but is not writable.", ' \
+              '"Project was attempted to be written but is not writable.", ' \
+              '"Subject was attempted to be written but is not writable.", ' \
+              '"Description was attempted to be written but is not writable."]'
           end
           let(:permission) { nil }
 
@@ -607,7 +607,7 @@ describe MailHandler, type: :model do
         end
       end
 
-      context 'email from emission address', with_settings: { mail_from: 'openproject@example.net' } do
+      context 'for email from emission address', with_settings: { mail_from: 'openproject@example.net' } do
         before do
           Role.non_member.add_permission!(:add_work_packages)
         end
@@ -639,11 +639,11 @@ describe MailHandler, type: :model do
         end
       end
 
-      context 'wp with status' do
+      context 'for wp with status' do
         let!(:status) { create(:status, name: 'Resolved') }
 
         # This email contains: 'Project: onlinestore' and 'Status: Resolved'
-        include_context 'wp_on_given_project'
+        include_context 'for wp_on_given_project'
 
         it_behaves_like 'work package created'
 
@@ -653,13 +653,13 @@ describe MailHandler, type: :model do
         end
       end
 
-      context 'wp with status case insensitive' do
+      context 'for wp with status case insensitive' do
         let!(:status) { create(:status, name: 'Resolved') }
         let!(:priority_low) { create(:priority_low, name: 'Low', is_default: true) }
         let!(:version) { create(:version, name: 'alpha', project:) }
 
         # This email contains: 'Project: onlinestore' and 'Status: resolved'
-        include_context 'wp_on_given_project_case_insensitive'
+        include_context 'for wp_on_given_project_case_insensitive'
 
         it_behaves_like 'work package created'
 
@@ -670,7 +670,7 @@ describe MailHandler, type: :model do
         end
       end
 
-      context 'wp with cc' do
+      context 'for wp with cc' do
         include_context 'with wp create with cc'
 
         it_behaves_like 'work package created'
@@ -785,7 +785,7 @@ describe MailHandler, type: :model do
             .to include 'The text of the reply.'
         end
 
-        it 'alters the attributes' do
+        it 'alters the attributes', with_flag: { work_packages_duration_field_active: true } do
           subject
 
           expect(work_package.journals.reload.last.details)
@@ -815,13 +815,13 @@ describe MailHandler, type: :model do
           type.custom_fields << custom_field
           type.save!
 
-          allow_any_instance_of(WorkPackage).to receive(:available_custom_fields).and_return([custom_field])
+          allow(work_package).to receive(:available_custom_fields).and_return([custom_field])
 
           allow(WorkPackage).to receive(:find_by).with(id: 42).and_return(work_package)
           allow(User).to receive(:find_by_mail).with("h.wurst@openproject.com").and_return(mail_user)
         end
 
-        context 'of type text' do
+        context 'as type text' do
           let(:custom_field) { create :text_wp_custom_field, name: "Notes" }
 
           before do
@@ -831,13 +831,13 @@ describe MailHandler, type: :model do
           end
 
           it "sets the value" do
-            value = work_package.custom_values.where(custom_field_id: custom_field.id).pluck(:value).first
+            value = work_package.custom_values.where(custom_field_id: custom_field.id).pick(:value)
 
             expect(value).to eq "some text" # as given in .eml fixture
           end
         end
 
-        context 'of type list' do
+        context 'as type list' do
           let(:custom_field) { create :list_wp_custom_field, name: "Letters", possible_values: %w(A B C) }
 
           before do
@@ -848,7 +848,7 @@ describe MailHandler, type: :model do
 
           it "sets the value" do
             option = CustomOption.where(custom_field_id: custom_field.id, value: "B").first # as given in .eml fixture
-            value = work_package.custom_values.where(custom_field_id: custom_field.id).pluck(:value).first
+            value = work_package.custom_values.where(custom_field_id: custom_field.id).pick(:value)
 
             expect(value).to eq option.id.to_s
           end
@@ -880,9 +880,9 @@ describe MailHandler, type: :model do
       end
     end
 
-    context 'truncate emails based on the Setting' do
+    describe 'truncate emails based on the Setting' do
       context 'with no setting', with_settings: { mail_handler_body_delimiters: '' } do
-        include_context 'wp_on_given_project'
+        include_context 'for wp_on_given_project'
 
         it_behaves_like 'work package created'
 
@@ -896,7 +896,7 @@ describe MailHandler, type: :model do
       end
 
       context 'with a single string', with_settings: { mail_handler_body_delimiters: '---' } do
-        include_context 'wp_on_given_project'
+        include_context 'for wp_on_given_project'
 
         it_behaves_like 'work package created'
 
@@ -935,7 +935,7 @@ describe MailHandler, type: :model do
 
       context 'with multiple strings',
               with_settings: { mail_handler_body_delimiters: "---\nBREAK" } do
-        include_context 'wp_on_given_project'
+        include_context 'for wp_on_given_project'
 
         it_behaves_like 'work package created'
 
@@ -981,27 +981,26 @@ describe MailHandler, type: :model do
       "Subject:foo\nDescription:bar\n" \
         ">>> myserver.example.org 2016-01-27 15:56 >>>\n... (Email-Body) ..."
     end
-    let(:handler) { MailHandler.send :new }
+    let(:handler) { described_class.send :new }
 
     context 'with regex delimiter' do
       before do
         allow(Setting).to receive(:mail_handler_body_delimiter_regex).and_return('>>>.+?>>>.*')
         allow(handler).to receive(:plain_text_body).and_return(input)
-        expect(handler).to receive(:cleaned_up_text_body).and_call_original
+        allow(handler).to receive(:cleaned_up_text_body).and_call_original
       end
 
       it 'removes the irrelevant lines' do
         expect(handler.send(:cleaned_up_text_body)).to eq("Subject:foo\nDescription:bar")
+        expect(handler).to have_received(:cleaned_up_text_body)
       end
     end
   end
 
   private
 
-  FIXTURES_PATH = File.dirname(__FILE__) + '/../fixtures/mail_handler'
-
   def read_email(filename)
-    IO.read(File.join(FIXTURES_PATH, filename))
+    File.read(File.join("#{File.dirname(__FILE__)}/../fixtures/mail_handler", filename))
   end
 
   def submit_email(filename, options = {})
@@ -1009,7 +1008,7 @@ describe MailHandler, type: :model do
   end
 
   def work_package_created(work_package)
-    expect(work_package.is_a?(WorkPackage)).to be_truthy
+    expect(work_package).to be_a(WorkPackage)
     expect(work_package).not_to be_new_record
     work_package.reload
   end

spec/models/work_package/work_package_acts_as_journalized_spec.rb

@@ -43,7 +43,8 @@ describe WorkPackage, type: :model do
              type:,
              description: 'Description',
              priority:,
-             status:)
+             status:,
+             duration: 1)
     end
     let(:current_user) { create(:user) }
 
@@ -53,7 +54,7 @@ describe WorkPackage, type: :model do
       work_package
     end
 
-    context 'on work package creation' do
+    context 'for work package creation' do
       it { expect(Journal.all.count).to eq(1) }
 
       it 'has a journal entry' do
@@ -89,18 +90,14 @@ describe WorkPackage, type: :model do
       end
     end
 
-    context 'nothing is changed' do
-      before do
-        work_package.save!
-      end
-
-      it { expect(Journal.all.count).to eq(1) }
+    context 'when nothing is changed' do
+      it { expect { work_package.save! }.not_to change(Journal, :count) }
     end
 
-    context 'different newlines', with_settings: { journal_aggregation_time_minutes: 0 } do
+    context 'for different newlines', with_settings: { journal_aggregation_time_minutes: 0 } do
       let(:description) { "Description\n\nwith newlines\n\nembedded" }
       let(:changed_description) { description.gsub("\n", "\r\n") }
-      let!(:work_package_1) do
+      let!(:work_package1) do
         create(:work_package,
                project_id: project.id,
                type:,
@@ -109,23 +106,23 @@ describe WorkPackage, type: :model do
       end
 
       before do
-        work_package_1.description = changed_description
+        work_package1.description = changed_description
       end
 
       context 'when a new journal is created tracking a simultaneously applied change' do
         before do
-          work_package_1.subject += 'changed'
-          work_package_1.save!
+          work_package1.subject += 'changed'
+          work_package1.save!
         end
 
         describe 'does not track the changed newline characters' do
-          subject { work_package_1.journals.last.data.description }
+          subject { work_package1.journals.last.data.description }
 
           it { is_expected.to eq(description) }
         end
 
         describe 'tracks only the other change' do
-          subject { work_package_1.journals.last.details }
+          subject { work_package1.journals.last.details }
 
           it { is_expected.to have_key :subject }
           it { is_expected.not_to have_key :description }
@@ -133,48 +130,49 @@ describe WorkPackage, type: :model do
       end
 
       context 'when there is a legacy journal containing non-escaped newlines' do
-        let!(:work_package_journal_1) do
+        let!(:work_package_journal1) do
           create(:work_package_journal,
-                 journable_id: work_package_1.id,
+                 journable_id: work_package1.id,
                  version: 2,
                  data: build(:journal_work_package_journal,
                              description:))
         end
-        let!(:work_package_journal_2) do
+        let!(:work_package_journal2) do
           create(:work_package_journal,
-                 journable_id: work_package_1.id,
+                 journable_id: work_package1.id,
                  version: 3,
                  data: build(:journal_work_package_journal,
                              description: changed_description))
         end
 
-        subject { work_package_1.journals.reload.last.details }
+        subject { work_package1.journals.reload.last.details }
 
         it { is_expected.not_to have_key :description }
       end
     end
 
-    context 'on work package change', with_settings: { journal_aggregation_time_minutes: 0 } do
+    describe 'on work package change', with_settings: { journal_aggregation_time_minutes: 0 } do
       let(:parent_work_package) do
         create(:work_package,
                project_id: project.id,
                type:,
                priority:)
       end
-      let(:type_2) { create :type }
-      let(:status_2) { create :status }
-      let(:priority_2) { create :priority }
+      let(:type2) { create :type }
+      let(:status2) { create :status }
+      let(:priority2) { create :priority }
 
       before do
-        project.types << type_2
+        project.types << type2
 
         work_package.subject = 'changed'
         work_package.description = 'changed'
-        work_package.type = type_2
-        work_package.status = status_2
-        work_package.priority = priority_2
+        work_package.type = type2
+        work_package.status = status2
+        work_package.priority = priority2
         work_package.start_date = Date.new(2013, 1, 24)
         work_package.due_date = Date.new(2013, 1, 31)
+        work_package.duration = 8
         work_package.estimated_hours = 40.0
         work_package.assigned_to = User.current
         work_package.responsible = User.current
@@ -184,16 +182,26 @@ describe WorkPackage, type: :model do
         work_package.save!
       end
 
-      context 'last created journal' do
+      context 'for last created journal', with_flag: { work_packages_duration_field_active: true } do
         subject { work_package.journals.reload.last.details }
 
         it 'contains all changes' do
           %i(subject description type_id status_id priority_id
              start_date due_date estimated_hours assigned_to_id
-             responsible_id parent_id schedule_manually).each do |a|
+             responsible_id parent_id schedule_manually duration).each do |a|
             expect(subject).to have_key(a.to_s), "Missing change for #{a}"
           end
         end
+
+        context 'when duration feature flag is inactive', with_flag: { work_packages_duration_field_active: false } do
+          it 'contains all changes' do
+            %i(subject description type_id status_id priority_id
+               start_date due_date estimated_hours assigned_to_id
+               responsible_id parent_id schedule_manually).each do |a|
+              expect(subject).to have_key(a.to_s), "Missing change for #{a}"
+            end
+          end
+        end
       end
 
       shared_examples_for 'old value' do
@@ -209,37 +217,69 @@ describe WorkPackage, type: :model do
       end
 
       describe 'journaled value for' do
-        context 'description' do
+        describe 'description' do
           let(:property) { 'description' }
 
-          context 'old_value' do
+          context 'for old value' do
             let(:expected_value) { 'Description' }
 
             it_behaves_like 'old value'
           end
 
-          context 'new value' do
+          context 'for new value' do
             let(:expected_value) { 'changed' }
 
             it_behaves_like 'new value'
           end
         end
 
-        context 'schedule_manually' do
+        describe 'schedule_manually' do
           let(:property) { 'schedule_manually' }
 
-          context 'old_value' do
+          context 'for old value' do
             let(:expected_value) { false }
 
             it_behaves_like 'old value'
           end
 
-          context 'new value' do
+          context 'for new value' do
             let(:expected_value) { true }
 
             it_behaves_like 'new value'
           end
         end
+
+        describe 'duration', with_flag: { work_packages_duration_field_active: true } do
+          let(:property) { 'duration' }
+
+          context 'for old value' do
+            let(:expected_value) { 1 }
+
+            it_behaves_like 'old value'
+          end
+
+          context 'for new value' do
+            let(:expected_value) { 8 }
+
+            it_behaves_like 'new value'
+          end
+
+          context 'when duration feature flag is inactive', with_flag: { work_packages_duration_field_active: false } do
+            let(:property) { 'duration' }
+
+            context 'for old value' do
+              let(:expected_value) { nil }
+
+              it_behaves_like 'old value'
+            end
+
+            context 'for new value' do
+              let(:expected_value) { nil }
+
+              it_behaves_like 'new value'
+            end
+          end
+        end
       end
 
       describe 'adding journal with a missing journal and an existing journal' do
@@ -271,7 +311,7 @@ describe WorkPackage, type: :model do
       end
     end
 
-    context 'attachments', with_settings: { journal_aggregation_time_minutes: 0 } do
+    describe 'attachments', with_settings: { journal_aggregation_time_minutes: 0 } do
       let(:attachment) { build :attachment }
       let(:attachment_id) { "attachments_#{attachment.id}" }
 
@@ -280,7 +320,7 @@ describe WorkPackage, type: :model do
         work_package.save!
       end
 
-      context 'new attachment' do
+      context 'for new attachment' do
         subject { work_package.journals.reload.last.details }
 
         it { is_expected.to have_key attachment_id }
@@ -288,20 +328,12 @@ describe WorkPackage, type: :model do
         it { expect(subject[attachment_id]).to eq([nil, attachment.filename]) }
       end
 
-      context 'attachment saved w/o change' do
-        before do
-          @original_journal_count = work_package.journals.reload.count
-
-          attachment.save!
-        end
-
-        subject { work_package.journals.reload.count }
-
-        it { is_expected.to eq(@original_journal_count) }
+      context 'when attachment saved w/o change' do
+        it { expect { attachment.save! }.not_to change(Journal, :count) }
       end
     end
 
-    context 'custom values', with_settings: { journal_aggregation_time_minutes: 0 } do
+    describe 'custom values', with_settings: { journal_aggregation_time_minutes: 0 } do
       let(:custom_field) { create :work_package_custom_field }
       let(:custom_value) do
         build :custom_value,
@@ -311,7 +343,7 @@ describe WorkPackage, type: :model do
 
       let(:custom_field_id) { "custom_fields_#{custom_value.custom_field_id}" }
 
-      shared_context 'work package with custom value' do
+      shared_context 'for work package with custom value' do
         before do
           project.work_package_custom_fields << custom_field
           type.custom_fields << custom_field
@@ -321,8 +353,8 @@ describe WorkPackage, type: :model do
         end
       end
 
-      context 'new custom value' do
-        include_context 'work package with custom value'
+      context 'for new custom value' do
+        include_context 'for work package with custom value'
 
         subject { work_package.journals.reload.last.details }
 
@@ -331,8 +363,8 @@ describe WorkPackage, type: :model do
         it { expect(subject[custom_field_id]).to eq([nil, custom_value.value]) }
       end
 
-      context 'custom value modified' do
-        include_context 'work package with custom value'
+      context 'for custom value modified' do
+        include_context 'for work package with custom value'
 
         let(:modified_custom_value) do
           create :custom_value,
@@ -352,8 +384,8 @@ describe WorkPackage, type: :model do
         it { expect(subject[custom_field_id]).to eq([custom_value.value.to_s, modified_custom_value.value.to_s]) }
       end
 
-      context 'work package saved w/o change' do
-        include_context 'work package with custom value'
+      context 'when work package saved w/o change' do
+        include_context 'for work package with custom value'
 
         let(:unmodified_custom_value) do
           create :custom_value,
@@ -362,20 +394,14 @@ describe WorkPackage, type: :model do
         end
 
         before do
-          @original_journal_count = work_package.journals.reload.count
-
           work_package.custom_values = [unmodified_custom_value]
-
-          work_package.save!
         end
 
-        subject { work_package.journals.reload.count }
-
-        it { is_expected.to eq(@original_journal_count) }
+        it { expect { work_package.save! }.not_to change(Journal, :count) }
       end
 
-      context 'custom value removed' do
-        include_context 'work package with custom value'
+      context 'when custom value removed' do
+        include_context 'for work package with custom value'
 
         before do
           work_package.custom_values.delete(custom_value)
@@ -389,7 +415,7 @@ describe WorkPackage, type: :model do
         it { expect(subject[custom_field_id]).to eq([custom_value.value, nil]) }
       end
 
-      context 'custom value did not exist before' do
+      context 'when custom value did not exist before' do
         let(:custom_field) do
           create :work_package_custom_field,
                  is_required: false,
@@ -404,20 +430,20 @@ describe WorkPackage, type: :model do
         end
 
         describe 'empty values are recognized as unchanged' do
-          include_context 'work package with custom value'
+          include_context 'for work package with custom value'
 
           it { expect(work_package.journals.reload.last.customizable_journals).to be_empty }
         end
 
         describe 'empty values handled as non existing' do
-          include_context 'work package with custom value'
+          include_context 'for work package with custom value'
 
           it { expect(work_package.journals.reload.last.customizable_journals.count).to eq(0) }
         end
       end
     end
 
-    context 'on only journal notes adding' do
+    context 'for only journal notes adding' do
       before do
         work_package.add_journal(User.current, 'some notes')
         work_package.save
@@ -432,7 +458,7 @@ describe WorkPackage, type: :model do
       end
     end
 
-    context 'on mixed journal notes and attribute adding' do
+    context 'for mixed journal notes and attribute adding' do
       before do
         work_package.add_journal(User.current, 'some notes')
         work_package.subject = 'blubs'
@@ -448,7 +474,7 @@ describe WorkPackage, type: :model do
       end
     end
 
-    context 'updated within aggregation time' do
+    context 'when updated within aggregation time' do
       subject(:journals) { work_package.journals }
 
       let(:current_user) { user1 }
@@ -471,7 +497,7 @@ describe WorkPackage, type: :model do
         work_package.save!
       end
 
-      context 'by author of last change' do
+      context 'as author of last change' do
         let(:new_author) { user1 }
 
         it 'leads to a single journal' do
@@ -479,7 +505,7 @@ describe WorkPackage, type: :model do
         end
 
         it 'is the initial journal' do
-          expect(subject.first.initial?).to be_truthy
+          expect(subject.first).to be_initial
         end
 
         it 'contains the changes of both updates with the later overwriting the former' do
@@ -514,8 +540,8 @@ describe WorkPackage, type: :model do
             end
 
             it 'has one initial journal and one non-initial journal' do
-              expect(subject.first.initial?).to be_truthy
-              expect(subject.second.initial?).to be_falsey
+              expect(subject.first).to be_initial
+              expect(subject.second).not_to be_initial
             end
           end
 
@@ -541,7 +567,7 @@ describe WorkPackage, type: :model do
         end
       end
 
-      context 'by a different author' do
+      context 'as a different author' do
         let(:new_author) { user2 }
 
         it 'leads to two journals' do
@@ -558,12 +584,12 @@ describe WorkPackage, type: :model do
       end
     end
 
-    context 'updated after aggregation timeout expired', with_settings: { journal_aggregation_time_minutes: 1 } do
+    context 'when updated after aggregation timeout expired', with_settings: { journal_aggregation_time_minutes: 1 } do
       subject(:journals) { work_package.journals }
 
       before do
-        work_package.journals.last.update_columns(created_at: Time.now - 2.minutes,
-                                                  updated_at: Time.now - 2.minutes)
+        work_package.journals.last.update_columns(created_at: 2.minutes.ago,
+                                                  updated_at: 2.minutes.ago)
 
         work_package.status = build(:status)
         work_package.save!
@@ -574,10 +600,10 @@ describe WorkPackage, type: :model do
       end
     end
 
-    context 'updating with aggregation disabled', with_settings: { journal_aggregation_time_minutes: 0 } do
+    context 'when updating with aggregation disabled', with_settings: { journal_aggregation_time_minutes: 0 } do
       subject(:journals) { work_package.journals }
 
-      context 'WP updated within milliseconds' do
+      context 'when WP updated within milliseconds' do
         before do
           work_package.status = build(:status)
           work_package.save!
@@ -590,7 +616,7 @@ describe WorkPackage, type: :model do
     end
   end
 
-  context 'on #destroy' do
+  describe 'on #destroy' do
     let(:project) { create(:project) }
     let(:type) { create(:type) }
     let(:custom_field) do