From 9863a00d051421c68773493eafce0e2e656620a0 Mon Sep 17 00:00:00 2001 From: Aaron Date: Thu, 25 Jun 2026 16:16:21 +0800 Subject: [PATCH] Initial edit lock implementation --- app/controllers/concerns/editing_lock.rb | 111 ++++++++++++++++ app/controllers/evidence_controller.rb | 18 +++ app/controllers/issues_controller.rb | 18 +++ app/controllers/notes_controller.rb | 18 +++ .../controllers/editing_lock_controller.js | 53 ++++++++ app/views/evidence/edit.html.erb | 13 +- app/views/issues/edit.html.erb | 13 +- app/views/notes/edit.html.erb | 13 +- app/views/shared/_editing_locked.html.erb | 13 ++ config/routes.rb | 13 +- spec/controllers/evidence_controller_spec.rb | 20 +++ spec/controllers/issues_controller_spec.rb | 19 +++ spec/controllers/notes_controller_spec.rb | 21 +++ spec/support/editing_lock_shared_examples.rb | 124 ++++++++++++++++++ 14 files changed, 461 insertions(+), 6 deletions(-) create mode 100644 app/controllers/concerns/editing_lock.rb create mode 100644 app/javascript/controllers/editing_lock_controller.js create mode 100644 app/views/shared/_editing_locked.html.erb create mode 100644 spec/controllers/evidence_controller_spec.rb create mode 100644 spec/controllers/issues_controller_spec.rb create mode 100644 spec/controllers/notes_controller_spec.rb create mode 100644 spec/support/editing_lock_shared_examples.rb diff --git a/app/controllers/concerns/editing_lock.rb b/app/controllers/concerns/editing_lock.rb new file mode 100644 index 0000000000..8e3b07d9c8 --- /dev/null +++ b/app/controllers/concerns/editing_lock.rb @@ -0,0 +1,111 @@ +module EditingLock + LOCK_TTL = 120 + + def self.included(base) + base.before_action :set_editing_lock_record, only: [:lock, :unlock] + end + + # Controllers must implement this to return the relevant record. + def editing_lock_record + raise NotImplementedError, "#{self.class} must implement #editing_lock_record" + end + + def lock + if renew_lock(@editing_lock_record) + head :ok + else + render json: lock_owner(@editing_lock_record), status: :conflict + end + end + + def unlock + release_lock(@editing_lock_record) + head :no_content + end + + protected + + # Acquires the lock for the current user. + # + # Returns true if the lock was acquired (or already held by this user). + # Returns false if another user holds the lock. + def acquire_lock(record) + key = lock_key(record) + existing = redis.get(key) + + if existing + data = JSON.parse(existing) + if data['user_id'] == current_user.id + redis.expire(key, LOCK_TTL) + return true + else + return false + end + end + + set_lock(key) + true + end + + # Acquires the lock regardless of who currently holds it. + # The displaced user will discover the takeover on their next heartbeat. + def force_lock(record) + set_lock(lock_key(record)) + end + + # Releases the lock only if the current user holds it. + def release_lock(record) + key = lock_key(record) + existing = redis.get(key) + return unless existing + + data = JSON.parse(existing) + redis.del(key) if data['user_id'] == current_user.id + end + + # Renews the lock TTL if the current user holds it (used by heartbeat). + # + # Returns true if renewed, false if the lock belongs to another user or is gone. + def renew_lock(record) + key = lock_key(record) + existing = redis.get(key) + return false unless existing + + data = JSON.parse(existing) + return false unless data['user_id'] == current_user.id + + redis.expire(key, LOCK_TTL) + true + end + + # Returns { 'user_id' => ..., 'user_name' => ... } or nil. + def lock_owner(record) + existing = redis.get(lock_key(record)) + JSON.parse(existing) if existing + end + + def locked_by_other?(record) + owner = lock_owner(record) + owner && owner['user_id'] != current_user.id + end + + private + + def set_editing_lock_record + @editing_lock_record = editing_lock_record + end + + def lock_key(record) + model = record.model_name.name.downcase + "editing:#{current_project.id}:#{model}:#{record.id}" + end + + def set_lock(key) + data = { user_id: current_user.id, user_name: current_user.name }.to_json + redis.set(key, data, ex: LOCK_TTL) + end + + def redis + Resque.redis + end +end diff --git a/app/controllers/evidence_controller.rb b/app/controllers/evidence_controller.rb index 3afcce2c71..e38593563b 100644 --- a/app/controllers/evidence_controller.rb +++ b/app/controllers/evidence_controller.rb @@ -1,6 +1,7 @@ class EvidenceController < NestedNodeResourceController include AttachmentsCopier include ConflictResolver + include EditingLock include EvidenceHelper include LiquidEnabledResource include Mentioned @@ -45,6 +46,17 @@ def create end def edit + if locked_by_other?(@evidence) + if params[:force] + force_lock(@evidence) + else + @lock_owner = lock_owner(@evidence) + return + end + else + acquire_lock(@evidence) + end + @form_preview_path = preview_project_node_evidence_path(current_project, @node, @evidence) end @@ -58,6 +70,7 @@ def update copy_attachments(@evidence) if @evidence.node_changed? if @evidence.save + release_lock(@evidence) track_updated(@evidence) check_for_edit_conflicts(@evidence, updated_at_before_save) format.html do @@ -75,6 +88,7 @@ def update end def destroy + release_lock(@evidence) respond_to do |format| if @evidence.destroy track_destroyed(@evidence) @@ -105,6 +119,10 @@ def destroy private + def editing_lock_record + @evidence + end + def autogenerate_issue @evidence.issue = Issue.autogenerate_from(@evidence) track_created(@evidence.issue) diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index 90b1bc0011..fe45abab7d 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -1,6 +1,7 @@ class IssuesController < AuthenticatedController include ConflictResolver include ContentFromTemplate + include EditingLock include DynamicFieldNamesCacher include EventPublisher include IssuesHelper @@ -73,6 +74,17 @@ def create end def edit + if locked_by_other?(@issue) + if params[:force] + force_lock(@issue) + else + @lock_owner = lock_owner(@issue) + return + end + else + acquire_lock(@issue) + end + @form_preview_path = preview_project_issue_path(current_project, @issue) end @@ -82,6 +94,7 @@ def update if @issue.update(issue_params) @modified = true + release_lock(@issue) check_for_edit_conflicts(@issue, updated_at_before_save) format.html { redirect_to_main_or_qa } publish_event('issue.updated', @issue.to_event_payload) @@ -97,6 +110,7 @@ def update end def destroy + release_lock(@issue) respond_to do |format| if @issue.destroy format.html { redirect_to project_issues_path(current_project), notice: 'Issue deleted.' } @@ -126,6 +140,10 @@ def import private + def editing_lock_record + @issue + end + def liquid_resource_assigns { 'issue' => IssueDrop.new(@issue) } end diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 29dd984a4d..60ff629521 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -3,6 +3,7 @@ class NotesController < NestedNodeResourceController include AttachmentsCopier include ConflictResolver + include EditingLock include LiquidEnabledResource include Mentioned include MultipleDestroy @@ -38,6 +39,17 @@ def show end def edit + if locked_by_other?(@note) + if params[:force] + force_lock(@note) + else + @lock_owner = lock_owner(@note) + return + end + else + acquire_lock(@note) + end + @versions_count = @note.versions.count @form_preview_path = preview_project_node_note_path(current_project, @node, @note) end @@ -50,6 +62,7 @@ def update copy_attachments(@note) if @note.node_changed? if @note.save + release_lock(@note) track_updated(@note) check_for_edit_conflicts(@note, updated_at_before_save) # if the note has just been moved to another node, we must reload @@ -64,6 +77,7 @@ def update # Remove a Note from the back-end database. def destroy + release_lock(@note) if @note.destroy track_destroyed(@note) redirect_to project_node_path(current_project, @node), notice: 'Note deleted' @@ -86,6 +100,10 @@ def find_or_initialize_note end end + def editing_lock_record + @note + end + def liquid_resource_assigns { 'note' => NoteDrop.new(@note) } end diff --git a/app/javascript/controllers/editing_lock_controller.js b/app/javascript/controllers/editing_lock_controller.js new file mode 100644 index 0000000000..1b3a679fce --- /dev/null +++ b/app/javascript/controllers/editing_lock_controller.js @@ -0,0 +1,53 @@ +import { Controller } from "@hotwired/stimulus" + +const HEARTBEAT_INTERVAL = 60000 + +export default class extends Controller { + static values = { lockUrl: String, unlockUrl: String } + + connect() { + this.heartbeatTimer = setInterval(() => this.sendHeartbeat(), HEARTBEAT_INTERVAL) + } + + disconnect() { + clearInterval(this.heartbeatTimer) + this.releaseLock() + } + + sendHeartbeat() { + fetch(this.lockUrlValue, { + method: 'PATCH', + headers: { 'X-CSRF-Token': this.csrfToken }, + }).then(response => { + if (response.status === 409) { + response.json().then(({ user_name: userName }) => { + clearInterval(this.heartbeatTimer) + this.showLockTakenWarning(userName) + }) + } + }) + } + + showLockTakenWarning(userName) { + const submitBtn = this.element.nextElementSibling?.querySelector('[type=submit]') || + document.querySelector('form [type=submit]') + if (submitBtn) submitBtn.disabled = true + + const alert = document.createElement('div') + alert.className = 'alert alert-danger' + alert.textContent = `${userName} has taken over this edit. Your changes cannot be saved.` + this.element.after(alert) + } + + releaseLock() { + fetch(this.unlockUrlValue, { + method: 'DELETE', + headers: { 'X-CSRF-Token': this.csrfToken }, + keepalive: true, + }) + } + + get csrfToken() { + return document.querySelector('meta[name="csrf-token"]')?.content ?? '' + } +} diff --git a/app/views/evidence/edit.html.erb b/app/views/evidence/edit.html.erb index 6c87376956..05f5081663 100644 --- a/app/views/evidence/edit.html.erb +++ b/app/views/evidence/edit.html.erb @@ -9,6 +9,17 @@

