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 @@
+ <%= 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 } %> +