From abf36be366b1c7b5468645d87255b9d178b48a30 Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 11:20:59 -0800 Subject: [PATCH 1/8] :gift: add dynamic toggling for job scheduling using environment variables - Updated `find_or_schedule_jobs` to dynamically include jobs based on environment variables. - Introduced the following environment flags to control job scheduling: - `ENABLE_BATCH_EMAIL` for `BatchEmailNotificationJob` - `ENABLE_DEPOSITOR_EMAIL` for `DepositorEmailNotificationJob` - `ENABLE_USER_STAT` for `UserStatCollectionJob` - This allows buggy jobs to be disabled temporarily without modifying the codebase. - Default behavior ensures only non-buggy jobs are scheduled when flags are not set. --- app/models/account.rb | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index efdab3481..09dc364b4 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -176,15 +176,21 @@ def find_job(klass) def find_or_schedule_jobs account = Site.account AccountElevator.switch!(self) - [ + + jobs_to_schedule = [ EmbargoAutoExpiryJob, - LeaseAutoExpiryJob, - BatchEmailNotificationJob, - DepositorEmailNotificationJob, - UserStatCollectionJob - ].each do |klass| + LeaseAutoExpiryJob + ] + + # Add conditional checks based on a config or environment variable + jobs_to_schedule << BatchEmailNotificationJob if ENV['ENABLE_BATCH_EMAIL'] == 'true' + jobs_to_schedule << DepositorEmailNotificationJob if ENV['ENABLE_DEPOSITOR_EMAIL'] == 'true' + jobs_to_schedule << UserStatCollectionJob if ENV['ENABLE_USER_STAT'] == 'true' + + jobs_to_schedule.each do |klass| klass.perform_later unless find_job(klass) end + account ? AccountElevator.switch!(account) : reset! end end From 52e90c5687e55f92f2ebad8010a579c0ff8268a6 Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 14:25:43 -0800 Subject: [PATCH 2/8] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Refactor=20to=20remove?= =?UTF-8?q?=20ENV=20vars=20in=20favor=20of=20account=20settings=20pattern?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Adds batch_email_notifications, depositor_email_notifications, and user_analystics to account settings - only displays batch email frequency if batch email notifications are enabled from account settings --- app/models/account.rb | 6 +++--- app/models/concerns/account_settings.rb | 3 +++ .../dashboard/profiles/_edit_primary.html.erb | 14 ++++++++------ app/views/hyrax/users/_user_info.html.erb | 8 +++++--- 4 files changed, 19 insertions(+), 12 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 09dc364b4..c62e382a6 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -183,9 +183,9 @@ def find_or_schedule_jobs ] # Add conditional checks based on a config or environment variable - jobs_to_schedule << BatchEmailNotificationJob if ENV['ENABLE_BATCH_EMAIL'] == 'true' - jobs_to_schedule << DepositorEmailNotificationJob if ENV['ENABLE_DEPOSITOR_EMAIL'] == 'true' - jobs_to_schedule << UserStatCollectionJob if ENV['ENABLE_USER_STAT'] == 'true' + jobs_to_schedule << BatchEmailNotificationJob if batch_email_notifications + jobs_to_schedule << DepositorEmailNotificationJob if depositor_email_notifications + jobs_to_schedule << UserStatCollectionJob if user_analytics jobs_to_schedule.each do |klass| klass.perform_later unless find_job(klass) diff --git a/app/models/concerns/account_settings.rb b/app/models/concerns/account_settings.rb index 2e9dc2e91..5d2996c06 100644 --- a/app/models/concerns/account_settings.rb +++ b/app/models/concerns/account_settings.rb @@ -23,11 +23,13 @@ module AccountSettings setting :allow_downloads, type: 'boolean', default: true setting :allow_signup, type: 'boolean', default: true setting :analytics_provider, type: 'string' + setting :batch_email_notifications, type: 'boolean', default: false setting :bulkrax_field_mappings, type: 'json_editor', default: Hyku.default_bulkrax_field_mappings.to_json setting :bulkrax_validations, type: 'boolean', disabled: true setting :cache_api, type: 'boolean', default: false setting :contact_email, type: 'string', default: 'change-me-in-settings@example.com' setting :contact_email_to, type: 'string', default: 'change-me-in-settings@example.com' + setting :depositor_email_notifications, type: 'boolean', default: false setting :doi_reader, type: 'boolean', default: false setting :doi_writer, type: 'boolean', default: false setting :file_acl, type: 'boolean', default: true, private: true @@ -50,6 +52,7 @@ module AccountSettings setting :smtp_settings, type: 'hash', private: true, default: {} setting :solr_collection_options, type: 'hash', default: solr_collection_options setting :ssl_configured, type: 'boolean', default: true, private: true + setting :user_analytics, type: 'boolean', default: false setting :weekly_email_list, type: 'array', disabled: true setting :yearly_email_list, type: 'array', disabled: true diff --git a/app/views/hyrax/dashboard/profiles/_edit_primary.html.erb b/app/views/hyrax/dashboard/profiles/_edit_primary.html.erb index 06ae76b4a..e16f2a34f 100644 --- a/app/views/hyrax/dashboard/profiles/_edit_primary.html.erb +++ b/app/views/hyrax/dashboard/profiles/_edit_primary.html.erb @@ -56,12 +56,14 @@ -
- <%= f.label :batch_email_frequency, t("hyrax.user_profile.email_frequency.label").html_safe, class: 'col-4 col-form-label' %> -
- <%= f.select :batch_email_frequency, controller.frequency_options, {}, { class: "form-control" } %> -
-
+ <% if current_account.batch_email_notifications %> +
+ <%= f.label :batch_email_frequency, t("hyrax.user_profile.email_frequency.label").html_safe, class: 'col-4 col-form-label' %> +
+ <%= f.select :batch_email_frequency, controller.frequency_options, {}, { class: "form-control" } %> +
+
+ <% end %> <%= render 'trophy_edit', trophies: @trophies %> diff --git a/app/views/hyrax/users/_user_info.html.erb b/app/views/hyrax/users/_user_info.html.erb index e6c62a1a2..08b1e4ef0 100644 --- a/app/views/hyrax/users/_user_info.html.erb +++ b/app/views/hyrax/users/_user_info.html.erb @@ -81,7 +81,9 @@ <% end %> -
<%= t("hyrax.user_profile.email_frequency.label").html_safe %>
- <% frequency = user.batch_email_frequency || 'not_set' %> -
<%= t("hyrax.user_profile.email_frequency.#{frequency}") %>
+ <% if current_account.batch_email_notifications %> +
<%= t("hyrax.user_profile.email_frequency.label").html_safe %>
+ <% frequency = user.batch_email_frequency || 'not_set' %> +
<%= t("hyrax.user_profile.email_frequency.#{frequency}") %>
+ <% end %> From 06afd3e8b3980a98b0b4825cd4b2471b21f8dd94 Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 14:37:54 -0800 Subject: [PATCH 3/8] =?UTF-8?q?=F0=9F=A7=B9=20Automatically=20schedule=20j?= =?UTF-8?q?obs=20when=20account=20settings=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When certain account settings (user_analytics, batch_email_notifications, or depositor_email_notifications) are updated, the system will now automatically schedule the corresponding background jobs. This eliminates the need for a server restart to activate these features. - Added after_save callback to detect settings changes - Added logic to compare relevant settings before and after save - Triggers find_or_schedule_jobs when settings change This ensures features like user analytics start working immediately after being enabled through the settings interface. --- app/models/account.rb | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/app/models/account.rb b/app/models/account.rb index c62e382a6..00dc0f2bc 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -47,6 +47,8 @@ class Account < ApplicationRecord format: { with: /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/ }, unless: proc { |a| a.tenant == 'public' || a.tenant == 'single' } + after_save :schedule_jobs_if_settings_changed + def self.admin_host host = ENV.fetch('HYKU_ADMIN_HOST', nil) host ||= ENV['HOST'] @@ -182,7 +184,6 @@ def find_or_schedule_jobs LeaseAutoExpiryJob ] - # Add conditional checks based on a config or environment variable jobs_to_schedule << BatchEmailNotificationJob if batch_email_notifications jobs_to_schedule << DepositorEmailNotificationJob if depositor_email_notifications jobs_to_schedule << UserStatCollectionJob if user_analytics @@ -193,5 +194,24 @@ def find_or_schedule_jobs account ? AccountElevator.switch!(account) : reset! end + + private + + def schedule_jobs_if_settings_changed + return unless self.class.column_names.include?('settings') + + relevant_settings = [ + 'batch_email_notifications', + 'depositor_email_notifications', + 'user_analytics' + ] + + return unless saved_changes['settings'] + old_settings = saved_changes['settings'][0] || {} + new_settings = saved_changes['settings'][1] || {} + + return unless old_settings.slice(*relevant_settings) != new_settings.slice(*relevant_settings) + find_or_schedule_jobs + end end # rubocop:enable Metrics/ClassLength From 50040bc16d1f7252429f81aa1a8b56cc6faa0dcf Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 14:51:14 -0800 Subject: [PATCH 4/8] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Refactor=20account=20s?= =?UTF-8?q?ettings=20change=20detection=20for=20better=20readability?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplifies the #schedule_jobs_if_settings_changed method to be more intuitive and easier to maintain: - Replace database column check with direct settings check - Add descriptive variable names and comments - Break complex logic into clear, distinct steps - Improve code organization with early returns The functionality remains the same, but the code is now more self-documenting and easier to understand. --- app/models/account.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 00dc0f2bc..36ae40574 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -198,7 +198,7 @@ def find_or_schedule_jobs private def schedule_jobs_if_settings_changed - return unless self.class.column_names.include?('settings') + return unless settings relevant_settings = [ 'batch_email_notifications', @@ -210,7 +210,10 @@ def schedule_jobs_if_settings_changed old_settings = saved_changes['settings'][0] || {} new_settings = saved_changes['settings'][1] || {} - return unless old_settings.slice(*relevant_settings) != new_settings.slice(*relevant_settings) + old_relevant_settings = old_settings.slice(*relevant_settings) + new_relevant_settings = new_settings.slice(*relevant_settings) + + return unless old_relevant_settings != new_relevant_settings find_or_schedule_jobs end end From 30e144c936af57de9eab369b5534d86000a62bda Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 15:46:26 -0800 Subject: [PATCH 5/8] =?UTF-8?q?=E2=9C=85=20update=20schedule=5Frecurring?= =?UTF-8?q?=5Fjobs=20specs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add test contexts for enabled and disabled account settings - Verify that EmbargoAutoExpiryJob and LeaseAutoExpiryJob always run - Verify that BatchEmailNotificationJob, DepositorEmailNotificationJob, and UserStatCollectionJob only run when their respective settings are enabled --- spec/services/create_account_spec.rb | 58 ++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/spec/services/create_account_spec.rb b/spec/services/create_account_spec.rb index 70ba896ff..73b20f95f 100644 --- a/spec/services/create_account_spec.rb +++ b/spec/services/create_account_spec.rb @@ -116,18 +116,54 @@ end describe '#schedule_recurring_jobs' do - it "Enqueues Recurring jobs" do - [ - EmbargoAutoExpiryJob, - LeaseAutoExpiryJob, - BatchEmailNotificationJob, - DepositorEmailNotificationJob, - UserStatCollectionJob - ].each do |klass| - expect(account).to receive(:find_job).with(klass).and_return(false) - expect(klass).to receive(:perform_later) + context 'when settings are enabled' do + before do + allow(account).to receive(:batch_email_notifications).and_return(true) + allow(account).to receive(:depositor_email_notifications).and_return(true) + allow(account).to receive(:user_analytics).and_return(true) + end + + it "enqueues recurring jobs" do + [ + EmbargoAutoExpiryJob, + LeaseAutoExpiryJob, + BatchEmailNotificationJob, + DepositorEmailNotificationJob, + UserStatCollectionJob + ].each do |klass| + expect(account).to receive(:find_job).with(klass).and_return(false) + expect(klass).to receive(:perform_later) + end + subject.schedule_recurring_jobs + end + end + + context 'when settings are disabled' do + before do + allow(account).to receive(:batch_email_notifications).and_return(false) + allow(account).to receive(:depositor_email_notifications).and_return(false) + allow(account).to receive(:user_analytics).and_return(false) + end + + it "only enqueues embargo and lease jobs" do + # These jobs should always run regardless of settings + [EmbargoAutoExpiryJob, LeaseAutoExpiryJob].each do |klass| + expect(account).to receive(:find_job).with(klass).and_return(false) + expect(klass).to receive(:perform_later) + end + + # These jobs should not run when settings are disabled + [ + BatchEmailNotificationJob, + DepositorEmailNotificationJob, + UserStatCollectionJob + ].each do |klass| + expect(account).not_to receive(:find_job).with(klass) + expect(klass).not_to receive(:perform_later) + end + + subject.schedule_recurring_jobs end - subject.schedule_recurring_jobs end end end From 20b1c9718a1e284a435ca0c91387767ee58c2bc8 Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 15:49:36 -0800 Subject: [PATCH 6/8] =?UTF-8?q?=E2=9C=85=20update=20account=20settings=20s?= =?UTF-8?q?pecs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- spec/models/concerns/account_settings_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/models/concerns/account_settings_spec.rb b/spec/models/concerns/account_settings_spec.rb index 4a57aea9a..1f9ae3fee 100644 --- a/spec/models/concerns/account_settings_spec.rb +++ b/spec/models/concerns/account_settings_spec.rb @@ -10,10 +10,12 @@ expect(account.public_settings(is_superadmin: true).keys.sort).to eq %i[allow_downloads allow_signup analytics_provider + batch_email_notifications bulkrax_field_mappings cache_api contact_email contact_email_to + depositor_email_notifications doi_reader doi_writer email_domain @@ -30,7 +32,8 @@ s3_bucket smtp_settings solr_collection_options - ssl_configured] + ssl_configured + user_analytics] end # rubocop:enable RSpec/ExampleLength end From b2794c511863f9b9be0f3e33c495989a241660a5 Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 15:58:07 -0800 Subject: [PATCH 7/8] =?UTF-8?q?=E2=9C=85=20Update=20create=5Faccount=5Fspe?= =?UTF-8?q?c.rb?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- spec/services/create_account_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/services/create_account_spec.rb b/spec/services/create_account_spec.rb index 73b20f95f..e13e7db11 100644 --- a/spec/services/create_account_spec.rb +++ b/spec/services/create_account_spec.rb @@ -146,13 +146,11 @@ end it "only enqueues embargo and lease jobs" do - # These jobs should always run regardless of settings [EmbargoAutoExpiryJob, LeaseAutoExpiryJob].each do |klass| expect(account).to receive(:find_job).with(klass).and_return(false) expect(klass).to receive(:perform_later) end - # These jobs should not run when settings are disabled [ BatchEmailNotificationJob, DepositorEmailNotificationJob, From fe02a778cf7385cfc2e254ce9233657f36e3cf04 Mon Sep 17 00:00:00 2001 From: Shana Moore Date: Fri, 24 Jan 2025 16:15:23 -0800 Subject: [PATCH 8/8] =?UTF-8?q?=E2=9C=85=20Mock=20find=5For=5Fschedule=5Fj?= =?UTF-8?q?obs=20in=20account=20endpoints=20spec?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The account endpoints spec was failing because it attempted to schedule jobs before the tenant schema was created. By mocking find_or_schedule_jobs in this specific test, we avoid the schema error while preserving the actual job scheduling behavior in other tests. --- spec/features/accounts_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/features/accounts_spec.rb b/spec/features/accounts_spec.rb index d8e412c2c..5ad12c7db 100644 --- a/spec/features/accounts_spec.rb +++ b/spec/features/accounts_spec.rb @@ -16,6 +16,7 @@ allow(Apartment::Tenant).to receive(:switch).with(account.tenant) do |&block| block.call end + allow_any_instance_of(Account).to receive(:find_or_schedule_jobs) end around do |example|