From 81ee54fff47bc27ae07b8c93d0b77259643f6e29 Mon Sep 17 00:00:00 2001
From: echarp <emmanuel.charpentier@free.fr>
Date: Sat, 11 Oct 2014 16:12:07 +0200
Subject: [PATCH] =?UTF-8?q?L'=C3=A9dition=20concurrent=20d'un=20=C3=A9v?=
 =?UTF-8?q?=C3=A9nement=20est=20maintenant=20bloqu=C3=A9e?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 app/assets/stylesheets/all.css.sass           |  3 +-
 app/controllers/events_controller.rb          | 11 +++++-
 app/controllers/moderations_controller.rb     | 14 +++++--
 app/views/events/_form.html.haml              |  9 +++--
 app/views/events/show.text.haml               |  2 +-
 app/views/moderations/edit.html.haml          |  4 +-
 config/locales/views/en.yml                   | 18 +++++----
 config/locales/views/fr.yml                   | 38 ++++++++++++++-----
 ...141011100700_add_lock_version_to_events.rb |  7 ++++
 db/schema.rb                                  | 25 ++++++------
 test/controllers/events_controller_test.rb    | 34 +++++++++--------
 .../moderations_controller_test.rb            | 12 +++++-
 test/fixtures/events.yml                      |  3 ++
 13 files changed, 118 insertions(+), 62 deletions(-)
 create mode 100644 db/migrate/20141011100700_add_lock_version_to_events.rb

diff --git a/app/assets/stylesheets/all.css.sass b/app/assets/stylesheets/all.css.sass
index 1bdd089a..f2ff416d 100644
--- a/app/assets/stylesheets/all.css.sass
+++ b/app/assets/stylesheets/all.css.sass
@@ -63,8 +63,7 @@ h3.warning
   margin: 0
   padding: 1em
   text-align: center
-  border-color: #de2b0f
-  background-color: #f04124
+  background-color: orange
   +box-shadow(0 0 0.3em gray)
   +inline-block()
   +border-radius(1em)
diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb
index 1da1f671..0384ff45 100644
--- a/app/controllers/events_controller.rb
+++ b/app/controllers/events_controller.rb
@@ -7,6 +7,7 @@ class EventsController < ApplicationController
   before_action :check_secret, only:
     [:edit, :preview, :update, :cancel, :destroy]
   before_action :set_mailer_host
+  rescue_from ActiveRecord::StaleObjectError, with: :locked
 
   def index
     respond_to do |format|
@@ -105,8 +106,9 @@ class EventsController < ApplicationController
   # through.
   def event_params
     params.require(:event)
-      .permit :title, :start_time, :end_time, :description, :address, :city,
-              :region, :locality, :url, :contact, :submitter, :tags
+      .permit :lock_version, :title, :start_time, :end_time, :description,
+              :address, :city, :region, :locality, :url, :contact, :submitter,
+              :tags
   end
 
   # Check that you can only edit an existing event if you know its secret
@@ -131,4 +133,9 @@ class EventsController < ApplicationController
     # Send an update mail to moderators
     ModerationMailer.update(@older_event, @event, nil).deliver
   end
+
+  def locked
+    redirect_to edit_event_url(@event, secret: @event.secret),
+                alert: t('staleObjectError')
+  end
 end
diff --git a/app/controllers/moderations_controller.rb b/app/controllers/moderations_controller.rb
index 49f767ee..4ad59fd7 100644
--- a/app/controllers/moderations_controller.rb
+++ b/app/controllers/moderations_controller.rb
@@ -3,6 +3,7 @@ class ModerationsController < ApplicationController
   before_action :authenticate_user!
   before_action :set_moderation, :set_mailer_host, only:
     [:show, :edit, :preview, :update, :validate, :accept, :refuse, :destroy]
+  rescue_from ActiveRecord::StaleObjectError, with: :locked
 
   def index
     @events = Event.unmoderated
