diff --git a/app/contracts/labels/create_contract.rb b/app/contracts/labels/create_contract.rb index aa865cd7f669..7e2b290e70ad 100644 --- a/app/contracts/labels/create_contract.rb +++ b/app/contracts/labels/create_contract.rb @@ -29,6 +29,8 @@ module Labels class CreateContract < BaseContract + attribute :created_inline + validate :user_allowed_to_create private diff --git a/app/models/concerns/labelable.rb b/app/models/concerns/labelable.rb index 9ecd958b9f67..7522a9be4a1e 100644 --- a/app/models/concerns/labelable.rb +++ b/app/models/concerns/labelable.rb @@ -73,7 +73,8 @@ def normalize_label_ids(new_labels) def validate_label_id_replacements return unless override_labels? - missing = label_id_replacements - Label.where(id: label_id_replacements).pluck(:id) + # KEY SHARE keeps the cleanup job from deleting these labels until this save commits. + missing = label_id_replacements - Label.where(id: label_id_replacements).lock("FOR KEY SHARE").pluck(:id) errors.add(:labels, :does_not_exist) if missing.any? end diff --git a/app/models/label.rb b/app/models/label.rb index 6ad8f287f83a..456c958caddc 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -58,6 +58,13 @@ class Label < ApplicationRecord where("LOWER(labels.name) = LOWER(?)", normalize_value_for(:name, name)) } + scope :created_inline, -> { where(created_inline: true) } + + scope :never_used, -> { + where("NOT EXISTS (SELECT 1 FROM labelings WHERE labelings.label_id = labels.id)") + .where("NOT EXISTS (SELECT 1 FROM label_journals WHERE label_journals.label_id = labels.id)") + } + normalizes :name, with: -> { it.squish } validates :name, diff --git a/app/services/labels/find_or_create_service.rb b/app/services/labels/find_or_create_service.rb index e4bc5c558dcf..7421371cb058 100644 --- a/app/services/labels/find_or_create_service.rb +++ b/app/services/labels/find_or_create_service.rb @@ -38,7 +38,7 @@ def initialize(user:) def call(name:) existing = Label.named(name).first - return ServiceResult.success(result: existing) if existing + return found(existing) if existing create(name) end @@ -46,14 +46,21 @@ def call(name:) private def create(name) - call = CreateService.new(user:).call(name:) - return call if call.success? || call.errors.symbols_for(:name).exclude?(:taken) + call = CreateService.new(user:).call(name:, created_inline: true) - existing(name) + if call.success? + CleanupUnusedInlineJob.perform_after_grace + call + elsif call.errors.symbols_for(:name).include?(:taken) + found(Label.named(name).first!) + else + call + end end - def existing(name) - ServiceResult.success(result: Label.named(name).first!) + def found(label) + label.touch if label.created_inline? + ServiceResult.success(result: label) end end end diff --git a/app/services/labels/set_attributes_service.rb b/app/services/labels/set_attributes_service.rb index f6aec6c7dfe3..5a8fee4d847d 100644 --- a/app/services/labels/set_attributes_service.rb +++ b/app/services/labels/set_attributes_service.rb @@ -31,6 +31,15 @@ module Labels class SetAttributesService < ::BaseServices::SetAttributes private + def set_attributes(params) + super + clear_inline_origin if model.persisted? && model.changed? + end + + def clear_inline_origin + model.change_by_system { model.created_inline = false } + end + def set_default_attributes(_params) model.change_by_system do model.author = user diff --git a/app/workers/labels/cleanup_unused_inline_job.rb b/app/workers/labels/cleanup_unused_inline_job.rb new file mode 100644 index 000000000000..aa9dadb182db --- /dev/null +++ b/app/workers/labels/cleanup_unused_inline_job.rb @@ -0,0 +1,66 @@ +# 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class Labels::CleanupUnusedInlineJob < ApplicationJob + include JobConcurrency + + GRACE_PERIOD = 15.minutes + MIN_WAIT = 1.minute + + queue_with_priority :low + good_job_control_concurrency_with(enqueue_limit: 1, key: name) + + def self.perform_after_grace(from: Time.current) + set(wait_until: [from + GRACE_PERIOD, MIN_WAIT.from_now].max).perform_later + end + + def perform + Label.transaction do + ids = candidate_ids + Label.never_used.where(id: ids).delete_all if ids.any? + end + + schedule_next_run + end + + private + + def schedule_next_run + oldest = Label.created_inline.never_used.minimum(:updated_at) + self.class.perform_after_grace(from: oldest) if oldest + end + + def candidate_ids + candidates.lock("FOR UPDATE SKIP LOCKED").pluck(:id) + end + + def candidates + Label.created_inline.never_used.where(updated_at: ..GRACE_PERIOD.ago) + end +end diff --git a/config/initializers/cronjobs.rb b/config/initializers/cronjobs.rb index aa20fa110f4b..41e077c0b004 100644 --- a/config/initializers/cronjobs.rb +++ b/config/initializers/cronjobs.rb @@ -78,6 +78,10 @@ "AI::TextTransformRuns::CleanupJob": { cron: "*/10 * * * *", class: AI::TextTransformRuns::CleanupJob.name + }, + "Labels::CleanupUnusedInlineJob": { + cron: "37 2 * * *", # runs at 2:37 nightly + class: Labels::CleanupUnusedInlineJob.name } } ) diff --git a/db/migrate/20261021100000_add_created_inline_to_labels.rb b/db/migrate/20261021100000_add_created_inline_to_labels.rb new file mode 100644 index 000000000000..e0b27f9da419 --- /dev/null +++ b/db/migrate/20261021100000_add_created_inline_to_labels.rb @@ -0,0 +1,34 @@ +# 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. + +class AddCreatedInlineToLabels < ActiveRecord::Migration[8.1] + def change + add_column :labels, :created_inline, :boolean, null: false, default: false + end +end diff --git a/spec/factories/label_factory.rb b/spec/factories/label_factory.rb index dc8e7eeeda05..e1745df0a164 100644 --- a/spec/factories/label_factory.rb +++ b/spec/factories/label_factory.rb @@ -31,5 +31,9 @@ factory :label do sequence(:name) { "Label #{it}" } author factory: :user + + trait :created_inline do + created_inline { true } + end end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 0a93f90002f7..c8d7df87d099 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -104,6 +104,36 @@ def label_work_packages(label, project, count:) end end + describe ".created_inline" do + it "returns only labels created inline" do + inline = create(:label, :created_inline) + create(:label) + + expect(described_class.created_inline).to contain_exactly(inline) + end + end + + describe ".never_used" do + let!(:unused) { create(:label) } + + it "includes labels without labelings or journal snapshots" do + expect(described_class.never_used).to contain_exactly(unused) + end + + it "excludes labels with a labeling" do + create(:labeling, label: create(:label)) + + expect(described_class.never_used).to contain_exactly(unused) + end + + it "excludes labels only referenced by a journal snapshot" do + journaled = create(:label) + create(:journal_label_journal, journal: create(:work_package).journals.last, label: journaled) + + expect(described_class.never_used).to contain_exactly(unused) + end + end + describe ".page_of" do let!(:alpha) { create(:label, name: "Alpha") } let!(:bravo) { create(:label, name: "bravo") } diff --git a/spec/services/labels/create_service_spec.rb b/spec/services/labels/create_service_spec.rb index 7b94595d29d4..c6eb9a40af4c 100644 --- a/spec/services/labels/create_service_spec.rb +++ b/spec/services/labels/create_service_spec.rb @@ -48,6 +48,10 @@ expect(label.name).to eq("Bug") end + it "does not flag the label as created inline" do + expect(subject.result).not_to be_created_inline + end + it "fails when the name is already taken, case-insensitively" do create(:label, name: "Bug") diff --git a/spec/services/labels/find_or_create_service_spec.rb b/spec/services/labels/find_or_create_service_spec.rb index 434c2b4e550b..b3eef44bf1f7 100644 --- a/spec/services/labels/find_or_create_service_spec.rb +++ b/spec/services/labels/find_or_create_service_spec.rb @@ -43,6 +43,11 @@ expect(call.result).to be_persisted expect(call.result.name).to eq("Bug") expect(call.result).to be_previously_new_record + expect(call.result).to be_created_inline + end + + it "books a cleanup run" do + expect { call }.to have_enqueued_job(Labels::CleanupUnusedInlineJob) end end @@ -57,6 +62,7 @@ expect(call).to be_success expect(call.result).to eq(existing) expect(call.result).not_to be_previously_new_record + expect(call.result).not_to be_created_inline end context "with a name Ruby and Postgres lowercase differently" do @@ -71,6 +77,32 @@ end end + context "when an inline-created label with that name exists" do + shared_let(:existing) { create(:label, :created_inline, name: "bug", created_at: 2.days.ago, updated_at: 2.days.ago) } + + let(:name) { "Bug" } + + it "books no cleanup run" do + expect { call }.not_to have_enqueued_job(Labels::CleanupUnusedInlineJob) + end + + it "touches an existing inline label so the cleanup grace period restarts" do + freeze_time + + expect { call }.to change { existing.reload.updated_at }.to(Time.current) + end + end + + context "when an admin-created label with that name exists" do + shared_let(:existing) { create(:label, name: "bug", updated_at: 2.days.ago) } + + let(:name) { "Bug" } + + it "leaves an admin-created label untouched" do + expect { call }.not_to change { existing.reload.updated_at } + end + end + context "when another request creates the label between lookup and validation" do let(:name) { "Bug" } diff --git a/spec/services/labels/update_service_spec.rb b/spec/services/labels/update_service_spec.rb index 09d2ff911b32..1ffb05f35aea 100644 --- a/spec/services/labels/update_service_spec.rb +++ b/spec/services/labels/update_service_spec.rb @@ -48,6 +48,24 @@ expect(label.reload.name).to eq("Defect") end + context "with a label created inline" do + let(:label) { create(:label, :created_inline, name: "Bug") } + + it "turns it into a curated label" do + result = instance.call(name: "Defect") + + expect(result).to be_success + expect(label.reload).not_to be_created_inline + end + + it "stays a cleanup candidate when nothing changes" do + result = instance.call(name: "Bug") + + expect(result).to be_success + expect(label.reload).to be_created_inline + end + end + context "when the new name is already taken in a different case" do before { create(:label, name: "defect") } diff --git a/spec/workers/labels/cleanup_unused_inline_job_spec.rb b/spec/workers/labels/cleanup_unused_inline_job_spec.rb new file mode 100644 index 000000000000..5578235264a8 --- /dev/null +++ b/spec/workers/labels/cleanup_unused_inline_job_spec.rb @@ -0,0 +1,159 @@ +# 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, see . +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Labels::CleanupUnusedInlineJob, type: :job do + let(:label) { create(:label, :created_inline, created_at: 2.days.ago, updated_at: 2.days.ago) } + + it "is scheduled nightly" do + expect(Rails.application.config.good_job.cron[:"Labels::CleanupUnusedInlineJob"]) + .to include(cron: "37 2 * * *", class: described_class.name) + end + + it "collapses repeated bookings into one pending run", with_good_job: described_class do + 2.times { described_class.perform_after_grace } + + expect(GoodJob::Job.where(concurrency_key: described_class.name).count).to eq(1) + end + + describe ".perform_after_grace" do + it "enqueues a run once the grace period has passed" do + freeze_time + + expect { described_class.perform_after_grace } + .to have_enqueued_job(described_class).at(described_class::GRACE_PERIOD.from_now) + end + end + + it "deletes an inline label older than the grace period that was never used" do + label + + expect { described_class.perform_now }.to change(Label, :count).by(-1) + expect(Label.exists?(label.id)).to be(false) + end + + it "keeps an inline label younger than the grace period" do + young = create(:label, :created_inline, created_at: 5.minutes.ago, updated_at: 5.minutes.ago) + + expect { described_class.perform_now }.not_to change(Label, :count) + expect(Label.exists?(young.id)).to be(true) + end + + it "keeps an inline label that was handed out again within the grace period" do + recent = create(:label, :created_inline, created_at: 2.days.ago, updated_at: 5.minutes.ago) + + expect { described_class.perform_now }.not_to change(Label, :count) + expect(Label.exists?(recent.id)).to be(true) + end + + it "books the next run for when the oldest remaining inline label leaves its grace period" do + freeze_time + create(:label, :created_inline, updated_at: 5.minutes.ago) + create(:label, :created_inline, updated_at: 10.minutes.ago) + + expect { described_class.perform_now } + .to have_enqueued_job(described_class).at(5.minutes.from_now) + end + + it "waits before retrying a label it could not lock" do + freeze_time + label + job = described_class.new + allow(job).to receive(:candidate_ids).and_return([]) + + expect { job.perform_now } + .to have_enqueued_job(described_class).at(described_class::MIN_WAIT.from_now) + end + + it "books no further run when no unused inline label remains" do + label + + expect { described_class.perform_now }.not_to have_enqueued_job(described_class) + end + + it "keeps an admin-created label that was never used" do + curated = create(:label, created_at: 2.days.ago, updated_at: 2.days.ago) + + expect { described_class.perform_now }.not_to change(Label, :count) + expect(Label.exists?(curated.id)).to be(true) + end + + it "keeps an inline label that only a journal snapshot references" do + create(:journal_label_journal, journal: create(:work_package).journals.last, label:) + + expect { described_class.perform_now }.not_to change(Label, :count) + end + + it "keeps a label that gets used between candidate selection and deletion" do + label + job = described_class.new + allow(job).to receive(:candidate_ids).and_wrap_original do |original| + original.call.tap { create(:labeling, label:) } + end + + expect { job.perform_now }.not_to change(Label, :count) + expect(Labeling.where(label:)).to exist + end + + describe "labels added to and removed from a work package" do + shared_let(:user) { create(:admin) } + shared_let(:work_package) do + create(:work_package, journals: { 10.minutes.ago => { user: } }) + end + + current_user { user } + + def set_labels(label_ids) + WorkPackages::UpdateService.new(user:, model: work_package).call(label_ids:) + end + + context "when both changes happen within the journal aggregation time", + with_settings: { journal_aggregation_time_minutes: 5 } do + it "leaves no journal snapshot behind and the label is deleted" do + set_labels([label.id]) + set_labels([]) + + expect(Journal::LabelJournal.where(label:)).to be_empty + expect { described_class.perform_now }.to change(Label, :count).by(-1) + end + end + + context "when the removal happens after the journal aggregation time", + with_settings: { journal_aggregation_time_minutes: 0 } do + it "keeps the label because an earlier journal still references it" do + set_labels([label.id]) + set_labels([]) + + expect(Journal::LabelJournal.where(label:)).to exist + expect { described_class.perform_now }.not_to change(Label, :count) + end + end + end +end