correctly handle category changes in inspector form

This resolves two issues when updating the report category in the
staff users inspect form:
* report category is only updated if the rest of the form is valid
* only one update on the report is left

Previously changing the category would create an update in addition to
any public update created, and the category was always updated even when
validation errors occurred elsewhere in the form.

5 files changed, 150 insertions, 14 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index baf561ebc..a36271bf7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@
     - Bugfixes
         - Shortlist menu item always remains a link #1855
         - Fix encoded entities in RSS output. #1859
+        - Only save category changes if staff user update valid #1857
+        - Only create one update when staff user updating category #1857
     - Admin improvements:
       - Character length limit can be placed on report detailed information #1848
       - Inspector panel shows nearest address if available #1850
diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index 0caa25710..b854f16f5 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -950,7 +950,7 @@ Handles changing a problem's category and the complexity that comes with it.
 =cut
 
 sub report_edit_category : Private {
-    my ($self, $c, $problem) = @_;
+    my ($self, $c, $problem, $no_comment) = @_;
 
     if ((my $category = $c->get_param('category')) ne $problem->category) {
         my $category_old = $problem->category;
@@ -978,16 +978,21 @@ sub report_edit_category : Private {
         }
 
         $problem->bodies_str(join( ',', @new_body_ids ));
-        $problem->add_to_comments({
-            text => '*' . sprintf(_('Category changed from ‘%s’ to ‘%s’'), $category_old, $category) . '*',
-            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,
-        });
+        my $update_text = '*' . sprintf(_('Category changed from ‘%s’ to ‘%s’'), $category_old, $category) . '*';
+        if ($no_comment) {
+            $c->stash->{update_text} = $update_text;
+        } 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,
+            });
+        }
     }
 }
 