@@ -19,7 +20,7 @@ class ModerationsController < ApplicationController
   def update
     @older_mod = Event.new @event.attributes
     respond_to do |format|
-      if @moderation.update(moderation_params) && send_moderation_mails
+      if @moderation.update_attributes(moderation_params) && send_mails
         format.html { redirect_to moderations_path, notice: t('.ok') }
         format.json { head :no_content }
       else
@@ -71,8 +72,9 @@ class ModerationsController < ApplicationController
   # through.
   def moderation_params
     params.require(:event)
-      .permit :title, :start_time, :end_time, :description, :address, :city,
-              :region, :locality, :url, :contact, :submitter, :tags
+      .permit :lock_version, :title, :start_time, :end_time, :description,
+              :address, :city, :region, :locality, :url, :contact, :submitter,
+              :tags
   end
 
   # Useful to manage absolute url in mails
@@ -80,7 +82,7 @@ class ModerationsController < ApplicationController
     ActionMailer::Base.default_url_options[:host] = request.host_with_port
   end
 
-  def send_moderation_mails
+  def send_mails
     # Send an update mail to moderators
     ModerationMailer.update(@older_mod, @moderation, current_user).deliver
   end
@@ -112,4 +114,8 @@ class ModerationsController < ApplicationController
       @reason = t "moderations.refuse.reason_#{params[:reason]}_long"
     end
   end
+
+  def locked
+    redirect_to edit_moderation_url(@moderation), alert: t('staleObjectError')
+  end
 end
diff --git a/app/views/events/_form.html.haml b/app/views/events/_form.html.haml
index 52198b07..bd187c4f 100644
--- a/app/views/events/_form.html.haml
+++ b/app/views/events/_form.html.haml
@@ -1,13 +1,14 @@
 = form_for @event, url: (@moderation ? moderation_path(@moderation) : @event.persisted? ? event_path(@event) : nil) do |f|
+  - if @event.persisted? 
+    = f.hidden_field :lock_version
+    - unless @moderation
+      = hidden_field_tag :secret, params[:secret]
+
   - if @event.errors.any?
     #flash_messages
       - @event.errors.full_messages.each do |msg|
         %p.flash.alert= msg
 
-  - if @event.persisted?
-    - unless @moderation
-      = hidden_field_tag :secret, params[:secret]
-
   .field.title
     = f.label :title
     = f.text_field :title, required: true, size: 70,
diff --git a/app/views/events/show.text.haml b/app/views/events/show.text.haml
index f1bfe8e1..4b8a07ef 100644
--- a/app/views/events/show.text.haml
+++ b/app/views/events/show.text.haml
@@ -1,11 +1,11 @@
 =====================================================
-#{Event.human_attribute_name(:locality).concat(':').ljust 12   } #{t "attributes.locality_#{@event.locality}"}
 #{Event.human_attribute_name(:title).concat(':').ljust 12      } #{@event.title}
 #{Event.human_attribute_name(:start_time).concat(':').ljust 12 } #{l @event.start_time, format: :at}
 #{Event.human_attribute_name(:end_time).concat(':').ljust 12   } #{l @event.end_time, format: :at}
 #{Event.human_attribute_name(:address).concat(':').ljust 12    } #{@event.address}
 #{Event.human_attribute_name(:city).concat(':').ljust 12       } #{@event.city}
 #{Event.human_attribute_name(:region).concat(':').ljust 12     } #{@event.related_region}
+#{Event.human_attribute_name(:locality).concat(':').ljust 12   } #{t "attributes.locality_#{@event.locality}"}
 #{Event.human_attribute_name(:url).concat(':').ljust 12        } #{@event.url}
 #{Event.human_attribute_name(:contact).concat(':').ljust 12    } #{@event.contact}
 #{Event.human_attribute_name(:submitter).concat(':').ljust 12  } #{@event.submitter}
