diff --git a/app/workers/application_job.rb b/app/workers/application_job.rb index 44c81ab93723..c2197a5f15bd 100644 --- a/app/workers/application_job.rb +++ b/app/workers/application_job.rb @@ -30,13 +30,7 @@ class ApplicationJob < ActiveJob::Base include ::JobStatus::ApplicationJobWithStatus - - ## - # By default, do not log the arguments of a background job - # to avoid leaking sensitive information to logs - self.log_arguments = false - - around_perform :prepare_job_context + include SharedJobSetup ## # Return a priority number on the given payload @@ -65,45 +59,7 @@ def self.queue_with_priority(value = :default) end end - # Resets the thread local request store. - # This should be done, because normal application code expects the RequestStore to be - # invalidated between multiple requests and does usually not care whether it is executed - # from a request or from a delayed job. - # For a delayed job, each job execution is the thing that comes closest to - # the concept of a new request. - def with_clean_request_store - store = RequestStore.store - - begin - RequestStore.clear! - yield - ensure - # Reset to previous value - RequestStore.clear! - RequestStore.store.merge! store - end - end - - # Reloads the thread local ActionMailer configuration. - # Since the email configuration is now done in the web app, we need to - # make sure that any changes to the configuration is correctly picked up - # by the background jobs at runtime. - def reload_mailer_settings! - Setting.reload_mailer_settings! - end - def job_scheduled_at GoodJob::Job.where(id: job_id).pick(:scheduled_at) end - - private - - def prepare_job_context - with_clean_request_store do - ::OpenProject::Appsignal.tag_request - reload_mailer_settings! - - yield - end - end end diff --git a/app/workers/mails/mailer_job.rb b/app/workers/mails/mailer_job.rb index beaed7c77fae..696daa79ec4a 100644 --- a/app/workers/mails/mailer_job.rb +++ b/app/workers/mails/mailer_job.rb @@ -26,46 +26,44 @@ # See COPYRIGHT and LICENSE files for more details. #++ -## -# This job gets called when internally using +# OpenProject is configured to use this job when sending emails like this: # # ``` # UserMailer.some_mail("some param").deliver_later # ``` # -# because we want to have the sending of the email run in an `ApplicationJob` -# as opposed to using `ActiveJob::QueueAdapters::DelayedJobAdapter::JobWrapper`. -# We want it to run in an `ApplicationJob` because of the shared setup required -# such as reloading the mailer configuration and resetting the request store. -class Mails::MailerJob < ApplicationJob - queue_as { ActionMailer::Base.deliver_later_queue_name } - - # Retry mailing jobs three times with polinomial backoff - retry_on StandardError, wait: :polynomially_longer, attempts: 3 - - # If exception is handled in mail handler - # retry_on will be ignored - rescue_from StandardError, with: :handle_exception_with_mailer_class - - def perform(mailer, mail_method, delivery, args:) - mailer.constantize.public_send(mail_method, *args).send(delivery) - end - - private - - # "Deserialize" the mailer class name by hand in case another argument - # (like a Global ID reference) raised DeserializationError. - def mailer_class - if mailer = Array(@serialized_arguments).first || Array(arguments).first - mailer.constantize - end - end +# This job is used because all our `XxxMailer` classes inherit from +# `ApplicationMailer`, and `ApplicationMailer.delivery_job` is set to +# `::Mails::MailerJob`. +# +# The `delivery_job` is customized to add the shared job setup required for +# OpenProject such as reloading the mailer configuration and resetting the +# request store on each job execution. +# +# It also adds retry logic to the job. +class Mails::MailerJob < ActionMailer::MailDeliveryJob + include SharedJobSetup - def handle_exception_with_mailer_class(exception) - if klass = mailer_class - klass.handle_exception exception - else - raise exception - end - end + # Retry mailing jobs 14 times with polynomial backoff (retries for ~ 1.5 days). + # + # with polynomial backoff, the formula to get wait_duration is: + # + # ((executions**4) + (Kernel.rand * (executions**4) * jitter)) + 2 + # + # as the default jitter is 0.0, the formula becomes: + # + # ((executions**4) + 2) + # + # To get the numbers, run this: + # + # (1..20).reduce(0) do |total_wait, i| + # wait = (i**4) + 2 + # total_wait += wait + # puts "Execution #{i} waits #{wait} secs. Total wait: #{total_wait} secs" + # total_wait + # end + # + # We set attemps to 14 to have it retry for 127715 seconds which is more than + # 1 day (~= 1 day 11 hours 30 min) + retry_on StandardError, wait: :polynomially_longer, attempts: 14 end diff --git a/app/workers/shared_job_setup.rb b/app/workers/shared_job_setup.rb new file mode 100644 index 000000000000..21019b136b1d --- /dev/null +++ b/app/workers/shared_job_setup.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +# Shared setup for jobs. +# +# This module is included in `ApplicationJob` and `Mails::MailerJob` and does +# the following: +# +# - disable logging of arguments +# - before each job execution: +# - reloads the mailer settings +# - resets the request store +# - tags the request for AppSignal +module SharedJobSetup + extend ActiveSupport::Concern + + included do + # By default, do not log the arguments of a background job + # to avoid leaking sensitive information to logs + self.log_arguments = false + + around_perform :prepare_job_context + end + + # Prepare the job execution by cleaning the request store, reloading the + # mailer settings and tagging the request + def prepare_job_context + with_clean_request_store do + ::OpenProject::Appsignal.tag_request + reload_mailer_settings! + + yield + end + end + + # Resets the thread local request store. + # + # This should be done, because normal application code expects the + # RequestStore to be invalidated between multiple requests and does usually + # not care whether it is executed from a request or from a job. + # + # For a job, each job execution is the thing that comes closest to the concept + # of a new request. + def with_clean_request_store + store = RequestStore.store + + begin + RequestStore.clear! + yield + ensure + # Reset to previous value + RequestStore.clear! + RequestStore.store.merge! store + end + end + + # Reloads the thread local ActionMailer configuration. + # + # Since the email configuration is done in the web app, it makes sure that any + # changes to the configuration is correctly picked up by the background jobs + # at runtime. + def reload_mailer_settings! + Setting.reload_mailer_settings! + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 613ada194fdf..0d88e67097f2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -897,7 +897,7 @@ en: blank: "can't be blank." blank_nested: "needs to have the property '%{property}' set." cannot_delete_mapping: "is required. Cannot be deleted." - is_for_all_cannot_modify: "is for all. Cannot be modified." + is_for_all_cannot_modify: "is for all projects and can therefore not be modified." cant_link_a_work_package_with_a_descendant: "A work package cannot be linked to one of its subtasks." circular_dependency: "This relation would create a circular dependency." confirmation: "doesn't match %{attribute}." diff --git a/docs/release-notes/14-6-0/README.md b/docs/release-notes/14-6-0/README.md index f1200affcc95..e08d50f6fbdf 100644 --- a/docs/release-notes/14-6-0/README.md +++ b/docs/release-notes/14-6-0/README.md @@ -56,7 +56,7 @@ Macros such as *workPackageValue:assignee* have been implemented, allowing the d ![Screenshot showing a work package with macros in the description](openproject-14-6-macros.png) -See our [documentation for a list of available attributes for work packages](../../user-guide/wysiwyg/#available-attributes-for-work-packages). +See our [documentation for a list of available attributes for work packages](../../user-guide/wysiwyg/#available-attributes-for-work-packages) and take a look at [this blog article to learn more about using macros with OpenProject](https://www.openproject.org/blog/how-to-use-macros/). ### Show empty lines in saved rich text @@ -138,6 +138,10 @@ On the Meetings tab in the split screen view, the number next to the “Meetings A very special thank you goes to City of Cologne again for sponsoring features in project lists. Also, a big thanks to our Community members for reporting bugs and helping us identify and provide fixes. Special thanks for reporting and finding bugs go to Jan H, Joris Ceelen, André van Kaam, and Christian Jeschke. -Last but not least, we are very grateful for our very engaged translation contributors on Crowdin, who translated quite a few OpenProject strings! This release, we would like to highlight [DKrukoff](https://crowdin.com/profile/dkrukoff), for an outstanding number of translations into Russian. +Last but not least, we are very grateful for our very engaged translation contributors on Crowdin, who translated quite a few OpenProject strings! This release we would like to highlight +- [DKrukoff](https://crowdin.com/profile/dkrukoff), for translations into Russian. +- [Sara Ruela](https://crowdin.com/profile/Sara.PT), for translations into Portuguese. +- [BigSeung](https://crowdin.com/profile/BigSeung), for translations into Korean. +- [Raffaele Brevetti](https://crowdin.com/profile/rbrevetti), for translations into Italian. Would you like to help out with translations yourself? Then take a look at our [translation guide](../../development/translate-openproject/) and find out exactly how you can contribute. It is very much appreciated! \ No newline at end of file diff --git a/lookbook/docs/patterns/02-forms.md.erb b/lookbook/docs/patterns/02-forms.md.erb index 81a134ef375c..5008f1872a85 100644 --- a/lookbook/docs/patterns/02-forms.md.erb +++ b/lookbook/docs/patterns/02-forms.md.erb @@ -45,14 +45,6 @@ The options are: <%= embed Patterns::FormsPreview, :default, panels: %i[] %> -## Vertical spacing - -By default, form elements do not have proper vertical spacing in Primer. We recommend a 16px (`stack/gap/normal`) vertical separate between individual elements. There is an [open issue in Primer's GitHub](https://github.com/primer/view_components/issues/3042) to fix this. - -Until this is fixed at a component level, please do not manually apply form padding. - -An alternative approach is to wrap individual form elements using `Form group`. Please only use this sparingly and only when it is absolutely necessarily. - ## Technical notes ### Usage diff --git a/spec/workers/mails/mailer_job_spec.rb b/spec/workers/mails/mailer_job_spec.rb new file mode 100644 index 000000000000..d8697d8376e5 --- /dev/null +++ b/spec/workers/mails/mailer_job_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 "spec_helper" + +class MyRSpecExampleMailer < ApplicationMailer + default from: "openproject@example.com", + subject: "Welcome to OpenProject" + + def welcome_email + user = params[:user] + mail(to: user) do |format| + format.text { render plain: "Welcome!" } + format.html { render html: "

