Centralise update creation to include fields.

Given the user, we can infer the name if not provided, and the extra
data if a staff user. We can also provide defaults for various other
fields. Always have superuser take precedence over from_body.

CHANGELOG.md

@@ -28,6 +28,7 @@
  - Display user name/email for contributed as reports. #2990
  - Interface for enabling anonymous reports for certain categories. #2989
  - Better sort admin user table.
+ - Centralise update creation to include fields.
  - Development improvements:
  - `#geolocate_link` is now easier to re-style. #3006
  - Links inside `#front-main` can be customised using `$primary_link_*` Sass variables. #3007

bin/fixmystreet.com/banes-close-reports

@@ -50,7 +50,6 @@ my $q = FixMyStreet::DB->resultset("Problem")->search({
 # Provide some variables to the archiving script
 FixMyStreet::Script::ArchiveOldEnquiries::update_options({
  user => $body->comment_user->id,
- user_name => $body->comment_user->name,
  closure_text => CLOSURE_TEXT,
  retain_alerts => 1,
  commit => $opts->commit,

bin/fixmystreet.com/buckinghamshire-flytipping

@@ -79,7 +79,6 @@ sub find_problems {
  # Provide some variables to the archiving script
  FixMyStreet::Script::ArchiveOldEnquiries::update_options({
  user => $body->comment_user->id,
- user_name => $body->comment_user->name,
  closure_text => $template->text,
  retain_alerts => $retain_alerts,
  commit => $opts->commit,

bin/one-off-update-staff

@@ -0,0 +1,38 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+BEGIN {
+ use File::Basename qw(dirname);
+ use File::Spec;
+ my $d = dirname(File::Spec->rel2abs($0));
+ require "$d/../setenv.pl";
+}
+
+use FixMyStreet::DB;
+
+my $rs = FixMyStreet::DB->resultset("Comment")->search({
+ 'user.from_body' => { '!=', undef },
+ 'user.is_superuser' => 0,
+ 'me.extra' => [ undef, { -not_like => '%is_body_user%' } ],
+}, {
+ "+columns" => ["user.from_body"],
+ join => 'user',
+});
+while (my $row = $rs->next) {
+ my $id = $row->user->{_column_data}->{from_body}; # Avoid DB lookups
+ $row->set_extra_metadata( is_body_user => $id );
+ $row->update;
+}
+
+$rs = FixMyStreet::DB->resultset("Comment")->search({
+ 'user.is_superuser' => 1,
+ 'me.extra' => [ undef, { -not_like => '%is_superuser%' } ],
+}, {
+ join => 'user',
+});
+while (my $row = $rs->next) {
+ $row->set_extra_metadata( is_superuser => 1 );
+ $row->update;
+}

perllib/FixMyStreet/App/Controller/Admin/Reports.pm

@@ -368,24 +368,10 @@ sub edit : Path('/admin/report_edit') : Args(1) {
  if ( $problem->state ne $old_state ) {
  $c->forward( '/admin/log_edit', [ $id, 'problem', 'state_change' ] );
 
- my $name = $c->user->moderating_user_name;
- my $extra = { is_superuser => 1 };
- if ($c->user->from_body) {
- delete $extra->{is_superuser};
- $extra->{is_body_user} = $c->user->from_body->id;
- }
- my $timestamp = \'current_timestamp';
  $problem->add_to_comments( {
  text => $c->stash->{update_text} || '',
- created => $timestamp,
- confirmed => $timestamp,
- user_id => $c->user->id,
- name => $name,
- mark_fixed => 0,
- anonymous => 0,
- state => 'confirmed',
+ user => $c->user->obj,
  problem_state => $problem->state,
- extra => $extra
  } );
  }
  $c->forward( '/admin/log_edit', [ $id, 'problem', 'edit' ] );
@@ -444,13 +430,7 @@ sub edit_category : Private {
  } else {
  $problem->add_to_comments({
  text => $update_text,
- created => \'current_timestamp',
- confirmed => \'current_timestamp',
- user_id => $c->user->id,
- name => $c->user->from_body ? $c->user->from_body->name : $c->user->name,
- state => 'confirmed',
- mark_fixed => 0,
- anonymous => 0,
+ user => $c->user->obj,
  });
  }
  $c->forward( '/admin/log_edit', [ $problem->id, 'problem', 'category_change' ] );

perllib/FixMyStreet/App/Controller/Admin/Triage.pm

@@ -117,27 +117,14 @@ sub update : Private {
  $c->stash->{problem}->update( { state => 'confirmed' } );
  $c->forward( '/admin/log_edit', [ $problem->id, 'problem', 'triage' ] );
 
- my $name = $c->user->moderating_user_name;
- my $extra = { is_superuser => 1 };
- if ($c->user->from_body) {
- delete $extra->{is_superuser};
- $extra->{is_body_user} = $c->user->from_body->id;
- }
-
+ my $extra;
  $extra->{triage_report} = 1;
  $extra->{holding_category} = $current_category;
  $extra->{new_category} = $new_category;
 
- my $timestamp = \'current_timestamp';
  my $comment = $problem->add_to_comments( {
  text => "Report triaged from $current_category to $new_category",
- created => $timestamp,
- confirmed => $timestamp,
- user_id => $c->user->id,
- name => $name,
- mark_fixed => 0,
- anonymous => 0,
- state => 'confirmed',
+ user => $c->user->obj,
  problem_state => $problem->state,
  extra => $extra,
  whensent => \'current_timestamp',

perllib/FixMyStreet/App/Controller/Moderate.pm

@@ -340,13 +340,8 @@ sub moderate_state : Private {
  $problem->state($new_state);
  $problem->add_to_comments( {
  text => $c->stash->{moderation_reason},
- created => \'current_timestamp',
- confirmed => \'current_timestamp',
- user_id => $c->user->id,
- name => $c->user->from_body ? $c->user->from_body->name : $c->user->name,
- state => 'confirmed',
- mark_fixed => 0,
- anonymous => $c->user->from_body ? 0 : 1,
+ user => $c->user->obj,
+ anonymous => $c->user->is_superuser || $c->user->from_body ? 0 : 1,
  problem_state => $new_state,
  } );
  return 'state';

perllib/FixMyStreet/App/Controller/Questionnaire.pm

@@ -206,16 +206,12 @@ sub submit_standard : Private {
  $update = $c->model('DB::Comment')->new(
  {
  problem => $problem,
- name => $problem->name,
  user => $problem->user,
  text => $update,
- state => 'confirmed',
  mark_fixed => $c->stash->{new_state} eq 'fixed - user' ? 1 : 0,
  mark_open => $c->stash->{new_state} eq 'confirmed' ? 1 : 0,
  lang => $c->stash->{lang_code},
  cobrand => $c->cobrand->moniker,
- cobrand_data => '',
- confirmed => \'current_timestamp',
  anonymous => $problem->anonymous,
  }
  );

perllib/FixMyStreet/App/Controller/Report.pm

@@ -569,16 +569,11 @@ sub inspect : Private {
  epoch => $saved_at
  );
  }
- my $name = $c->user->from_body ? $c->user->from_body->name : $c->user->name;
  $problem->add_to_comments( {
  text => $update_text,
  created => $timestamp,
  confirmed => $timestamp,
- user_id => $c->user->id,
- name => $name,
- state => 'confirmed',
- mark_fixed => 0,
- anonymous => 0,
+ user => $c->user->obj,
  %update_params,
  } );
  }

perllib/FixMyStreet/DB/Result/Comment.pm

@@ -110,6 +110,41 @@ with 'FixMyStreet::Roles::Abuser',
  'FixMyStreet::Roles::Moderation',
  'FixMyStreet::Roles::PhotoSet';
 
+=head2 FOREIGNBUILDARGS
+
+Make sure that when creating a new Comment object, certain
+other fields are set based upon the supplied data.
+
+=cut
+
+sub FOREIGNBUILDARGS {
+ my ($class, $opts) = @_;
+
+ if (my $user = $opts->{user}) {
+ my $name;
+ if ($user->is_superuser) {
+ $opts->{extra}->{is_superuser} = 1;
+ $name = _('an administrator');
+ } elsif (my $body = $user->from_body) {
+ $opts->{extra}->{is_body_user} = $body->id;
+ $name = $body->name;
+ $name = 'Island Roads' if $name eq 'Isle of Wight Council';
+ } else {
+ $name = $user->name;
+ }
+ $opts->{name} //= $name;
+ }
+
+ $opts->{anonymous} //= 0;
+ $opts->{mark_fixed} //= 0;
+ $opts->{state} //= 'confirmed'; # it's only public updates that need to be unconfirmed
+ if ($opts->{state} eq 'confirmed') {
+ $opts->{confirmed} //= \'current_timestamp';
+ }
+
+ return $opts;
+};
+
 =head2 get_cobrand_logged
 
 Get a cobrand object for the cobrand the update was made on.

perllib/FixMyStreet/Script/ArchiveOldEnquiries.pm

@@ -141,7 +141,6 @@ sub close_problems {
  my $problems = shift;
 
  my $extra = { auto_closed_by_script => 1 };
- $extra->{is_superuser} = 1 if !$opts->{user_name};
 
  my $cobrand;
  while (my $problem = $problems->next) {
@@ -152,16 +151,9 @@ sub close_problems {
  $cobrand->set_lang_and_domain($problem->lang, 1);
  }
 
- my $timestamp = \'current_timestamp';
  my $comment = $problem->add_to_comments( {
  text => $opts->{closure_text} || '',
- created => $timestamp,
- confirmed => $timestamp,
- user_id => $opts->{user},
- name => $opts->{user_name} || _('an administrator'),
- mark_fixed => 0,
- anonymous => 0,
- state => 'confirmed',
+ user => FixMyStreet::DB->resultset("User")->find($opts->{user}),
  problem_state => $opts->{closed_state},
  extra => $extra,
  } );

perllib/Open311/UpdatesBase.pm

@@ -154,13 +154,8 @@ sub process_update {
  $request, $p, $state, $old_state,
  $external_status_code, $old_external_status_code
  ),
- mark_fixed => 0,
- mark_open => 0,
- anonymous => 0,
- name => $self->system_user->name,
  confirmed => $request->{comment_time},
  created => $request->{comment_time},
- state => 'confirmed',
  }
  );
 

t/app/controller/admin/report_edit.t

@@ -329,7 +329,6 @@ foreach my $test (
  closed_updates => undef,
  },
  expect_comment => 1,
- user_body => $oxfordshire,
  changes => { state => 'investigating' },
  log_entries => [
  qw/edit state_change edit edit resend edit state_change edit state_change edit state_change edit state_change edit state_change edit edit edit edit edit/
@@ -351,7 +350,6 @@ foreach my $test (
  },
  expect_comment => 1,
  expected_text => '*Category changed from ‘Other’ to ‘Potholes’*',
- user_body => $oxfordshire,
  changes => { state => 'in progress', category => 'Potholes' },
  log_entries => [
  qw/edit state_change category_change edit state_change edit edit resend edit state_change edit state_change edit state_change edit state_change edit state_change edit edit edit edit edit/
@@ -364,11 +362,6 @@ foreach my $test (
  $report->comments->delete;
  $log_entries->reset;
 
- if ( $test->{user_body} ) {
- $superuser->from_body( $test->{user_body}->id );
- $superuser->update;
- }
-
  $mech->get_ok("/admin/report_edit/$report_id");
 
  @{$test->{fields}}{'external_id', 'external_body', 'external_team', 'category'} = (13, "", "", "Other");
@@ -440,21 +433,12 @@ foreach my $test (
  } else {
  is $comment->text, '', 'comment has no text';
  }
- if ( $test->{user_body} ) {
- ok $comment->get_extra_metadata('is_body_user'), 'body user metadata set';
- ok !$comment->get_extra_metadata('is_superuser'), 'superuser metadata not set';
- is $comment->name, $test->{user_body}->name, 'comment name is body name';
- } else {
- ok !$comment->get_extra_metadata('is_body_user'), 'body user metadata not set';
- ok $comment->get_extra_metadata('is_superuser'), 'superuser metadata set';
- is $comment->name, _('an administrator'), 'comment name is admin';
- }
+ ok !$comment->get_extra_metadata('is_body_user'), 'body user metadata not set';
+ ok $comment->get_extra_metadata('is_superuser'), 'superuser metadata set';
+ is $comment->name, _('an administrator'), 'comment name is admin';
  } else {
  is $report->comments->count, 0, 'report has no comments';
  }
-
- $superuser->from_body(undef);
- $superuser->update;
  };
 }
 

t/app/controller/admin/update_edit.t

@@ -81,7 +81,7 @@ for my $test (
  fields => {
  text => 'this is an update',
  state => 'confirmed',
- name => '',
+ name => 'Test User',
  anonymous => 1,
  username => $update->user->email,
  },
@@ -96,7 +96,7 @@ for my $test (
  fields => {
  text => 'this is a changed update',
  state => 'confirmed',
- name => '',
+ name => 'Test User',
  anonymous => 1,
  username => $update->user->email,
  },

t/app/controller/report_updates.t

@@ -1096,10 +1096,10 @@ subtest $test->{desc} => sub {
  unlike $update_meta->[1], qr/Commenter/, 'commenter name not included';
  like $update_meta->[0], qr/investigating/i, 'update meta includes state change';
 
- if ($test->{body} || $test->{bodyuser}) {
- like $update_meta->[1], qr/Westminster/, 'body user update uses body name';
- } elsif ($test->{superuser}) {
+ if ($test->{superuser}) {
  like $update_meta->[1], qr/an administrator/, 'superuser update says an administrator';
+ } elsif ($test->{body} || $test->{bodyuser}) {
+ like $update_meta->[1], qr/Westminster/, 'body user update uses body name';
  }
 
  ok $user->user_body_permissions->create({

t/app/model/comment.t

@@ -1,20 +1,27 @@
 use FixMyStreet::Test;
 
-my $comment_rs = FixMyStreet::DB->resultset('Comment');
+my $user = FixMyStreet::DB->resultset('User')->new({ name => 'Test User', is_superuser => 1 });
 
+my $comment_rs = FixMyStreet::DB->resultset('Comment');
 my $comment = $comment_rs->new(
  {
- user_id => 1,
+ user => $user,
  problem_id => 1,
  text => '',
- state => 'confirmed',
- anonymous => 0,
- mark_fixed => 0,
- cobrand => 'default',
- cobrand_data => '',
  }
 );
 
-is $comment->confirmed, undef, 'inflating null confirmed ok';
-is $comment->created, undef, 'inflating null confirmed ok';
+is $comment->created, undef, 'inflating null created ok';
+is $comment->mark_fixed, 0, 'mark fixed default set';
+is $comment->state, 'confirmed', 'state default is confirmed';
+is $comment->name, 'an administrator';
+
+$user->is_superuser(0);
+$comment = $comment_rs->new({
+ user => $user,
+ problem_id => 1,
+ text => '',
+});
+is $comment->name, 'Test User';
+
 done_testing();

t/open311.t

@@ -402,6 +402,7 @@ subtest 'Hounslow update description is correct for a different user' => sub {
 
  my $comment = make_comment('hounslow');
  $comment->user($user2);
+ $comment->name($user2->name);
  my $results;
  FixMyStreet::override_config {
  ALLOWED_COBRANDS => 'hounslow',