diff options
24 files changed, 451 insertions, 156 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index cd2f4a038..e26bb9e31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ - Add Mark/View private reports permission #2306 - Store more original stuff on moderation. - Sort user updates in reverse date order. + - Improve update display on admin report edit page. + - Keep all moderation history, and show in report/update admin. #2329 + - Bugfixes: + - Restore map zoom out when navigating to /around from /report. #1649 - Open311 improvements: - Fix bug in contact group handling. #2323 - Improve validation of fetched reports timestamps. #2327 diff --git a/bin/handlemail-support b/bin/handlemail-support index 76fcb4f4e..975773bd9 100755 --- a/bin/handlemail-support +++ b/bin/handlemail-support @@ -22,6 +22,7 @@ BEGIN { } use FixMyStreet; +use FixMyStreet::Email::Sender; use mySociety::HandleMail; my %data = mySociety::HandleMail::get_message(); diff --git a/bin/update-schema b/bin/update-schema index a2f20c306..1e1afa72c 100755 --- a/bin/update-schema +++ b/bin/update-schema @@ -212,6 +212,8 @@ else { # (assuming schema change files are never half-applied, which should be the case) sub get_db_version { return 'EMPTY' if ! table_exists('problem'); + return '0065' if constraint_contains('admin_log_object_type_check', 'moderation'); + return '0064' if index_exists('moderation_original_data_problem_id_comment_id_idx'); return '0063' if column_exists('moderation_original_data', 'extra'); return '0062' if column_exists('users', 'created'); return '0061' if column_exists('body', 'extra'); @@ -320,3 +322,9 @@ sub function_exists { my $fn = shift; return $db->dbh->selectrow_array('select count(*) from pg_proc where proname = ?', {}, $fn); } + +# Returns true if an index exists +sub index_exists { + my $idx = shift; + return $db->dbh->selectrow_array('select count(*) from pg_indexes where indexname = ?', {}, $idx); +} diff --git a/db/downgrade_0064---0063.sql b/db/downgrade_0064---0063.sql new file mode 100644 index 000000000..7cce67680 --- /dev/null +++ b/db/downgrade_0064---0063.sql @@ -0,0 +1,7 @@ +BEGIN; + +DROP INDEX moderation_original_data_problem_id_comment_id_idx; +ALTER TABLE moderation_original_data + ADD CONSTRAINT moderation_original_data_comment_id_key UNIQUE (comment_id); + +COMMIT; diff --git a/db/downgrade_0065---0064.sql b/db/downgrade_0065---0064.sql new file mode 100644 index 000000000..455627b77 --- /dev/null +++ b/db/downgrade_0065---0064.sql @@ -0,0 +1,14 @@ +BEGIN; + +DELETE FROM admin_log WHERE object_type = 'moderation'; + +ALTER TABLE admin_log DROP CONSTRAINT admin_log_object_type_check; + +ALTER TABLE admin_log ADD CONSTRAINT admin_log_object_type_check CHECK ( + object_type = 'problem' + OR object_type = 'update' + OR object_type = 'user' +); + +COMMIT; + diff --git a/db/schema.sql b/db/schema.sql index a448e05fa..c97e8d585 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -427,6 +427,7 @@ create table admin_log ( object_type = 'problem' or object_type = 'update' or object_type = 'user' + or object_type = 'moderation' ), object_id integer not null, action text not null, @@ -441,7 +442,7 @@ create table moderation_original_data ( -- Problem details problem_id int references problem(id) ON DELETE CASCADE not null, - comment_id int references comment(id) ON DELETE CASCADE unique, + comment_id int references comment(id) ON DELETE CASCADE, title text null, detail text null, -- or text for comment @@ -456,6 +457,7 @@ create table moderation_original_data ( latitude double precision, longitude double precision ); +create index moderation_original_data_problem_id_comment_id_idx on moderation_original_data(problem_id, comment_id); create table user_body_permissions ( id serial not null primary key, diff --git a/db/schema_0064-allow-multiple-moderations.sql b/db/schema_0064-allow-multiple-moderations.sql new file mode 100644 index 000000000..5fee96282 --- /dev/null +++ b/db/schema_0064-allow-multiple-moderations.sql @@ -0,0 +1,8 @@ +BEGIN; + +ALTER TABLE moderation_original_data + DROP CONSTRAINT moderation_original_data_comment_id_key; +CREATE INDEX moderation_original_data_problem_id_comment_id_idx + ON moderation_original_data(problem_id, comment_id); + +COMMIT; diff --git a/db/schema_0065-add-moderation-admin-log.sql b/db/schema_0065-add-moderation-admin-log.sql new file mode 100644 index 000000000..9a5385db7 --- /dev/null +++ b/db/schema_0065-add-moderation-admin-log.sql @@ -0,0 +1,13 @@ +BEGIN; + +ALTER TABLE admin_log DROP CONSTRAINT admin_log_object_type_check; + +ALTER TABLE admin_log ADD CONSTRAINT admin_log_object_type_check CHECK ( + object_type = 'problem' + OR object_type = 'update' + OR object_type = 'user' + OR object_type = 'moderation' +); + +COMMIT; + diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm index 60dea4c1b..75f83c6b6 100644 --- a/perllib/FixMyStreet/App/Controller/Admin.pm +++ b/perllib/FixMyStreet/App/Controller/Admin.pm @@ -889,7 +889,7 @@ sub report_edit : Path('report_edit') : Args(1) { $c->stash->{updates} = [ $c->model('DB::Comment') - ->search( { problem_id => $problem->id }, { order_by => 'created' } ) + ->search( { problem_id => $problem->id }, { order_by => [ 'created', 'id' ] } ) ->all ]; if (my $rotate_photo_param = $self->_get_rotate_photo_param($c)) { diff --git a/perllib/FixMyStreet/App/Controller/Moderate.pm b/perllib/FixMyStreet/App/Controller/Moderate.pm index 90396fa07..154f09176 100644 --- a/perllib/FixMyStreet/App/Controller/Moderate.pm +++ b/perllib/FixMyStreet/App/Controller/Moderate.pm @@ -23,9 +23,9 @@ data to change. - user to be from_body - user to have a "moderate" record in user_body_permissions -The original data of the report is stored in moderation_original_data, so -that it can be reverted/consulted if required. All moderation events are -stored in admin_log. +The original and previous data of the report is stored in +moderation_original_data, so that it can be reverted/consulted if required. +All moderation events are stored in admin_log. =head1 SEE ALSO @@ -55,7 +55,7 @@ sub report : Chained('moderate') : PathPart('report') : CaptureArgs(1) { $c->forward('/auth/check_csrf_token'); - my $original = $problem->find_or_new_related( moderation_original_data => { + $c->stash->{history} = $problem->new_related( moderation_original_data => { title => $problem->title, detail => $problem->detail, photo => $problem->photo, @@ -63,10 +63,10 @@ sub report : Chained('moderate') : PathPart('report') : CaptureArgs(1) { longitude => $problem->longitude, latitude => $problem->latitude, category => $problem->category, - extra => $problem->extra, + $problem->extra ? (extra => $problem->extra) : (), }); + $c->stash->{original} = $problem->moderation_original_data || $c->stash->{history}; $c->stash->{problem} = $problem; - $c->stash->{problem_original} = $original; $c->stash->{moderation_reason} = $c->get_param('moderation_reason') // ''; } @@ -119,23 +119,32 @@ sub moderating_user_name { return $user->from_body ? $user->from_body->name : _('an administrator'); } -sub report_moderate_audit : Private { - my ($self, $c, @types) = @_; +sub moderate_log_entry : Private { + my ($self, $c, $object_type, @types) = @_; my $user = $c->user->obj; my $reason = $c->stash->{'moderation_reason'}; - my $problem = $c->stash->{problem} or die; + my $object = $object_type eq 'update' ? $c->stash->{comment} : $c->stash->{problem}; my $types_csv = join ', ' => @types; + # We attach the log to the moderation entry if present, or the object if not (hiding) $c->model('DB::AdminLog')->create({ action => 'moderation', user => $user, admin_user => moderating_user_name($user), - object_id => $problem->id, - object_type => 'problem', + object_id => $c->stash->{history}->id || $object->id, + object_type => $c->stash->{history}->id ? 'moderation' : $object_type, reason => (sprintf '%s (%s)', $reason, $types_csv), }); +} + +sub report_moderate_audit : Private { + my ($self, $c, @types) = @_; + + my $problem = $c->stash->{problem} or die; + + $c->forward('moderate_log_entry', [ 'problem', @types ]); if ($problem->user->email_verified && $c->cobrand->send_moderation_notifications) { my $token = $c->model("DB::Token")->create({ @@ -143,6 +152,7 @@ sub report_moderate_audit : Private { data => { id => $problem->id } }); + my $types_csv = join ', ' => @types; $c->send_email( 'problem-moderated.txt', { to => [ [ $problem->user->email, $problem->name ] ], types => $types_csv, @@ -150,6 +160,7 @@ sub report_moderate_audit : Private { problem => $problem, report_uri => $c->stash->{report_uri}, report_complain_uri => $c->stash->{cobrand_base} . '/contact?m=' . $token->token, + moderated_data => $c->stash->{history}, }); } } @@ -172,29 +183,22 @@ sub report_moderate_hide : Private { sub moderate_text : Private { my ($self, $c, $thing) = @_; - my ($object, $original, $param); + my $object = $c->stash->{comment} || $c->stash->{problem}; + my $param = $c->stash->{comment} ? 'update_' : 'problem_'; + my $thing_for_original_table = $thing; - if (my $comment = $c->stash->{comment}) { - $object = $comment; - $original = $c->stash->{comment_original}; - $param = 'update_'; - # Update 'text' field is stored in original table's 'detail' field - $thing_for_original_table = 'detail' if $thing eq 'text'; - } else { - $object = $c->stash->{problem}; - $original = $c->stash->{problem_original}; - $param = 'problem_'; - } + # Update 'text' field is stored in original table's 'detail' field + $thing_for_original_table = 'detail' if $c->stash->{comment} && $thing eq 'text'; my $old = $object->$thing; - my $original_thing = $original->$thing_for_original_table; + my $original_thing = $c->stash->{original}->$thing_for_original_table; my $new = $c->get_param($param . 'revert_' . $thing) ? $original_thing : $c->get_param($param . $thing); if ($new ne $old) { - $original->insert unless $original->in_storage; + $c->stash->{history}->insert; $object->update({ $thing => $new }); return $thing_for_original_table; } @@ -205,18 +209,11 @@ sub moderate_text : Private { sub moderate_boolean : Private { my ( $self, $c, $thing, $reverse ) = @_; - my ($object, $original, $param); - if (my $comment = $c->stash->{comment}) { - $object = $comment; - $original = $c->stash->{comment_original}; - $param = 'update_'; - } else { - $object = $c->stash->{problem}; - $original = $c->stash->{problem_original}; - $param = 'problem_'; - } + my $object = $c->stash->{comment} || $c->stash->{problem}; + my $param = $c->stash->{comment} ? 'update_' : 'problem_'; + my $original = $c->stash->{original}->photo; - return if $thing eq 'photo' && !$original->photo; + return if $thing eq 'photo' && !$original; my $new; if ($reverse) { @@ -227,9 +224,9 @@ sub moderate_boolean : Private { my $old = $object->$thing ? 1 : 0; if ($new != $old) { - $original->insert unless $original->in_storage; + $c->stash->{history}->insert; if ($thing eq 'photo') { - $object->update({ $thing => $new ? $original->photo : undef }); + $object->update({ $thing => $new ? $original : undef }); } else { $object->update({ $thing => $new }); } @@ -241,14 +238,7 @@ sub moderate_boolean : Private { sub moderate_extra : Private { my ($self, $c) = @_; - my ($object, $original); - if (my $comment = $c->stash->{comment}) { - $object = $comment; - $original = $c->stash->{comment_original}; - } else { - $object = $c->stash->{problem}; - $original = $c->stash->{problem_original}; - } + my $object = $c->stash->{comment} || $c->stash->{problem}; my $changed; my @extra = grep { /^extra\./ } keys %{$c->req->params}; @@ -257,12 +247,12 @@ sub moderate_extra : Private { my $old = $object->get_extra_metadata($field_name) || ''; my $new = $c->get_param($_); if ($new ne $old) { - $original->insert unless $original->in_storage; $object->set_extra_metadata($field_name, $new); $changed = 1; } } if ($changed) { + $c->stash->{history}->insert; $object->update; return 'extra'; } @@ -272,7 +262,6 @@ sub moderate_location : Private { my ($self, $c) = @_; my $problem = $c->stash->{problem}; - my $original = $c->stash->{problem_original}; my $moved = $c->forward('/admin/report_edit_location', [ $problem ]); if (!$moved) { @@ -283,7 +272,7 @@ sub moderate_location : Private { } if ($moved == 2) { - $original->insert unless $original->in_storage; + $c->stash->{history}->insert; $problem->update; return 'location'; } @@ -299,12 +288,11 @@ sub moderate_category : Private { $c->forward('/admin/categories_for_point'); my $problem = $c->stash->{problem}; - my $original = $c->stash->{problem_original}; my $changed = $c->forward( '/admin/report_edit_category', [ $problem, 1 ] ); # It might need to set_report_extras in future if ($changed) { - $original->insert unless $original->in_storage; + $c->stash->{history}->insert; $problem->update; return 'category'; } @@ -317,14 +305,14 @@ sub update : Chained('report') : PathPart('update') : CaptureArgs(1) { # Make sure user can moderate this update $c->detach unless $comment && $c->user->can_moderate($comment); - my $original = $comment->find_or_new_related( moderation_original_data => { + $c->stash->{history} = $comment->new_related( moderation_original_data => { detail => $comment->text, photo => $comment->photo, anonymous => $comment->anonymous, - extra => $comment->extra, + $comment->extra ? (extra => $comment->extra) : (), }); $c->stash->{comment} = $comment; - $c->stash->{comment_original} = $original; + $c->stash->{original} = $comment->moderation_original_data || $c->stash->{history}; } sub moderate_update : Chained('update') : PathPart('') : Args(0) { @@ -338,27 +326,7 @@ sub moderate_update : Chained('update') : PathPart('') : Args(0) { $c->forward('moderate_extra'), $c->forward('moderate_boolean', [ 'photo' ]); - $c->detach( 'update_moderate_audit', \@types ) -} - -sub update_moderate_audit : Private { - my ($self, $c, @types) = @_; - - my $user = $c->user->obj; - my $reason = $c->stash->{'moderation_reason'}; - my $problem = $c->stash->{problem} or die; - my $comment = $c->stash->{comment} or die; - - my $types_csv = join ', ' => @types; - - $c->model('DB::AdminLog')->create({ - action => 'moderation', - user => $user, - admin_user => moderating_user_name($user), - object_id => $comment->id, - object_type => 'update', - reason => (sprintf '%s (%s)', $reason, $types_csv), - }); + $c->detach('moderate_log_entry', [ 'update', @types ]); } sub update_moderate_hide : Private { @@ -369,7 +337,7 @@ sub update_moderate_hide : Private { if ($c->get_param('update_hide')) { $comment->hide; - $c->detach( 'update_moderate_audit', ['hide'] ); # break chain here. + $c->detach('moderate_log_entry', [ 'update', 'hide' ]); # break chain here. } return; } diff --git a/perllib/FixMyStreet/DB/Result/Comment.pm b/perllib/FixMyStreet/DB/Result/Comment.pm index 738b58f82..31e9f3e63 100644 --- a/perllib/FixMyStreet/DB/Result/Comment.pm +++ b/perllib/FixMyStreet/DB/Result/Comment.pm @@ -6,7 +6,6 @@ package FixMyStreet::DB::Result::Comment; use strict; use warnings; -use FixMyStreet::Template; use base 'DBIx::Class::Core'; __PACKAGE__->load_components("FilterColumn", "InflateColumn::DateTime", "EncodedColumn"); @@ -70,8 +69,8 @@ __PACKAGE__->add_columns( { data_type => "timestamp", is_nullable => 1 }, ); __PACKAGE__->set_primary_key("id"); -__PACKAGE__->might_have( - "moderation_original_data", +__PACKAGE__->has_many( + "moderation_original_datas", "FixMyStreet::DB::Result::ModerationOriginalData", { "foreign.comment_id" => "self.id" }, { cascade_copy => 0, cascade_delete => 0 }, @@ -90,8 +89,8 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07035 @ 2015-08-13 16:33:38 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:ZR+YNA1Jej3s+8mr52iq6Q +# Created by DBIx::Class::Schema::Loader v0.07035 @ 2018-11-20 16:13:59 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:5w/4Og9uCy54lGyyJiLzxA # __PACKAGE__->load_components("+FixMyStreet::DB::RABXColumn"); @@ -99,9 +98,11 @@ __PACKAGE__->rabx_column('extra'); use Moo; use namespace::clean -except => [ 'meta' ]; +use FixMyStreet::Template; with 'FixMyStreet::Roles::Abuser', 'FixMyStreet::Roles::Extra', + 'FixMyStreet::Roles::Moderation', 'FixMyStreet::Roles::PhotoSet'; my $stz = sub { @@ -176,17 +177,6 @@ sub url { return "/report/" . $self->problem_id . '#update_' . $self->id; } -=head2 latest_moderation_log_entry - -Return most recent ModerationLog object - -=cut - -sub latest_moderation_log_entry { - my $self = shift; - return $self->admin_log_entries->search({ action => 'moderation' }, { order_by => { -desc => 'id' } })->first; -} - __PACKAGE__->has_many( "admin_log_entries", "FixMyStreet::DB::Result::AdminLog", @@ -197,24 +187,24 @@ __PACKAGE__->has_many( } ); -# we already had the `moderation_original_data` rel above, as inferred by -# Schema::Loader, but that doesn't know about the problem_id mapping, so we now -# (slightly hackishly) redefine here: -# -# we also add cascade_delete, though this seems to be insufficient. -# -# TODO: should add FK on moderation_original_data field for this, to get S::L to -# pick up without hacks. - +# This will return the oldest moderation_original_data, if any. +# The plural can be used to return all entries. __PACKAGE__->might_have( "moderation_original_data", "FixMyStreet::DB::Result::ModerationOriginalData", { "foreign.comment_id" => "self.id", "foreign.problem_id" => "self.problem_id", }, - { cascade_copy => 0, cascade_delete => 1 }, + { order_by => 'id', + rows => 1, + cascade_copy => 0, cascade_delete => 1 }, ); +sub moderation_filter { + my $self = shift; + { problem_id => $self->problem_id }; +} + =head2 meta_line Returns a string to be used on a report update, describing some of the metadata diff --git a/perllib/FixMyStreet/DB/Result/ModerationOriginalData.pm b/perllib/FixMyStreet/DB/Result/ModerationOriginalData.pm index 3e6482658..01ae1d6e1 100644 --- a/perllib/FixMyStreet/DB/Result/ModerationOriginalData.pm +++ b/perllib/FixMyStreet/DB/Result/ModerationOriginalData.pm @@ -47,7 +47,6 @@ __PACKAGE__->add_columns( { data_type => "double precision", is_nullable => 1 }, ); __PACKAGE__->set_primary_key("id"); -__PACKAGE__->add_unique_constraint("moderation_original_data_comment_id_key", ["comment_id"]); __PACKAGE__->belongs_to( "comment", "FixMyStreet::DB::Result::Comment", @@ -67,14 +66,140 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07035 @ 2018-11-13 10:48:41 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:OQAXPriTc3G2jKFPw0TqdQ +# Created by DBIx::Class::Schema::Loader v0.07035 @ 2018-11-20 16:13:59 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:zFOhQnS4WfVzD7qHxaAr6w use Moo; +use Text::Diff; +use Data::Dumper; with 'FixMyStreet::Roles::Extra'; __PACKAGE__->load_components("+FixMyStreet::DB::RABXColumn"); __PACKAGE__->rabx_column('extra'); +sub admin_log { + my $self = shift; + my $rs = $self->result_source->schema->resultset("AdminLog"); + my $log = $rs->search({ + object_id => $self->id, + object_type => 'moderation', + })->first; + return $log; +} + +sub compare_with { + my ($self, $other) = @_; + if ($self->comment_id) { + my $new_detail = $other->can('text') ? $other->text : $other->detail; + return { + detail => string_diff($self->detail, $new_detail), + photo => $self->compare_photo($other), + anonymous => $self->compare_anonymous($other), + extra => $self->compare_extra($other), + }; + } + return { + title => string_diff($self->title, $other->title), + detail => string_diff($self->detail, $other->detail), + photo => $self->compare_photo($other), + anonymous => $self->compare_anonymous($other), + coords => $self->compare_coords($other), + category => string_diff($self->category, $other->category, single => 1), + extra => $self->compare_extra($other), + } +} + +sub compare_anonymous { + my ($self, $other) = @_; + string_diff( + $self->anonymous ? _('Yes') : _('No'), + $other->anonymous ? _('Yes') : _('No'), + ); +} + +sub compare_coords { + my ($self, $other) = @_; + my $old = join ',', $self->latitude, $self->longitude; + my $new = join ',', $other->latitude, $other->longitude; + string_diff($old, $new, single => 1); +} + +sub compare_photo { + my ($self, $other) = @_; + + my $old = $self->photo || ''; + my $new = $other->photo || ''; + return '' if $old eq $new; + + $old = [ split /,/, $old ]; + $new = [ split /,/, $new ]; + + my $diff = Algorithm::Diff->new( $old, $new ); + my (@added, @deleted); + while ( $diff->Next ) { + next if $diff->Same; + push @deleted, $diff->Items(1); + push @added, $diff->Items(2); + } + return (join ', ', map { + "<del style='background-color:#fcc'>$_</del>"; + } @deleted) . (join ', ', map { + "<ins style='background-color:#cfc'>$_</ins>"; + } @added); +} + +sub compare_extra { + my ($self, $other) = @_; + + my $old = $self->get_extra_metadata; + my $new = $other->get_extra_metadata; + + my $both = { %$old, %$new }; + my @all_keys = sort keys %$both; + my @s; + foreach (@all_keys) { + if ($old->{$_} && $new->{$_}) { + push @s, string_diff("$_ = $old->{$_}", "$_ = $new->{$_}"); + } elsif ($new->{$_}) { + push @s, string_diff("", "$_ = $new->{$_}"); + } else { + push @s, string_diff("$_ = $old->{$_}", ""); + } + } + return join ', ', @s; +} + +sub string_diff { + my ($old, $new, %options) = @_; + + return '' if $old eq $new; + + $old = FixMyStreet::Template::html_filter($old); + $new = FixMyStreet::Template::html_filter($new); + + if ($options{single}) { + $old = [ $old ]; + $new = [ $new ]; + } + $old = [ split //, $old ] unless ref $old; + $new = [ split //, $new ] unless ref $new; + my $diff = Algorithm::Diff->new( $old, $new ); + my $string; + while ($diff->Next) { + my $d = $diff->Diff; + if ($d & 1) { + my $deleted = join '', $diff->Items(1); + $string .= "<del style='background-color:#fcc'>$deleted</del>"; + } + my $inserted = join '', $diff->Items(2); + if ($d & 2) { + $string .= "<ins style='background-color:#cfc'>$inserted</ins>"; + } else { + $string .= $inserted; + } + } + return $string; +} + 1; diff --git a/perllib/FixMyStreet/DB/Result/Problem.pm b/perllib/FixMyStreet/DB/Result/Problem.pm index 0b8bc4de0..a222ea1f6 100644 --- a/perllib/FixMyStreet/DB/Result/Problem.pm +++ b/perllib/FixMyStreet/DB/Result/Problem.pm @@ -177,11 +177,15 @@ __PACKAGE__->has_one( { cascade_copy => 0, cascade_delete => 0 }, ); +# This will return the oldest moderation_original_data, if any. +# The plural can be used to return all entries. __PACKAGE__->might_have( "moderation_original_data", "FixMyStreet::DB::Result::ModerationOriginalData", { "foreign.problem_id" => "self.id" }, { where => { 'comment_id' => undef }, + order_by => 'id', + rows => 1, cascade_copy => 0, cascade_delete => 1 }, ); @@ -206,6 +210,7 @@ my $IM = eval { with 'FixMyStreet::Roles::Abuser', 'FixMyStreet::Roles::Extra', + 'FixMyStreet::Roles::Moderation', 'FixMyStreet::Roles::Translatable', 'FixMyStreet::Roles::PhotoSet'; @@ -958,17 +963,6 @@ sub as_hashref { return $out; } -=head2 latest_moderation_log_entry - -Return most recent ModerationLog object - -=cut - -sub latest_moderation_log_entry { - my $self = shift; - return $self->admin_log_entries->search({ action => 'moderation' }, { order_by => { -desc => 'id' } })->first; -} - __PACKAGE__->has_many( "admin_log_entries", "FixMyStreet::DB::Result::AdminLog", @@ -979,6 +973,11 @@ __PACKAGE__->has_many( } ); +sub moderation_filter { + my $self = shift; + { comment_id => undef }; +} + sub get_time_spent { my $self = shift; my $admin_logs = $self->admin_log_entries->search({}, diff --git a/perllib/FixMyStreet/ImageMagick.pm b/perllib/FixMyStreet/ImageMagick.pm index 26c5c6d74..af9f56478 100644 --- a/perllib/FixMyStreet/ImageMagick.pm +++ b/perllib/FixMyStreet/ImageMagick.pm @@ -3,6 +3,7 @@ package FixMyStreet::ImageMagick; use Moo; my $IM = eval { + return 0 if FixMyStreet->test_mode; require Image::Magick; Image::Magick->import; 1; diff --git a/perllib/FixMyStreet/Roles/Moderation.pm b/perllib/FixMyStreet/Roles/Moderation.pm new file mode 100644 index 000000000..f43b65208 --- /dev/null +++ b/perllib/FixMyStreet/Roles/Moderation.pm @@ -0,0 +1,41 @@ +package FixMyStreet::Roles::Moderation; +use Moo::Role; + +=head2 latest_moderation_log_entry + +Return most recent AdminLog object concerning moderation + +=cut + +sub latest_moderation_log_entry { + my $self = shift; + + my $latest = $self->moderation_original_datas->search( + $self->moderation_filter, + { order_by => { -desc => 'id' } })->first; + return unless $latest; + + my $rs = $self->result_source->schema->resultset("AdminLog"); + my $log = $rs->search({ + object_id => $latest->id, + object_type => 'moderation', + })->first; + return $log if $log; + + return $self->admin_log_entries->search({ action => 'moderation' }, { order_by => { -desc => 'id' } })->first; +} + +=head2 moderation_history + +Returns all moderation history, most recent first. + +=cut + +sub moderation_history { + my $self = shift; + return $self->moderation_original_datas->search( + $self->moderation_filter, + { order_by => { -desc => 'id' } })->all; +} + +1; diff --git a/t/app/controller/admin/search.t b/t/app/controller/admin/search.t index 497ac9fd6..f8e70cb7a 100644 --- a/t/app/controller/admin/search.t +++ b/t/app/controller/admin/search.t @@ -103,7 +103,7 @@ subtest 'report search' => sub { $update->update; $mech->get_ok('/admin/reports?search=' . $report->user->email); - $mech->content_like( qr{<tr [^>]*hidden[^>]*> \s* <td> \s* $u_id \s* </td>}xs ); + $mech->content_like( qr{<tr [^>]*hidden[^>]*> \s* <td[^>]*> \s* $u_id \s* </td>}xs ); $report->state('hidden'); $report->update; diff --git a/t/app/controller/moderate.t b/t/app/controller/moderate.t index e80355075..a064a88d4 100644 --- a/t/app/controller/moderate.t +++ b/t/app/controller/moderate.t @@ -126,6 +126,11 @@ subtest 'Problem moderation' => sub { $report->discard_changes; is $report->title, 'Good bad good'; is $report->detail, 'Good bad bad bad good bad'; + + my @history = $report->moderation_original_datas->search(undef, { order_by => 'id' })->all; + is @history, 2, 'Right number of entries'; + is $history[0]->title, 'Good bad good', 'Correct original title'; + is $history[1]->title, 'Good good', 'Correct second title'; }; subtest 'Make anonymous' => sub { @@ -454,4 +459,7 @@ subtest 'And do it as a superuser' => sub { $mech->content_contains('Moderated by an administrator'); }; +subtest 'Check moderation history in admin' => sub { + $mech->get_ok('/admin/report_edit/' . $report->id); +}; done_testing(); diff --git a/templates/email/default/problem-moderated.html b/templates/email/default/problem-moderated.html index 1a12446d0..142f27fc2 100644 --- a/templates/email/default/problem-moderated.html +++ b/templates/email/default/problem-moderated.html @@ -25,8 +25,8 @@ the team at <a href="[% report_complain_uri %]">[% report_complain_uri %]</a></p [% end_padded_box %] </th> [% WRAPPER '_email_sidebar.html' object = problem %] - <h2 style="[% h2_style %]">[% problem.moderation_original_data.title | html %]</h2> - <p style="[% secondary_p_style %]">[% problem.moderation_original_data.detail | html %]</p> + <h2 style="[% h2_style %]">[% moderated_data.title | html %]</h2> + <p style="[% secondary_p_style %]">[% moderated_data.detail | html %]</p> [% END %] [% INCLUDE '_email_bottom.html' %] diff --git a/templates/email/default/problem-moderated.txt b/templates/email/default/problem-moderated.txt index 7c1c3b5c2..ee219672f 100644 --- a/templates/email/default/problem-moderated.txt +++ b/templates/email/default/problem-moderated.txt @@ -15,11 +15,11 @@ The following data has been changed: Your report had the title: -[% problem.moderation_original_data.title %] +[% moderated_data.title %] And details: -[% problem.moderation_original_data.detail %] +[% moderated_data.detail %] [% UNLESS types == 'hide' %] You can see the report at [% report_uri %] diff --git a/templates/web/base/admin/list_updates.html b/templates/web/base/admin/list_updates.html index a28c4ff81..b23cd7ca3 100644 --- a/templates/web/base/admin/list_updates.html +++ b/templates/web/base/admin/list_updates.html @@ -9,24 +9,25 @@ <th>[% loc('Council') %]</th> <th>[% loc('Cobrand') %]</th> <th>[% loc('State') %]</th> - <th>[% loc('Text') %]</th> <th>*</th> </tr> [% FOREACH update IN updates -%] <tr[% ' class="adminhidden"' IF update.state == 'hidden' || update.problem.state == 'hidden' %]> - <td>[%- IF update.state == 'confirmed' && update.problem.state != 'hidden' -%] - [%- cobrand_data = update.cobrand_data; - cobrand_data = c.data_for_generic_update IF !update.cobrand; - IF cobrand_data; + <td rowspan=2> + [%~ IF update.state == 'confirmed' && update.problem.state != 'hidden' -%] + [%- cobrand_data = update.cobrand_data; + cobrand_data = c.data_for_generic_update IF !update.cobrand; + IF cobrand_data; uri = c.uri_for_email( '/report', update.problem.id, cobrand_data ); - ELSE; + ELSE; uri = c.uri_for_email( '/report', update.problem.id ); - END; - %] - <a href="[% uri %]#update_[% update.id %]" class="admin-offsite-link">[% update.id %]</a> - [%- ELSE %] - [%- update.id %] - [%- END -%]</td> + END; + %] + <a href="[% uri %]#update_[% update.id %]" class="admin-offsite-link">[% update.id %]</a> + [%- ELSE %] + [%- update.id %] + [%- END ~%] + </td> <td>[% PROCESS value_or_nbsp value=update.name %] <br>[% PROCESS value_or_nbsp value=update.user.email %] <br>[% loc('Anonymous') %]: [% IF update.anonymous %][% loc('Yes') %][% ELSE %][% loc('No') %][% END %] @@ -38,13 +39,15 @@ [% loc('Created:') %] [% PROCESS format_time time=update.created %] <br>[% loc('Confirmed:') %] [% PROCESS format_time time=update.confirmed %] </small></td> - <td>[% update.text | html %]</td> - <td> + <td rowspan=2> [% IF c.user.has_permission_to('report_edit', update.problem.bodies_str_ids) %] <a href="[% c.uri_for( 'update_edit', update.id ) %]">[% loc('Edit') %]</a> [% END %] </td> </tr> + <tr> + <td colspan=5>[% update.text | html %]</td> + </tr> [% END -%] </table> diff --git a/templates/web/base/admin/report_edit.html b/templates/web/base/admin/report_edit.html index 794e19e4c..175863549 100644 --- a/templates/web/base/admin/report_edit.html +++ b/templates/web/base/admin/report_edit.html @@ -182,6 +182,31 @@ class="admin-offsite-link">[% problem.latitude %], [% problem.longitude %]</a> </div> +[% moderation_history = problem.moderation_history %] +[% IF moderation_history %] +<div class="full-width" style="margin-bottom:-2em; padding-bottom: 2em"> + <h2>[% loc('Moderation history') %]</h2> + [% last_history = problem %] + <ul> + [% FOR history IN moderation_history %] + [% SET diff = history.compare_with(last_history) %] + [% SET log = history.admin_log %] + <li><i>[% tprintf(loc('Moderated by %s at %s'), log.user.name OR loc('an administrator'), prettify_dt(history.created)) %] + [%~ IF log.reason %]<br>“[% log.reason %]”[% END %]</i> + [% IF diff.title %]<br>[% loc('Subject:') %] [% diff.title %][% END %] + [% IF diff.detail %]<br>[% loc('Details:') %] [% diff.detail %][% END %] + [% IF diff.photo %]<br>[% loc('Photo') %]: [% diff.photo %][% END %] + [% IF diff.anonymous %]<br>[% loc('Anonymous:') %] [% diff.anonymous %][% END %] + [% IF diff.coords %]<br>[% loc('Latitude/Longitude:') %] [% diff.coords %][% END %] + [% IF diff.category %]<br>[% loc('Category:') %] [% diff.category %][% END %] + [% IF diff.extra %]<br>[% loc('Extra data:') %] [% diff.extra %][% END %] + </li> + [% last_history = history %] + [% END %] + </ul> +</div> +[% END %] + <div class="full-width full-width--bottom"> [% INCLUDE 'admin/list_updates.html' %] </div> diff --git a/templates/web/base/admin/update_edit.html b/templates/web/base/admin/update_edit.html index cb420fc67..fca9b1904 100644 --- a/templates/web/base/admin/update_edit.html +++ b/templates/web/base/admin/update_edit.html @@ -81,4 +81,24 @@ </ul> <input type="submit" class="btn" name="Submit changes" value="[% loc('Submit changes') %]" ></form> +[% moderation_history = update.moderation_history %] +[% IF moderation_history %] + <h2>[% loc('Moderation history') %]</h2> + [% last_history = update %] + <ul> + [% FOR history IN moderation_history %] + [% SET diff = history.compare_with(last_history) %] + [% SET log = history.admin_log %] + <li><i>[% tprintf(loc('Moderated by %s at %s'), log.user.name OR loc('an administrator'), prettify_dt(history.created)) %] + [%~ IF log.reason %]<br>“[% log.reason %]”[% END %]</i> + [% IF diff.detail %]<br>[% loc('Text:') %] [% diff.detail %][% END %] + [% IF diff.photo %]<br>[% loc('Photo') %]: [% diff.photo %][% END %] + [% IF diff.anonymous %]<br>[% loc('Anonymous:') %] [% diff.anonymous %][% END %] + [% IF diff.extra %]<br>[% loc('Extra data:') %] [% diff.extra %][% END %] + </li> + [% last_history = history %] + [% END %] + </ul> +[% END %] + [% INCLUDE 'admin/footer.html' %] diff --git a/web/cobrands/fixmystreet/fixmystreet.js b/web/cobrands/fixmystreet/fixmystreet.js index 9d3b9388f..15cc6b26a 100644 --- a/web/cobrands/fixmystreet/fixmystreet.js +++ b/web/cobrands/fixmystreet/fixmystreet.js @@ -935,6 +935,8 @@ $.extend(fixmystreet.set_up, { }, ajax_history: function() { + var around_map_state = null; + $('#map_sidebar').on('click', '.item-list--reports a', function(e) { if (e.metaKey || e.ctrlKey) { return; @@ -953,10 +955,30 @@ $.extend(fixmystreet.set_up, { fixmystreet.map.setCenter( marker.geometry.getBounds().getCenterLonLat(), fixmystreet.map.getNumZoomLevels() - 1 ); + // replaceState rather than pushState so the back button returns + // to the zoomed-out map with all pins. + history.replaceState({ + reportId: reportId, + reportPageUrl: reportPageUrl, + mapState: fixmystreet.maps.get_map_state() + }, null); } return; } + if (fixmystreet.page.match(/reports|around|my/)) { + around_map_state = fixmystreet.maps.get_map_state(); + // Preserve the current map state in the initial state so we can + // restore it if the user navigates back. This needs doing here, + // rather than the 'fake history' replaceState call that sets the + // initial state, because the map hasn't been loaded at that point. + if ('state' in history && history.state.initial) { + history.state.mapState = around_map_state; + // NB can't actually modify current state directly, needs a + // call to replaceState() + history.replaceState(history.state, null); + } + } fixmystreet.display.report(reportPageUrl, reportId, function() { // Since this navigation was the result of a user action, // we want to record the navigation as a state, so the user @@ -964,32 +986,44 @@ $.extend(fixmystreet.set_up, { if ('pushState' in history) { history.pushState({ reportId: reportId, - reportPageUrl: reportPageUrl + reportPageUrl: reportPageUrl, + mapState: fixmystreet.maps.get_map_state() }, null, reportPageUrl); } }); }); $('#map_sidebar').on('click', '.js-back-to-report-list', function(e) { - if (e.metaKey || e.ctrlKey) { - return; - } - - e.preventDefault(); - var reportListUrl = $(this).attr('href'); - fixmystreet.display.reports_list(reportListUrl, function() { - // Since this navigation was the result of a user action, - // we want to record the navigation as a state, so the user - // can return to it later using their Back button. - if ('pushState' in history) { - history.pushState({ initial: true }, null, reportListUrl); - } - }); + var report_list_url = $(this).attr('href'); + var map_state = around_map_state; + var set_map_state = true; + fixmystreet.back_to_reports_list(e, report_list_url, map_state, set_map_state); }); } }); +fixmystreet.back_to_reports_list = function(e, report_list_url, map_state, set_map_state) { + if (e.metaKey || e.ctrlKey) { + return; + } + e.preventDefault(); + fixmystreet.display.reports_list(report_list_url, function() { + // Since this navigation was the result of a user action, + // we want to record the navigation as a state, so the user + // can return to it later using their Back button. + if ('pushState' in history) { + history.pushState({ + initial: true, + mapState: map_state + }, null, report_list_url); + } + if (set_map_state && map_state) { + fixmystreet.maps.set_map_state(map_state); + } + }); +}; + fixmystreet.update_report_a_problem_btn = function() { var zoom = fixmystreet.map.getZoom(); var center = fixmystreet.map.getCenterWGS84(); @@ -1284,10 +1318,14 @@ fixmystreet.display = { // wrong (but what is shown is correct). $('.js-back-to-report-list').attr('href', fixmystreet.original.href); - // Problems nearby should act the same as 'Back to all reports' on around, - // but on /my and /reports should go to that around page. - if (fixmystreet.original.page == 'around') { - $sideReport.find('#key-tool-problems-nearby').addClass('js-back-to-report-list'); + // Problems nearby on /my should go to the around page, + // otherwise show reports within the current map view. + if (fixmystreet.original.page === 'around' || fixmystreet.original.page === 'reports') { + $sideReport.find('#key-tool-problems-nearby').click(function(e) { + var report_list_url = fixmystreet.original.href; + var map_state = fixmystreet.maps.get_map_state(); + fixmystreet.back_to_reports_list(e, report_list_url, map_state); + }); } fixmystreet.set_up.map_sidebar_key_tools(); fixmystreet.set_up.form_validation(); @@ -1467,6 +1505,10 @@ $(function() { // This popstate was just here because the hash changed. // (eg: mobile nav click.) We want to ignore it. } + if ('mapState' in e.state) { + fixmystreet.maps.set_map_state(e.state.mapState); + } + }); }, 0); }); diff --git a/web/js/map-OpenLayers.js b/web/js/map-OpenLayers.js index 66168925a..826c9eb6b 100644 --- a/web/js/map-OpenLayers.js +++ b/web/js/map-OpenLayers.js @@ -340,6 +340,22 @@ $.extend(fixmystreet.utils, { $('#loading-indicator').attr('aria-hidden', true); } } + }, + + get_map_state: function() { + var centre = fixmystreet.map.getCenter(); + return { + zoom: fixmystreet.map.getZoom(), + lat: centre.lat, + lon: centre.lon, + }; + }, + + set_map_state: function(state) { + fixmystreet.map.setCenter( + new OpenLayers.LonLat( state.lon, state.lat ), + state.zoom + ); } }); |