Initial edit lock implementation

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

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)

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

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

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 ?? ''
+  }
+}

app/views/evidence/edit.html.erb

@@ -9,6 +9,17 @@
 <div class="content-container">
   <h4 class="header-underline">Edit evidence</h4>
   <div class="note-text-inner">
-    <%= 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 %>
+      <div data-controller="editing-lock"
+           data-editing-lock-lock-url-value="<%= lock_project_node_evidence_path(current_project, @node, @evidence) %>"
+           data-editing-lock-unlock-url-value="<%= unlock_project_node_evidence_path(current_project, @node, @evidence) %>">
+      </div>
+      <%= render 'form' %>
+    <% end %>
   </div>
 </div>

app/views/issues/edit.html.erb

@@ -17,6 +17,17 @@
     <h4 class="header-underline">Edit issue (<%= @issue.state.humanize %>)</h4>
     <!-- This div is needed to allow correct tabbing order in Chrome with page is loaded with turbolinks -->
     <div class="autofocus" autofocus="true" tabindex="1"></div>
-    <%= 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 %>
+      <div data-controller="editing-lock"
+           data-editing-lock-lock-url-value="<%= lock_project_issue_path(current_project, @issue) %>"
+           data-editing-lock-unlock-url-value="<%= unlock_project_issue_path(current_project, @issue) %>">
+      </div>
+      <%= render 'form' %>
+    <% end %>
   </div>
 </div>

app/views/notes/edit.html.erb

@@ -9,6 +9,17 @@
 <div class="content-container">
   <h4 class="header-underline">Edit note</h4>
   <div class="note-text-inner">
-    <%= 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 %>
+      <div data-controller="editing-lock"
+           data-editing-lock-lock-url-value="<%= lock_project_node_note_path(current_project, @node, @note) %>"
+           data-editing-lock-unlock-url-value="<%= unlock_project_node_note_path(current_project, @node, @note) %>">
+      </div>
+      <%= render "form" %>
+    <% end %>
   </div>
 </div>

app/views/shared/_editing_locked.html.erb

@@ -0,0 +1,13 @@
+<div class="alert alert-warning">
+  <p>
+    <strong><%= lock_owner['user_name'] %></strong> is currently editing this
+    <%= record.model_name.human.downcase %>. Editing at the same time may cause
+    conflicts.
+  </p>
+  <p>
+    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.
+  </p>
+  <%= link_to 'Take over edit', "#{force_path}?force=true", class: 'btn btn-sm btn-warning', data: { turbo: false } %>
+</div>