diff --git a/app/views/moderations/edit.html.haml b/app/views/moderations/edit.html.haml
index 531264e7..9edde274 100644
--- a/app/views/moderations/edit.html.haml
+++ b/app/views/moderations/edit.html.haml
@@ -3,7 +3,9 @@
   =t '.title'
 
 - if @moderation.moderated?
-  %h3.warning=t '.warning'
+  %h3.warning
+    %em.fa.fa-warning
+    =t '.warning'
 
 %fieldset
   %legend
diff --git a/config/locales/views/en.yml b/config/locales/views/en.yml
index d9fbbb7b..b75f7d9f 100644
--- a/config/locales/views/en.yml
+++ b/config/locales/views/en.yml
@@ -7,6 +7,8 @@ en:
   refuse: Refuse
   destroy: Destroy
   logout: Disconnect
+  staleObjectError: Sorry, your modification was rejected because someone else
+    already intervened
 
   # Translatings screens
   layouts:
@@ -71,14 +73,14 @@ it more readable or agreable.
         It will appear online as soon as a moderator validates it.
     edit:
       title: Edit an event
-      warning: Warning, this event is already moderated. Any modification will
-        be immediately visible on the site.
+      warning: 'Event already moderated: any modification will be immediately
+        visible on the site'
       forbidden: You are not authorised to modify this event
       preview: Previsualisation
       edit: Edition
     preview:
-      warning: Warning, this event is already moderated. Any modification will
-        be immediately visible on the site.
+      warning: 'Event already moderated: any modification will be immediately
+        visible on the site'
     update:
       ok: Your event was updated
     form:
@@ -104,8 +106,8 @@ it more readable or agreable.
       visualise: Visualise
     cancel:
       title: Cancel event
-      already_moderated: Warning, this event is already moderated. This
-        cancellation will remove it from Agenda du Libre.
+      already_moderated: 'Event already moderated: this cancellation will
+        remove it from Agenda du Libre'
       confirm: Do you confirm this event cancellation?
       preview: Event visualisation
       ok: Yes
@@ -168,8 +170,8 @@ Example: `%{daylimit}`"
     edit:
       title: Edit event
       moderation: Moderation
-      warning: Warning, this event is already moderated. Any modification will
-        be immediately visible on the site.
+      warning: 'Event already moderated: any modification will be immediately
+        visible on the site'
       preview: Previsualisation
       edit: Edition
     update:
diff --git a/config/locales/views/fr.yml b/config/locales/views/fr.yml
index 3325197c..490635b6 100644
--- a/config/locales/views/fr.yml
+++ b/config/locales/views/fr.yml
@@ -7,6 +7,8 @@ fr:
   refuse: Refuser
   destroy: Supprimer
   logout: Se déconnecter
+  staleObjectError: Désolé, votre modification est rejetée car une autre
+    personne est déjà intervenue
 
   # Traductions des écrans
   layouts:
@@ -63,28 +65,44 @@ fr:
         l'aura validé.
     edit:
       title: Éditer un événement
-      warning: Attention, cet événement est déjà modéré. Toute modification
-        sera immédiatement visible sur le site.
+      warning: 'Événement déjà modéré: toute modification sera immédiatement
+        visible sur le site'
       forbidden: Vous n'êtes pas authorisé à modifier cet événement
       preview: Prévisualisation
       edit: Édition
     preview:
-      warning: Attention, cet événement est déjà modéré. Toute modification
-        sera immédiatement visible sur le site.
+      warning: 'Événement déjà modéré: toute modification sera immédiatement
+        visible sur le site'
     update:
       ok: Votre événement a été mis à jour
     form:
       title_helper: Décrivez en moins de 5 mots votre événement, sans y
         indiquer le lieu, la ville ou la date
+      description_helper: Décrivez de la manière la plus complète possible
+        votre événement
       address_helper: "*Associée à la ville et la région, elle générerera une