diff --git a/perllib/FixMyStreet/App/Controller/Report.pm b/perllib/FixMyStreet/App/Controller/Report.pm
index ba9fa11b9..c72c75d3a 100644
--- a/perllib/FixMyStreet/App/Controller/Report.pm
+++ b/perllib/FixMyStreet/App/Controller/Report.pm
@@ -424,7 +424,11 @@ sub inspect : Private {
         }
 
         if ($permissions->{report_inspect} || $permissions->{report_edit_category}) {
-            $c->forward( '/admin/report_edit_category', [ $problem ] );
+            $c->forward( '/admin/report_edit_category', [ $problem, 1 ] );
+
+            if ($c->stash->{update_text}) {
+                $update_text .= "\n\n" . $c->stash->{update_text};
+            }
 
             # The new category might require extra metadata (e.g. pothole size), so
             # we need to update the problem with the new values.
diff --git a/t/app/controller/report_inspect.t b/t/app/controller/report_inspect.t
index 45bb4f8a7..f74c94c34 100644
--- a/t/app/controller/report_inspect.t
+++ b/t/app/controller/report_inspect.t
@@ -11,10 +11,18 @@ my $rp = FixMyStreet::DB->resultset("ResponsePriority")->create({
     body => $oxon,
     name => 'High Priority',
 });
+my $rp2 = FixMyStreet::DB->resultset("ResponsePriority")->create({
+    body => $oxon,
+    name => 'Low Priority',
+});
 FixMyStreet::DB->resultset("ContactResponsePriority")->create({
     contact => $contact,
     response_priority => $rp,
 });
+FixMyStreet::DB->resultset("ContactResponsePriority")->create({
+    contact => $contact3,
+    response_priority => $rp2,
+});
 my $wodc = $mech->create_body_ok(2420, 'West Oxfordshire District Council');
 $mech->create_contact_ok( body_id => $wodc->id, category => 'Horses', email => 'horses@example.net' );
 
@@ -254,7 +262,7 @@ FixMyStreet::override_config {
             $mech->submit_form_ok({
                 button => 'save',
                 with_fields => {
-                    $test->{priority} ? (priority => 1) : (),
+                    $test->{priority} ? (priority => $rp->id) : (),
                     $test->{category} ? (category => 'Cows') : (),
                     $test->{detailed} ? (detailed_information => 'Highland ones') : (),
                 }
@@ -262,6 +270,68 @@ FixMyStreet::override_config {
         };
     }
 
+    subtest "check priority not set for category with no priorities" => sub {
+        $report->discard_changes;
+        $report->update({ category => 'Cows', response_priority_id => undef });
+        $report->discard_changes;
+        is $report->response_priority, undef, 'response priority not set';
+        $user->user_body_permissions->delete;
+        $user->user_body_permissions->create({ body => $oxon, permission_type => 'report_edit_category' });
+        $user->user_body_permissions->create({ body => $oxon, permission_type => 'report_edit_priority' });
+        $mech->get_ok("/report/$report_id");
+        $mech->submit_form_ok({
+            button => 'save',
+            with_fields => {
+                priority => $rp->id,
+                category => 'Sheep',
+            }
+        });
+
+        $report->discard_changes;
+        is $report->response_priority, undef, 'response priority not set';
+    };
+
+    subtest "check can set priority for category when changing from category with no priorities" => sub {
+        $report->discard_changes;
+        $report->update({ category => 'Sheep', response_priority_id => undef });
+        $report->discard_changes;
+        is $report->response_priority, undef, 'response priority not set';
+        $user->user_body_permissions->delete;
+        $user->user_body_permissions->create({ body => $oxon, permission_type => 'report_edit_category' });
+        $user->user_body_permissions->create({ body => $oxon, permission_type => 'report_edit_priority' });
+        $mech->get_ok("/report/$report_id");
+        $mech->submit_form_ok({
+            button => 'save',
+            with_fields => {
+                priority => $rp->id,
+                category => 'Cows',
+            }
+        });
+
+        $report->discard_changes;
+        is $report->response_priority->id, $rp->id, 'response priority set';
+    };
+
+    subtest "check can't set priority that isn't for a category" => sub {
+        $report->discard_changes;
+        $report->update({ category => 'Cows', response_priority_id => $rp->id });
+        $report->discard_changes;
+        is $report->response_priority->id, $rp->id, 'response priority set';
+        $user->user_body_permissions->delete;
+        $user->user_body_permissions->create({ body => $oxon, permission_type => 'report_edit_category' });
+        $user->user_body_permissions->create({ body => $oxon, permission_type => 'report_edit_priority' });
+        $mech->get_ok("/report/$report_id");
+        $mech->submit_form_ok({
+            button => 'save',
+            with_fields => {
+                priority => $rp2->id,
+            }
+        });
+
+        $report->discard_changes;
+        is $report->response_priority, undef, 'response priority set';
+    };
+
     subtest "check nearest address display" => sub {
         $mech->get_ok("/report/$report_id");
         $mech->content_lacks('Nearest calculated address', 'No address displayed');
@@ -408,6 +478,61 @@ FixMyStreet::override_config {
 };
 
 FixMyStreet::override_config {
+    MAPIT_URL => 'http://mapit.uk/',
+    ALLOWED_COBRANDS => 'fixmystreet',
+}, sub {
+    subtest "test category not updated if fail to include public update" => sub {
+        $mech->get_ok("/report/$report_id");
+        $mech->submit_form(button => 'save', with_fields => { category => 'Badgers' });
+
+        $report->discard_changes;
+        is $report->category, "Cows", "Report in correct category";
+        $mech->content_contains('Badgers" selected', 'Changed category still selected');
+    };
+
+    subtest "test invalid form maintains Category and priority" => sub {
+        $mech->get_ok("/report/$report_id");
+        my $expected_fields = {
+          state => 'action scheduled',
+          category => 'Cows',
+          public_update => '',
+          priority => $rp->id,
+          include_update => '1',
+          detailed_information => 'XXX172XXXxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
+          defect_type => '',
+          traffic_information => ''
+        };
+        my $values = $mech->visible_form_values('report_inspect_form');
+        is_deeply $values, $expected_fields, 'correct form fields present';
+
+        $mech->submit_form(button => 'save', with_fields => { category => 'Badgers', priority => $rp2->id });
+
+        $expected_fields->{category} = 'Badgers';
+        $expected_fields->{priority} = $rp2->id;
+
+        my $new_values = $mech->visible_form_values('report_inspect_form');
+        is_deeply $new_values, $expected_fields, 'correct form fields present';
+    };
+
+    subtest "test changing category and leaving an update only creates one comment" => sub {
+        $report->comments->delete;
+        $mech->get_ok("/report/$report_id");
+        $mech->submit_form(
+            button => 'save',
+            with_fields => {
+                category => 'Badgers',
+                include_update => 1,
+                public_update => 'This is a public update',
+        });
+
+        $report->discard_changes;
+        is $report->category, "Badgers", "Report in correct category";
+        is $report->comments->count, 1, "Only leaves one update";
+        like $report->comments->first->text, qr/Category changed.*Badgers/, 'update text included category change';
+    };
+};
+
+FixMyStreet::override_config {
     ALLOWED_COBRANDS => [ 'oxfordshire', 'fixmystreet' ],
     BASE_URL => 'http://fixmystreet.site',
 }, sub {
diff --git a/templates/web/base/report/_inspect.html b/templates/web/base/report/_inspect.html
index 973db649e..e4e3ac9e6 100644
--- a/templates/web/base/report/_inspect.html
+++ b/templates/web/base/report/_inspect.html
@@ -6,7 +6,7 @@
 
     [% INCLUDE 'errors.html' %]
 
-    <form id="report_inspect_form" method="post" action="[% c.uri_for( '/report', problem.id ) %]" class="validate">
+    <form name="report_inspect_form" id="report_inspect_form" method="post" action="[% c.uri_for( '/report', problem.id ) %]" class="validate">
       <input type="hidden" name="js" value="">
 
       <div class="inspect-section">