-
Notifications
You must be signed in to change notification settings - Fork 227
add proactive edit locking for issues #1658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
MattBudz
wants to merge
15
commits into
develop
Choose a base branch
from
editing-sessions/add-edit-locking-2
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
16f9d34
add EditingSession model for tracking active editors
MattBudz d6b546a
add EditLockable concern to guard concurrent editing
MattBudz 21008c3
add feature and request specs for the edit locking flow
MattBudz bd97e42
add CHANGELOG entry for proactive edit locking
MattBudz baa62c0
close the btn-group div in qa/state_button
MattBudz cc33207
Add configurable STALE_AFTER
aapomm f39950f
close form-actions div once to fix edit-issue layout
MattBudz e58e149
release edit lock when canceling out of an issue edit form
MattBudz 2854a07
rename editing session started_at to created_at
MattBudz 7977b45
explain why for_record can't use the polymorphic record: shorthand
MattBudz f897319
rename EditingSession.acquire to acquire! (raises on invalid records)
MattBudz ae5c789
use class_names for the cancel link's data-behavior tokens
MattBudz c1a3654
Remove user from the unique index
aapomm 9fecb36
Merge branch 'develop' into editing-sessions/add-edit-locking-2
aapomm 9f2ef29
remove local SQLite databases committed by accident
MattBudz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| .edit-locked { | ||
| padding: 3rem 2rem; | ||
| text-align: center; | ||
|
|
||
| .actions { | ||
| display: flex; | ||
| gap: 0.75rem; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| .description { | ||
| color: var(--text-muted); | ||
| font-size: 1rem; | ||
| line-height: 1.6; | ||
| margin: 0 auto 2rem; | ||
| max-width: 28rem; | ||
| } | ||
|
|
||
| .editor { | ||
| align-items: center; | ||
| background: var(--secondary-bg); | ||
| border-radius: 0.5rem; | ||
| display: flex; | ||
| gap: 0.75rem; | ||
| justify-content: center; | ||
| margin: 0 auto 2rem; | ||
| max-width: 22rem; | ||
| padding: 1rem 1.5rem; | ||
| } | ||
|
|
||
| .editor-info { | ||
| text-align: left; | ||
| } | ||
|
|
||
| .editor-name { | ||
| display: block; | ||
| font-size: 1rem; | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| .editor-status { | ||
| align-items: center; | ||
| color: var(--text-muted); | ||
| display: flex; | ||
| font-size: 0.85rem; | ||
| gap: 0.35rem; | ||
| } | ||
|
|
||
| .heading { | ||
| color: var(--text-default); | ||
| font-size: 1.4rem; | ||
| font-weight: 700; | ||
| margin-bottom: 0.75rem; | ||
| } | ||
|
|
||
| .icon { | ||
| align-items: center; | ||
| background: $orange-100; | ||
| border-radius: 50%; | ||
| display: flex; | ||
| height: 4.5rem; | ||
| justify-content: center; | ||
| margin: 0 auto 1.5rem; | ||
| width: 4.5rem; | ||
|
|
||
| i { | ||
| color: $orange-500; | ||
| font-size: 1.8rem; | ||
| } | ||
| } | ||
|
|
||
| .pulse { | ||
| animation: edit-locked-pulse 2s infinite; | ||
| background: var(--brand-bg); | ||
| border-radius: 50%; | ||
| display: inline-block; | ||
| height: 0.5rem; | ||
| width: 0.5rem; | ||
| } | ||
| } | ||
|
|
||
| @keyframes edit-locked-pulse { | ||
| 0%, 100% { opacity: 1; } | ||
| 50% { opacity: 0.4; } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| module EditLockable | ||
| extend ActiveSupport::Concern | ||
|
|
||
| protected | ||
|
|
||
| def acquire_edit_session(record) | ||
| EditingSession.acquire(record_type: record.class.name, record_id: record.id, user: current_user) | ||
| end | ||
|
|
||
| def check_edit_lock | ||
| record = lockable_record | ||
| competing_sessions = EditingSession.for_record(record).active.by_others(current_user) | ||
|
|
||
| if competing_sessions.any? && params[:force] != 'true' | ||
| @locked_by = competing_sessions.includes(:user).map(&:user) | ||
| @locked_record = record | ||
| @back_path = url_from(request.referer) || root_path | ||
| render 'shared/edit_locked' | ||
| return | ||
| end | ||
|
|
||
| acquire_edit_session(record) | ||
| end | ||
|
|
||
| def lockable_record | ||
| @lockable_record ||= | ||
| instance_variable_get("@#{controller_name.singularize}") || | ||
| send("set_or_initialize_#{controller_name.singularize}") | ||
| end | ||
|
|
||
| def release_edit_session(record) | ||
| EditingSession.for_record(record).where(user: current_user).destroy_all | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| class EditingSession < ApplicationRecord | ||
| ALLOWED_RECORD_TYPES = %w[Issue].freeze | ||
| STALE_AFTER = 1.day | ||
|
|
||
| belongs_to :user | ||
| belongs_to :record, polymorphic: true | ||
|
|
||
| validates :record_type, presence: true, inclusion: { in: ALLOWED_RECORD_TYPES } | ||
|
|
||
| scope :active, -> { where(started_at: STALE_AFTER.ago..) } | ||
| scope :by_others, ->(user) { where.not(user: user) } | ||
| scope :for_record, ->(record) { | ||
| where(record_type: record.class.name, record_id: record.id) | ||
| } | ||
| scope :stale, -> { where(started_at: ...STALE_AFTER.ago) } | ||
|
|
||
| def self.acquire(record_type:, record_id:, user:) | ||
|
MattBudz marked this conversation as resolved.
Outdated
|
||
| purge_stale_for(record_type: record_type, record_id: record_id) | ||
| create_or_find_by!(record_type: record_type, record_id: record_id, user: user) | ||
| end | ||
|
|
||
| def self.purge_stale_for(record_type:, record_id:) | ||
| where(record_type: record_type, record_id: record_id).stale.destroy_all | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,3 +18,4 @@ | |
| <% end %> | ||
| <% end %> | ||
| </div> | ||
| </div> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| <% content_for :title, 'Record locked' %> | ||
|
|
||
| <div class="content-container mt-4"> | ||
| <div class="edit-locked"> | ||
| <div class="icon"> | ||
| <i class="fa-solid fa-lock"></i> | ||
| </div> | ||
|
|
||
| <h3 class="heading"> | ||
| This <%= @locked_record.model_name.human.downcase %> is currently being edited | ||
| </h3> | ||
|
|
||
| <p class="description"> | ||
| Another team member is currently editing this record. Go back, or edit | ||
| anyway and risk overwriting their changes. | ||
| </p> | ||
|
|
||
| <% @locked_by.each do |user| %> | ||
| <div class="editor"> | ||
| <%= avatar_image(user, size: 48) %> | ||
| <div class="editor-info"> | ||
| <span class="editor-name"><%= user.name %></span> | ||
| <span class="editor-status"> | ||
| <span class="pulse"></span> Currently editing | ||
| </span> | ||
| </div> | ||
| </div> | ||
| <% end %> | ||
|
|
||
| <div class="actions"> | ||
| <%= link_to @back_path, class: 'btn btn-primary' do %> | ||
| <i class="fa-solid fa-arrow-left"></i> Go back | ||
| <% end %> | ||
|
|
||
| <%= link_to url_for(request.parameters.merge(force: 'true')), class: 'btn btn-outline-danger' do %> | ||
| <i class="fa-solid fa-lock-open"></i> Edit anyway | ||
| <% end %> | ||
| </div> | ||
| </div> | ||
| </div> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| class CreateEditingSessions < ActiveRecord::Migration[8.0] | ||
| def change | ||
| create_table :editing_sessions do |t| | ||
| t.references :user, null: false, foreign_key: true | ||
| t.references :record, polymorphic: true, null: false | ||
| t.datetime :started_at, null: false, precision: nil, default: -> { 'CURRENT_TIMESTAMP' } | ||
|
MattBudz marked this conversation as resolved.
Outdated
|
||
|
|
||
| t.index [:user_id, :record_type, :record_id], unique: true, name: 'index_editing_sessions_uniqueness' | ||
| end | ||
| end | ||
| end | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| FactoryBot.define do | ||
| factory :editing_session do | ||
| association :user | ||
| record_type { 'Issue' } | ||
| record_id { create(:issue).id } | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| require 'rails_helper' | ||
|
|
||
| describe 'Edit locking multi-actor flow' do | ||
| let(:project) { Project.new } | ||
| let(:issue) { create(:issue, node: project.issue_library) } | ||
| let(:password) { 'spec-password' } | ||
|
|
||
| before do | ||
| Configuration.find_or_create_by(name: 'admin:password') | ||
| .update!(value: BCrypt::Password.create(password)) | ||
| end | ||
|
|
||
| def sign_in_as(username) | ||
| visit login_path | ||
| fill_in 'Username', with: username | ||
| fill_in 'Password', with: password | ||
| click_button 'Log in' | ||
| end | ||
|
|
||
| it 'locks the record for a second editor, lets them bypass it, and releases the lock on save' do | ||
| Capybara.using_session(:user_a) { sign_in_as('user-a@example.com') } | ||
| Capybara.using_session(:user_b) { sign_in_as('user-b@example.com') } | ||
|
|
||
| Capybara.using_session(:user_a) do | ||
| visit edit_project_issue_path(project, issue) | ||
| expect(page).to have_content('Edit issue') | ||
| end | ||
|
|
||
| Capybara.using_session(:user_b) do | ||
| visit edit_project_issue_path(project, issue) | ||
| expect(page).to have_content('currently being edited') | ||
| expect(page).to have_content('user-a@example.com') | ||
|
|
||
| click_link 'Go back' | ||
| expect(page).not_to have_content('currently being edited') | ||
| end | ||
|
|
||
| Capybara.using_session(:user_b) do | ||
| visit edit_project_issue_path(project, issue) | ||
| click_link 'Edit anyway' | ||
| expect(page).to have_content('Edit issue') | ||
| end | ||
|
|
||
| Capybara.using_session(:user_a) do | ||
| find('.btn-states button[type="submit"]').click | ||
| expect(page).to have_content('Issue updated.') | ||
| end | ||
|
|
||
| user_a = User.find_by(email: 'user-a@example.com') | ||
| expect(EditingSession.for_record(issue).where(user: user_a)).not_to exist | ||
| end | ||
| end |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.