-carte [OpenStreetMap](http://www.openstreetmap.org), affichée aux côtés de
-l'événement*"
+        carte [OpenStreetMap](http://www.openstreetmap.org), affichée aux côtés
+        de l'événement*"
+      url_helper: _Lien **direct** vers une page donnant plus d'informations
+        sur l'événement (lieu précis, horaire précis, programme précis...)_
+      contact_helper: _Adresse e-mail de contact, affichée de manière peu
+        compréhensible par les spammeurs_
+      submitter_helper: _Adresse e-mail du soumetteur de l'événement. Elle ne
+        sera utilisée que par les modérateurs pour contacter la personne ayant
+        proposé l'événement, pour l'informer de sa validation ou de son rejet.
+        Si cette adresse n'est pas présente, l'adresse de contact sera
+        utilisée_
+      tags_helper: _Tags pour l'événement. Les tags sont séparés par des
+        espaces. Un tag ne peut contenir que des lettres minuscules, des
+        chiffres et des tirets. Dans les tags, indiquez le nom de la ou des
+        associations organisatrices. N'indiquez pas le nom de la ville ou de la
+        région._
       save: Valider
       visualise: Visualiser
     cancel:
       title: Annulation de l'événement
-      already_moderated: Attention, cet événement est déjà modéré. Cette
-        annulation le fera disparaître de l'Agenda du Libre.
+      already_moderated: "Événement déjà modéré: cette annulation le fera
+        disparaître de l'Agenda du Libre"
       confirm: Confirmez-vous l'annulation de cet événement?
       preview: Visualisation de l'événement
       ok: Oui
@@ -152,8 +170,8 @@ Exemple: `%{daylimit}`"
     edit:
       title: Éditer un événement
       moderation: Modération
-      warning: Attention, cet événement est déjà modéré. Toute modification
-        sera immédiatement visible sur le site.
+      warning: 'Événement déjà modéré: toute modification sera
+        immédiatement visible sur le site'
       preview: Prévisualisation
       edit: Édition
     update:
diff --git a/db/migrate/20141011100700_add_lock_version_to_events.rb b/db/migrate/20141011100700_add_lock_version_to_events.rb
new file mode 100644
index 00000000..52870c23
--- /dev/null
+++ b/db/migrate/20141011100700_add_lock_version_to_events.rb
@@ -0,0 +1,7 @@
+# Add optimistic locking to events, so that moderators won't risk overwriting
+# each others' work
+class AddLockVersionToEvents < ActiveRecord::Migration
+  def change
+    add_column :events, :lock_version, :integer, default: 0, null: false
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 99c7d3d5..f472edd4 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -12,7 +12,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20140823111115) do
+ActiveRecord::Schema.define(version: 20141011100700) do
 
   create_table "active_admin_comments", force: true do |t|
     t.string   "namespace"
@@ -25,9 +25,9 @@ ActiveRecord::Schema.define(version: 20140823111115) do
     t.datetime "updated_at"
   end
 
-  add_index "active_admin_comments", ["author_type", "author_id"], name: "index_active_admin_comments_on_author_type_and_author_id"
-  add_index "active_admin_comments", ["namespace"], name: "index_active_admin_comments_on_namespace"
-  add_index "active_admin_comments", ["resource_type", "resource_id"], name: "index_active_admin_comments_on_resource_type_and_resource_id"
+  add_index "active_admin_comments", ["author_type", "author_id"], name: "index_active_admin_comments_on_author_type_and_author_id", using: :btree
+  add_index "active_admin_comments", ["namespace"], name: "index_active_admin_comments_on_namespace", using: :btree
+  add_index "active_admin_comments", ["resource_type", "resource_id"], name: "index_active_admin_comments_on_resource_type_and_resource_id", using: :btree
 
   create_table "admin_users", force: true do |t|
     t.string   "email",                  default: "", null: false
@@ -44,8 +44,8 @@ ActiveRecord::Schema.define(version: 20140823111115) do
     t.datetime "updated_at"
   end
 
-  add_index "admin_users", ["email"], name: "index_admin_users_on_email", unique: true
-  add_index "admin_users", ["reset_password_token"], name: "index_admin_users_on_reset_password_token", unique: true
+  add_index "admin_users", ["email"], name: "index_admin_users_on_email", unique: true, using: :btree
+  add_index "admin_users", ["reset_password_token"], name: "index_admin_users_on_reset_password_token", unique: true, using: :btree
 
   create_table "cities", force: true do |t|
     t.string  "name",                  default: "", null: false
@@ -57,7 +57,7 @@ ActiveRecord::Schema.define(version: 20140823111115) do
     t.float   "longitude",  limit: 24
   end
 
-  add_index "cities", ["name"], name: "cities_name"
+  add_index "cities", ["name"], name: "cities_name", using: :btree
 
   create_table "events", force: true do |t|
     t.string   "title",                        default: "", null: false
@@ -73,16 +73,17 @@ ActiveRecord::Schema.define(version: 20140823111115) do
     t.integer  "moderated",                    default: 0,  null: false
     t.string   "tags",                         default: "", null: false
     t.string   "secret",                       default: "", null: false
-    t.datetime "decision_time",                             null: false
-    t.datetime "submission_time",                           null: false
+    t.datetime "decision_time"
+    t.datetime "submission_time"
     t.string   "moderator_mail_id", limit: 32
     t.string   "submitter_mail_id", limit: 32
     t.text     "address"
-    t.float    "latitude"
-    t.float    "longitude"
+    t.float    "latitude",          limit: 24
+    t.float    "longitude",         limit: 24
+    t.integer  "lock_version",                 default: 0,  null: false
   end
 
-  add_index "events", ["start_time", "end_time"], name: "events_date"
+  add_index "events", ["start_time", "end_time"], name: "events_date", using: :btree
 
   create_table "lugs", force: true do |t|
     t.integer "region",     default: 0,  null: false
diff --git a/test/controllers/events_controller_test.rb b/test/controllers/events_controller_test.rb
index b885e556..dfb4dac2 100644
--- a/test/controllers/events_controller_test.rb
+++ b/test/controllers/events_controller_test.rb
@@ -11,7 +11,7 @@ class EventsControllerTest < ActionController::TestCase
   test 'should get index' do
     get :index
     assert_response :success
-    assert_not_nil assigns(:events)
+    assert_not_nil assigns :events
   end
 
   test 'should get new' do
@@ -23,8 +23,7 @@ class EventsControllerTest < ActionController::TestCase
     assert_no_difference 'Event.count' do
       post :preview_create, event: {
         title: @event.title,
-        start_time: @event.start_time,
-        end_time: @event.end_time,
+        start_time: @event.start_time, end_time: @event.end_time,
         description: @event.description,
         city: @event.city,
         region: @event.related_region,
@@ -42,8 +41,7 @@ class EventsControllerTest < ActionController::TestCase
     assert_difference 'Event.count' do
       post :create, event: {
         title: @event.title,
-        start_time: @event.start_time,
-        end_time: @event.end_time,
+        start_time: @event.start_time, end_time: @event.end_time,
         description: @event.description,
         city: @event.city,
         region: @event.related_region,
@@ -59,11 +57,7 @@ class EventsControllerTest < ActionController::TestCase
 
   test 'should not create event' do
     assert_no_difference 'Event.count' do
-      post :create, event: {
-        title: @event.title,
-        city: @event.city,
-        region: @event.related_region
-      }
+      post :create, event: { title: @event.title }
 
       assert_not_empty assigns(:event).errors.messages
     end
@@ -75,7 +69,7 @@ class EventsControllerTest < ActionController::TestCase
   end
 
   test 'should get edit' do
-    get :edit, id: @event, secret: 'MyString'
+    get :edit, id: @event, secret: @event.secret
     assert_response :success
   end
 
@@ -86,7 +80,7 @@ class EventsControllerTest < ActionController::TestCase
 
   test 'should preview' do
     assert_no_difference 'Event.count' do
-      patch :preview, id: @event, secret: 'MyString', event: {
+      patch :preview, id: @event, secret: @event.secret, event: {
         title: @event.title
       }
 
@@ -97,7 +91,7 @@ class EventsControllerTest < ActionController::TestCase
   end
 
   test 'should update event' do
-    patch :update, id: @event, secret: 'MyString', event: {
+    patch :update, id: @event, secret: @event.secret, event: {
       title: @event.title
     }
 
@@ -106,21 +100,29 @@ class EventsControllerTest < ActionController::TestCase
   end
 
   test 'should not update event' do
-    patch :update, id: @event, secret: 'MyString', event: {
+    patch :update, id: @event, secret: @event.secret, event: {
       title: nil
     }
 
     assert_not_empty assigns(:event).errors.messages
   end
 
+  test 'can not update event concurrently' do
+    patch :update, id: @event, secret: @event.secret, event: {
+      lock_version: @event.lock_version - 1
+    }
+
+    assert_redirected_to edit_event_url(@event, secret: @event.secret)
+  end
+
   test 'should get cancel page' do
-    get :cancel, id: @event, secret: 'MyString'
+    get :cancel, id: @event, secret: @event.secret
     assert_response :success
   end
 
   test 'should destroy event' do
     assert_difference('Event.count', -1) do
-      delete :destroy, id: @event, secret: 'MyString'
+      delete :destroy, id: @event, secret: @event.secret
     end
 
     assert_redirected_to events_path
diff --git a/test/controllers/moderations_controller_test.rb b/test/controllers/moderations_controller_test.rb
index ecc6d784..6b0adb8d 100644
--- a/test/controllers/moderations_controller_test.rb
+++ b/test/controllers/moderations_controller_test.rb
@@ -58,7 +58,7 @@ class ModerationsControllerTest < ActionController::TestCase
   end
 
   test 'should update event' do
-    patch :update, id: @moderation, secret: 'MyString', event: {
+    patch :update, id: @moderation, event: {
       title: @moderation.title,
       start_time: @moderation.start_time,
       end_time: @moderation.end_time,
@@ -73,13 +73,21 @@ class ModerationsControllerTest < ActionController::TestCase
   end
 
   test 'should not update event' do
-    patch :update, id: @moderation, secret: 'MyString', event: {
+    patch :update, id: @moderation, event: {
       title: nil
     }
 
     assert_not_empty assigns(:moderation).errors
   end
 
+  test 'can not update event concurrently' do
+    patch :update, id: @moderation, event: {
+      lock_version: @moderation.lock_version - 1
+    }
+
+    assert_redirected_to edit_moderation_path @moderation
+  end
+
   test 'should reject event' do
     assert_difference 'Event.count', -1 do
       delete :destroy, id: @moderation
diff --git a/test/fixtures/events.yml b/test/fixtures/events.yml
index 3b14157d..fb1c969b 100644
--- a/test/fixtures/events.yml
+++ b/test/fixtures/events.yml
@@ -18,6 +18,7 @@ one:
   submission_time: <%= 3.days.ago %>
   moderator_mail_id: MyString
   submitter_mail_id: MyString
+  lock_version: 0
 
 two:
   title: MyString
@@ -37,6 +38,7 @@ two:
   submission_time: 2013-12-28 16:04:56
   moderator_mail_id: MyString
   submitter_mail_id: MyString
+  lock_version: 0
 
 proposed:
   title: MyString
@@ -56,3 +58,4 @@ proposed:
   submission_time: 2013-12-28 16:04:56
   moderator_mail_id: MyString
   submitter_mail_id: MyString
+  lock_version: 0