Welcome!

".html_safe } + end + end + + def welcome2(user) + mail(to: user) do |format| + format.text { render plain: "Welcome!" } + format.html { render html: "

Welcome!

".html_safe } + end + end +end + +RSpec.describe Mails::MailerJob do + subject { described_class.new } + + it "is used to send emails when calling .deliver_later on a mailer" do + user = create(:user, mail: "user@example.com") + job = MyRSpecExampleMailer.with(user:).welcome_email.deliver_later + expect(job).to be_an_instance_of(described_class) + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => described_class.name)) + enqueued_job = enqueued_jobs.first + + perform_enqueued_jobs + # job has been performed + expect(performed_jobs).to contain_exactly(enqueued_job) + # there are no more jobs + expect(enqueued_jobs).to be_empty + end + + it "retries sending email on StandardError" do + user = "Will raise ArgumentError because ApplicationMailer expect a User instance as recipient" + MyRSpecExampleMailer.with(user:).welcome_email.deliver_later + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => "Mails::MailerJob", + "executions" => 0, + "exception_executions" => {})) + + # let's execute the mailer job + job1 = enqueued_jobs.first + perform_enqueued_jobs + + # job has been performed, but has encountered an error + expect(performed_jobs).to contain_exactly(job1) + expect(job1).to include("exception_executions" => { "[StandardError]" => 1 }) + + # and it is being retried: another identical job is queued with an increased execution count + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => "Mails::MailerJob", + "executions" => 1, + "exception_executions" => { "[StandardError]" => 1 })) + + # we can run this retried job, it will be performed, fail again, and enqueue another retry job again + job2 = enqueued_jobs.first + perform_enqueued_jobs + expect(performed_jobs).to contain_exactly(job1, job2) + expect(job2).to include("exception_executions" => { "[StandardError]" => 2 }) + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => "Mails::MailerJob", + "executions" => 2, + "exception_executions" => { "[StandardError]" => 2 })) + + # and so on... + end +end