Edit evidence

- <%= render 'form' %> + <% if @lock_owner %> + <%= render 'shared/editing_locked', + lock_owner: @lock_owner, + record: @evidence, + force_path: edit_project_node_evidence_path(current_project, @node, @evidence) %> + <% else %> +
+
+ <%= render 'form' %> + <% end %>
diff --git a/app/views/issues/edit.html.erb b/app/views/issues/edit.html.erb index e4b56d97c6..79824ebd2d 100644 --- a/app/views/issues/edit.html.erb +++ b/app/views/issues/edit.html.erb @@ -17,6 +17,17 @@

Edit issue (<%= @issue.state.humanize %>)

- <%= render 'form' %> + <% if @lock_owner %> + <%= render 'shared/editing_locked', + lock_owner: @lock_owner, + record: @issue, + force_path: edit_project_issue_path(current_project, @issue) %> + <% else %> +
+
+ <%= render 'form' %> + <% end %> diff --git a/app/views/notes/edit.html.erb b/app/views/notes/edit.html.erb index e82ddc2688..274e94d9b7 100644 --- a/app/views/notes/edit.html.erb +++ b/app/views/notes/edit.html.erb @@ -9,6 +9,17 @@

Edit note

- <%= render "form" %> + <% if @lock_owner %> + <%= render 'shared/editing_locked', + lock_owner: @lock_owner, + record: @note, + force_path: edit_project_node_note_path(current_project, @node, @note) %> + <% else %> +
+
+ <%= render "form" %> + <% end %>
diff --git a/app/views/shared/_editing_locked.html.erb b/app/views/shared/_editing_locked.html.erb new file mode 100644 index 0000000000..d37176b965 --- /dev/null +++ b/app/views/shared/_editing_locked.html.erb @@ -0,0 +1,13 @@ +
+

