Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Merge pull request #11 from alphagov/artefact-audit-trail

Show a record on the edit form of all changes made to artefacts.

Reviewed by @jystewart.
  • Loading branch information...
commit e1ec482d0854a2e8d1a4a645504668e4d111d578 2 parents 6ef1600 + 500decb
@fatbusinessman fatbusinessman authored
View
2  Gemfile
@@ -33,7 +33,7 @@ gem 'lograge'
if ENV['CONTENT_MODELS_DEV']
gem "govuk_content_models", path: '../govuk_content_models'
else
- gem "govuk_content_models", "~> 0.2.4"
+ gem "govuk_content_models", "0.3.0"
end
if ENV['BUNDLE_DEV']
View
4 Gemfile.lock
@@ -117,7 +117,7 @@ GEM
json
gherkin (2.7.3)
json (>= 1.4.6)
- govuk_content_models (0.2.5)
+ govuk_content_models (0.3.0)
bson_ext
differ
gds-api-adapters
@@ -295,7 +295,7 @@ DEPENDENCIES
gds-sso (~> 1.2.0)
gds-warmup-controller (= 0.1.0)
gelf
- govuk_content_models (~> 0.2.4)
+ govuk_content_models (= 0.3.0)
launchy
lograge
minitest
View
12 app/controllers/artefacts_controller.rb
@@ -26,10 +26,18 @@ def new
end
def edit
+ # Construct a list of actions, with embedded diffs
+ # The reason for appending the nil is so that the initial action is
+ # included: the DiffEnabledAction class handles the case where the previous
+ # action does not exist
+ reverse_actions = @artefact.actions.reverse
+ @actions = (reverse_actions + [nil]).each_cons(2).map { |action, previous|
+ DiffEnabledAction.new(action, previous)
+ }
end
def create
- @artefact.save
+ @artefact.save_as current_user
respond_with @artefact, location: @artefact.admin_url(params.slice(:return_to))
end
@@ -54,7 +62,7 @@ def update
return
end
- saved = @artefact.update_attributes(parameters_to_use)
+ saved = @artefact.update_attributes_as(current_user, parameters_to_use)
flash[:notice] = saved ? 'Panopticon item updated' : 'Failed to save item'
if saved && params[:commit] == 'Save and continue editing'
View
4 app/helpers/artefacts_helper.rb
@@ -6,4 +6,8 @@ def need_url(artefact)
def published_url(artefact)
Plek.current.find('www') + "/#{artefact.slug}"
end
+
+ def human_timestamp(timestamp)
+ timestamp ? timestamp.strftime("%d/%m/%Y %R") : "(no timestamp)"
+ end
end
View
35 app/models/diff_enabled_action.rb
@@ -0,0 +1,35 @@
+class DiffEnabledAction
+
+ # Decorator for the ArtefactAction class, which can express what has changed.
+
+ extend Forwardable
+
+ def_delegators :@action, :action_type, :snapshot, :created_at, :user
+
+ def initialize(action, previous = nil)
+ @action, @previous = action, previous
+ end
+
+ def initial?
+ ! @previous
+ end
+
+ def changes
+ # If this is an initial action, fake out the previous snapshot as an empty
+ # hash, for consistency with the general case
+ previous_snapshot = @previous ? @previous.snapshot : {}
+ snapshots = [previous_snapshot, @action.snapshot]
+
+ changed_keys.reduce({}) { |changes, key|
+ changes.merge key => snapshots.map { |snapshot| snapshot[key] }
+ }
+ end
+
+ def changed_keys
+ return @action.snapshot.keys unless @previous
+
+ (@action.snapshot.keys | @previous.snapshot.keys).reject { |key|
+ @action.snapshot[key] == @previous.snapshot[key]
+ }
+ end
+end
View
37 app/views/artefacts/_history.html.erb
@@ -0,0 +1,37 @@
+<div class="well">
+ <dl class="dl-horizontal">
+ <% actions.each do |action| %>
+ <dt><%= human_timestamp(action.created_at) %></dt>
+ <dd>
+ <p>
+ <% if ! action.user and action.action_type == "snapshot" %>
+ Automatic snapshot
+ <% else %>
+ <%= action.user || "An unknown user" %>
+ <% case action.action_type %>
+ <% when "create" %>
+ created this artefact
+ <% when "update" %>
+ updated this artefact
+ <% else %>
+ did a "<%= action.action_type %>" action
+ <% end %>
+ <% end %>
+ </p>
+ <% unless action.initial? %>
+ <% if action.changes.empty? %>
+ No changes
+ <% else %>
+ Changed fields:
+ <ul>
+ <% action.changes.each do |key, values| %>
+ <li><code><%= key %></code>
+ from <%= values[0].inspect %> to <%= values[1].inspect %></li>
+ <% end %>
+ </ul>
+ <% end %>
+ <% end %>
+ </dd>
+ <% end %>
+ </dl>
+</div>
View
15 app/views/artefacts/edit.html.erb
@@ -2,4 +2,17 @@
<div class="page-header">
<h1>Editing <%= @artefact.name %></h1>
</div>
-<%= render 'form', artefact: @artefact %>
+<div class="tabbable">
+ <ul class="nav nav-tabs">
+ <li class="active"><a href="#edit" data-toggle="tab">Edit</a></li>
+ <li><a href="#history" data-toggle="tab">History</a></li>
+ </ul>
+ <div class="tab-content">
+ <div class="tab-pane active" id="edit">
+ <%= render 'form', artefact: @artefact %>
+ </div>
+ <div class="tab-pane" id="history">
+ <%= render 'history', actions: @actions %>
+ </div>
+ </div>
+</div>
View
23 lib/tasks/snapshots.rake
@@ -0,0 +1,23 @@
+namespace :actions do
+ task :create_snapshots => :environment do
+ success_count = failure_count = skip_count = 0
+ Artefact.all.each do |artefact|
+ if artefact.actions.empty?
+ artefact.record_action "snapshot"
+ if artefact.save
+ STDERR.puts "Recorded snapshot for '#{artefact.name}'" if verbose
+ success_count += 1
+ else
+ STDERR.puts "Failed to save '#{artefact.name}'" if verbose
+ failure_count += 1
+ end
+ else
+ STDERR.puts "Skipping snapshot for '#{artefact.name}'" if verbose
+ skip_count += 1
+ end
+ end
+ STDERR.puts "#{success_count} succeeded"
+ STDERR.puts "#{failure_count} failed"
+ STDERR.puts "#{skip_count} skipped"
+ end
+end
View
57 test/functional/artefacts_controller_test.rb
@@ -57,6 +57,25 @@ class ArtefactsControllerTest < ActionController::TestCase
assert_equal "Whatever", parsed["name"]
assert parsed["id"].present?
end
+
+ should "record a create action on the artefact with the current user" do
+ post(
+ :create,
+ format: "json",
+ :slug => 'whatever',
+ :kind => 'guide',
+ :owning_app => 'publisher',
+ :rendering_app => 'frontend',
+ :name => 'Whatever',
+ :need_id => 1
+ )
+ parsed = JSON.parse(response.body)
+ artefact_id = parsed["id"]
+ artefact = Artefact.find(artefact_id)
+ assert_equal 1, artefact.actions.size
+ assert_equal "create", artefact.actions.first.action_type
+ assert_equal stub_user, artefact.actions.first.user
+ end
end
context "GET /artefacts/:id" do
@@ -86,6 +105,27 @@ class ArtefactsControllerTest < ActionController::TestCase
assert_equal 404, response.code.to_i
end
+ context "GET /artefacts/:id/edit" do
+ should "Include history" do
+ # Create and update the artefact to set up some actions
+ artefact = Artefact.create!(
+ :slug => 'whatever',
+ :kind => 'guide',
+ :owning_app => 'publisher',
+ :name => 'Whatever',
+ :need_id => 1,
+ )
+ artefact.update_attributes_as stub_user, name: "Changed"
+
+ get :edit, id: artefact.id, format: :html
+
+ # Check the actions: note reverse order
+ actions = assigns["actions"]
+ assert_equal ["update", "create"], actions.map(&:action_type)
+ assert_equal [false, true], actions.map(&:initial?)
+ end
+ end
+
end
context "PUT /artefacts/:id" do
@@ -101,6 +141,23 @@ class ArtefactsControllerTest < ActionController::TestCase
assert_equal "Changed", artefact.reload.name
end
+ should "Record the action and responsible user" do
+ artefact = Artefact.create!(
+ :slug => 'whatever',
+ :kind => 'guide',
+ :owning_app => 'publisher',
+ :rendering_app => 'frontend',
+ :name => 'Whatever',
+ :need_id => 1
+ )
+
+ put :update, id: artefact.id, format: :json, name: "Changed"
+ assert_response :success
+
+ artefact.reload
+ assert_equal stub_user, artefact.actions.last.user
+ end
+
should "Update our primary section and ensure it persists into sections" do
@tags = FactoryGirl.create_list(:tag, 3)
artefact = Artefact.create!(:slug => 'whatever', :kind => 'guide',
View
11 test/test_helper.rb
@@ -36,9 +36,16 @@ def app
Panopticon::Application
end
+ def stub_user
+ @stub_user ||= FactoryGirl.create(:user, :name => 'Stub User')
+ end
+
def login_as_stub_user
- temp_user = FactoryGirl.create(:user, :name => 'Stub User')
- request.env['warden'] = stub(:authenticate! => true, :authenticated? => true, :user => temp_user)
+ request.env['warden'] = stub(
+ :authenticate! => true,
+ :authenticated? => true,
+ :user => stub_user
+ )
end
def teardown
View
88 test/unit/diff_enabled_action_test.rb
@@ -0,0 +1,88 @@
+require_relative '../test_helper'
+
+class DiffEnabledActionTest < ActiveSupport::TestCase
+
+ test "should report being an initial action" do
+ action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {}
+ )
+ a = DiffEnabledAction.new(action, nil)
+ assert a.initial?
+ end
+
+ test "should report changes on an initial action" do
+ action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {"fish" => 1, "badger" => true, "walrus" => "yes"}
+ )
+ a = DiffEnabledAction.new(action, nil)
+ assert_equal(
+ {"fish" => [nil, 1], "badger" => [nil, true], "walrus" => [nil, "yes"]},
+ a.changes
+ )
+ end
+
+ test "should show no changes from itself" do
+ action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {}
+ )
+ a = DiffEnabledAction.new(action, action)
+ refute a.initial?
+ assert a.changed_keys.empty?
+ assert a.changes.empty?
+ end
+
+ test "should show changes" do
+ first_action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {"cheese" => 1, "walrus" => "I am a string"}
+ )
+ second_action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {"cheese" => 1, "walrus" => "I am another string"}
+ )
+ a = DiffEnabledAction.new(second_action, first_action)
+ assert_equal ["walrus"], a.changed_keys
+ assert_equal(
+ {"walrus" => ["I am a string", "I am another string"]},
+ a.changes
+ )
+ end
+
+ test "should handle removed keys" do
+ first_action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {"cheese" => 1, "walrus" => "I am a string"}
+ )
+ second_action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {"cheese" => 1}
+ )
+ a = DiffEnabledAction.new(second_action, first_action)
+ assert_equal ["walrus"], a.changed_keys
+ assert_equal(
+ {"walrus" => ["I am a string", nil]},
+ a.changes
+ )
+ end
+
+ test "should handle added keys" do
+ first_action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {"cheese" => 1}
+ )
+ second_action = ArtefactAction.new(
+ action_type: "update",
+ snapshot: {"cheese" => 1, "walrus" => "I am a string"}
+ )
+ a = DiffEnabledAction.new(second_action, first_action)
+ assert_equal ["walrus"], a.changed_keys
+ assert_equal(
+ {"walrus" => [nil, "I am a string"]},
+ a.changes
+ )
+ end
+
+end
Please sign in to comment.
Something went wrong with that request. Please try again.