+ <%= lock_owner['user_name'] %> is currently editing this + <%= record.model_name.human.downcase %>. Editing at the same time may cause + conflicts. +

+

+ You can wait for them to finish, or take over the edit. If you take over, + <%= lock_owner['user_name'] %> will be notified on their next auto-save + heartbeat and their save will be blocked. +

+ <%= link_to 'Take over edit', "#{force_path}?force=true", class: 'btn btn-sm btn-warning', data: { turbo: false } %> +
diff --git a/config/routes.rb b/config/routes.rb index 6bbfe11a41..c601c8cc1a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -39,6 +39,13 @@ end end + concern :editing_lockable do + member do + patch :lock + delete :unlock + end + end + concern :previewable do member do post :preview @@ -70,7 +77,7 @@ post :create_multiple_evidence, to: 'issues/evidence#create_multiple' - resources :issues, concerns: [:multiple_destroy, :previewable] do + resources :issues, concerns: [:editing_lockable, :multiple_destroy, :previewable] do collection do post :import resources :merge, only: [:new, :create], controller: 'issues/merge' @@ -101,11 +108,11 @@ resource :merge, only: [:create], controller: 'nodes/merge' - resources :notes, concerns: [:multiple_destroy, :previewable] do + resources :notes, concerns: [:editing_lockable, :multiple_destroy, :previewable] do resources :revisions, only: [:index, :show] end - resources :evidence, except: :index, concerns: [:multiple_destroy, :previewable] do + resources :evidence, except: :index, concerns: [:editing_lockable, :multiple_destroy, :previewable] do resources :revisions, only: [:index, :show] end diff --git a/spec/controllers/evidence_controller_spec.rb b/spec/controllers/evidence_controller_spec.rb new file mode 100644 index 0000000000..64b116e85c --- /dev/null +++ b/spec/controllers/evidence_controller_spec.rb @@ -0,0 +1,20 @@ +require 'rails_helper' + +describe EvidenceController, type: :controller do + let(:current_user) { create(:user) } + let(:other_user) { create(:user) } + let(:node) { create(:node, project: @project) } + let(:evidence) { create(:evidence, node: node) } + + let(:edit_params) { { project_id: @project.id, node_id: node.id, id: evidence.id } } + let(:lock_params) { { project_id: @project.id, node_id: node.id, id: evidence.id } } + let(:unlock_params) { { project_id: @project.id, node_id: node.id, id: evidence.id } } + let(:record) { evidence } + + before do + @project = create(:project) + login_as_user(current_user) + end + + it_behaves_like 'editing lock behavior' +end diff --git a/spec/controllers/issues_controller_spec.rb b/spec/controllers/issues_controller_spec.rb new file mode 100644 index 0000000000..097b5ee420 --- /dev/null +++ b/spec/controllers/issues_controller_spec.rb @@ -0,0 +1,19 @@ +require 'rails_helper' + +describe IssuesController, type: :controller do + let(:current_user) { create(:user) } + let(:other_user) { create(:user) } + let(:issue) { create(:issue, project: @project) } + + let(:edit_params) { { project_id: @project.id, id: issue.id } } + let(:lock_params) { { project_id: @project.id, id: issue.id } } + let(:unlock_params) { { project_id: @project.id, id: issue.id } } + let(:record) { issue } + + before do + @project = create(:project) + login_as_user(current_user) + end + + it_behaves_like 'editing lock behavior' +end diff --git a/spec/controllers/notes_controller_spec.rb b/spec/controllers/notes_controller_spec.rb new file mode 100644 index 0000000000..43a169c7e8 --- /dev/null +++ b/spec/controllers/notes_controller_spec.rb @@ -0,0 +1,21 @@ +require 'rails_helper' + +describe NotesController, type: :controller do + let(:current_user) { create(:user) } + let(:other_user) { create(:user) } + + let(:node) { create(:node, project: @project) } + let(:note) { create(:note, node: node) } + + let(:edit_params) { { project_id: @project.id, node_id: node.id, id: note.id } } + let(:lock_params) { { project_id: @project.id, node_id: node.id, id: note.id } } + let(:unlock_params) { { project_id: @project.id, node_id: node.id, id: note.id } } + let(:record) { note } + + before do + @project = create(:project) + login_as_user(current_user) + end + + it_behaves_like 'editing lock behavior' +end diff --git a/spec/support/editing_lock_shared_examples.rb b/spec/support/editing_lock_shared_examples.rb new file mode 100644 index 0000000000..f01552d5ee --- /dev/null +++ b/spec/support/editing_lock_shared_examples.rb @@ -0,0 +1,124 @@ +# Shared examples for controllers that include EditingLock. +# +# Required let variables: +# - record : the model instance (note, issue, or evidence) +# - edit_params : params hash for GET #edit +# - lock_params : params hash for PATCH #lock +# - unlock_params : params hash for DELETE #unlock +# - current_user : the signed-in user +# - other_user : a second user (for locked-by-other scenarios) + +shared_examples 'editing lock behavior' do + let(:redis_double) { double('Redis::Namespace') } + + before do + allow(Resque).to receive(:redis).and_return(redis_double) + end + + describe '#edit' do + context 'when the content is not locked' do + before { allow(redis_double).to receive(:get).and_return(nil) } + + it 'acquires the lock and renders the edit form' do + allow(redis_double).to receive(:set) + + get :edit, params: edit_params + + expect(redis_double).to have_received(:set) + expect(assigns(:lock_owner)).to be_nil + expect(response).to render_template(:edit) + end + end + + context 'when the content is locked by the current user' do + before do + allow(redis_double).to receive(:get).and_return( + { user_id: current_user.id, user_name: current_user.name }.to_json + ) + allow(redis_double).to receive(:expire) + end + + it 'renews the TTL and renders the edit form' do + get :edit, params: edit_params + + expect(redis_double).to have_received(:expire) + expect(assigns(:lock_owner)).to be_nil + expect(response).to render_template(:edit) + end + end + + context 'when the content is locked by another user' do + before do + allow(redis_double).to receive(:get).and_return( + { user_id: other_user.id, user_name: other_user.name }.to_json + ) + end + + it 'exposes @lock_owner and renders the edit template (showing interstitial)' do + get :edit, params: edit_params + + expect(assigns(:lock_owner)).to include('user_id' => other_user.id, 'user_name' => other_user.name) + expect(response).to render_template(:edit) + end + + context 'with force=true' do + it 'force-acquires the lock and renders the edit form' do + allow(redis_double).to receive(:set) + + get :edit, params: edit_params.merge(force: 'true') + + expect(redis_double).to have_received(:set) + expect(assigns(:lock_owner)).to be_nil + expect(response).to render_template(:edit) + end + end + end + end + + describe '#lock' do + context 'when the current user holds the lock' do + before do + allow(redis_double).to receive(:get).and_return( + { user_id: current_user.id, user_name: current_user.name }.to_json + ) + allow(redis_double).to receive(:expire).and_return(1) + end + + it 'returns 200 OK' do + patch :lock, params: lock_params + + expect(response).to have_http_status(:ok) + end + end + + context 'when the lock has been taken by another user' do + before do + allow(redis_double).to receive(:get).and_return( + { user_id: other_user.id, user_name: other_user.name }.to_json + ) + end + + it 'returns 409 Conflict with the new owner details' do + patch :lock, params: lock_params + + expect(response).to have_http_status(:conflict) + body = JSON.parse(response.body) + expect(body).to include('user_id' => other_user.id, 'user_name' => other_user.name) + end + end + end + + describe '#unlock' do + it 'releases the lock and returns 204' do + allow(redis_double).to receive(:get).and_return( + { user_id: current_user.id, user_name: current_user.name }.to_json + ) + allow(redis_double).to receive(:del) + + delete :unlock, params: unlock_params + + expect(redis_double).to have_received(:del) + expect(response).to have_http_status(:no_content) + end + end +end