always allow problems to be removed from shortlist

If a user is trying to remove a problem from their shortlist we should
always allow it regardless of the state of the problem. Previously if a
problem wasn't displayed on the site then it could not be removed from
the shortlist which was an issue with council cobrands and reports that
had changed body.

Fixes mysociety/fixmystreetforcouncils#243

4 files changed, 165 insertions, 8 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b4a57c9c6..419dff1d2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@
         - Do not include blank updates in email alerts #1857
         - Redirect inspectors correctly on creation in two-tier.
         - Report status filter All option works for body users #1845
+        - Always allow reports to be removed from shortlist #1882
     - 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/My.pm b/perllib/FixMyStreet/App/Controller/My.pm
index 9647fae9a..1693766ba 100644
--- a/perllib/FixMyStreet/App/Controller/My.pm
+++ b/perllib/FixMyStreet/App/Controller/My.pm
@@ -189,17 +189,21 @@ sub planned_change : Path('planned/change') {
     $c->go('planned') if grep { /^shortlist-(up|down|\d+)$/ } keys %{$c->req->params};
 
     my $id = $c->get_param('id');
-    $c->forward( '/report/load_problem_or_display_error', [ $id ] );
-
     my $add = $c->get_param('shortlist-add');
     my $remove = $c->get_param('shortlist-remove');
-    $c->detach('/page_error_403_access_denied', [])
-        unless $add || $remove;
 
-    if ($add) {
+    # we can't lookup the report for removing via load_problem_or_display_error
+    # as then there is no way to remove a report that has been hidden or moved
+    # to another body by a category change from the shortlist.
+    if ($remove) {
+        my $report = $c->model('DB::Problem')->find({ id => $id })
+            or $c->detach( '/page_error_404_not_found', [ _('Unknown problem ID') ] );
+        $c->user->remove_from_planned_reports($report);
+    } elsif ($add) {
+        $c->forward( '/report/load_problem_or_display_error', [ $id ] );
         $c->user->add_to_planned_reports($c->stash->{problem});
-    } elsif ($remove) {
-        $c->user->remove_from_planned_reports($c->stash->{problem});
+    } else {
+        $c->detach('/page_error_403_access_denied', []);
     }
 
     if ($c->get_param('ajax')) {
diff --git a/t/app/controller/my_planned.t b/t/app/controller/my_planned.t
index 51ea0297e..67d59e148 100644
--- a/t/app/controller/my_planned.t
+++ b/t/app/controller/my_planned.t
@@ -5,6 +5,7 @@ $mech->get_ok('/my/planned');
 is $mech->uri->path, '/auth', "got sent to the sign in page";
 
 my $body = $mech->create_body_ok(2237, 'Oxfordshire County Council');
+my $body2 = $mech->create_body_ok(2421, 'Oxford City Council');
 my ($problem) = $mech->create_problems_for_body(1, $body->id, 'Test Title');
 
 $mech->get_ok($problem->url);
@@ -77,4 +78,155 @@ subtest "POSTing multiple problems to my/planned/change adds all to shortlist" =
     $mech->text_contains('Shortlisted');
 };
 
+subtest "re-ordering shortlist on non shortlist page redirect to shortlist" => sub {
+    $user->user_planned_reports->remove();
+    my ($problem1) = $mech->create_problems_for_body(1, $body->id, 'New Problem');
+
+    $mech->get_ok($problem1->url);
+    my ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+    $mech->post_ok( '/my/planned/change', {
+            id => $problem1->id,
+            'shortlist-up' => 1,
+            token => $csrf,
+        },
+    );
+
+    $mech->content_contains('Your shortlist');
+};
+
+subtest "shortlist with no action is forbidden" => sub {
+    $user->user_planned_reports->remove();
+    my ($problem1) = $mech->create_problems_for_body(1, $body->id, 'New Problem');
+
+    $mech->get_ok($problem1->url);
+    my ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+    my $result = $mech->post( '/my/planned/change', {
+            id => $problem1->id,
+            token => $csrf,
+        },
+    );
+
+    is $result->code, 403, '403 response if no action';
+};
+
+subtest "cannot remove non-existant problems from shortlist" => sub {
+    $user->user_planned_reports->remove();
+    my ($problem1) = $mech->create_problems_for_body(1, $body->id, 'New Problem');
+
+    $mech->get_ok($problem1->url);
+    my ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+    my $result = $mech->post( '/my/planned/change', {
+            id => 999,
+            'shortlist-remove' => 1,
+            token => $csrf,
+        },
+    );
+
+    is $result->code, 404, 'removing missing report returns 404';
+};
+
+subtest "can remove problems from shortlist" => sub {
+    $user->user_planned_reports->remove();
+    my ($problem1, $problem2) = $mech->create_problems_for_body(2, $body->id, 'New Problem');
+
+    $mech->get_ok($problem1->url);
+    my ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+    $mech->post_ok( '/my/planned/change_multiple', {
+            'ids[]' => [
+                $problem1->id,
+                $problem2->id,
+            ],
+            token => $csrf,
+        }
+    );
+
+    $mech->get_ok($problem1->url);
+    $mech->text_contains('Shortlisted');
+
+    ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+    $mech->post_ok( '/my/planned/change', {
+            id => $problem1->id,
+            'shortlist-remove' => 1,
+            token => $csrf,
+        },
+    'Removed problem from shortlist');
+
+    $mech->get_ok($problem1->url);
+    $mech->text_lacks('Shortlisted');
+    $mech->text_contains('Shortlist');
+
+    # check cases where problem has changed body due
+    # to e.g. change of category
+    $problem2->update({
+        bodies_str => $body2->id,
+        title => 'Other body problem',
+    });
+
+    $mech->get_ok('/my/planned');
+    $mech->text_contains('Other body problem');
+
+    ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+    $mech->post_ok( '/my/planned/change', {
+            id => $problem2->id,
+            'shortlist-remove' => 1,
+            token => $csrf,
+        },
+    'Removed problem for other body from shortlist');
+
+    $mech->get_ok('/my/planned');
+    $mech->text_lacks('Other body problem');
+};
+
+FixMyStreet::override_config {
+    ALLOWED_COBRANDS => [ 'oxfordshire' ],
+    BASE_URL => 'http://oxfordshire.fixmystreet.site',
+}, sub {
+    subtest "can remove problems not displayed in cobrand from shortlist" => sub {
+        $user->user_planned_reports->remove();
+        my ($problem1) = $mech->create_problems_for_body(2, $body->id, 'New Problem');
+
+        $mech->get_ok($problem1->url);
+        my ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+        $mech->post_ok( '/my/planned/change_multiple', {
+                'ids[]' => [
+                    $problem1->id,
+                ],
+                token => $csrf,
+            }
+        );
+
+        $mech->get_ok('/my/planned');
+        $mech->text_contains('New Problem');
+        $mech->content_contains('Remove from shortlist');
+
+        $problem1->update({
+            bodies_str => $body2->id,
+        });
+
+        $mech->get_ok('/my/planned');
+        $mech->text_contains('New Problem');
+        $mech->content_contains('Remove from shortlist');
+
+        ($csrf) = $mech->content =~ /meta content="([^"]*)" name="csrf-token"/;
+
+        $mech->post_ok( '/my/planned/change', {
+                id => $problem1->id,
+                'shortlist-remove' => 1,
+                token => $csrf,
+                ajax => 1,
+            },
+        'Removed problem not displayed in this cobrand');
+
+        $mech->get_ok('/my/planned');
+        $mech->text_lacks('New Problem', 'Problem no longer in shortlist');
+    };
+};
+
 done_testing();
diff --git a/templates/web/base/report/_item.html b/templates/web/base/report/_item.html
index 9449ca55d..576f2b30f 100644
--- a/templates/web/base/report/_item.html
+++ b/templates/web/base/report/_item.html
@@ -2,7 +2,7 @@
 [% PROCESS 'admin/report_blocks.html' ~%]
 [% END ~%]
 
-[% IF c.user.has_permission_to('planned_reports', problem.bodies_str_ids) ~%]
+[% IF c.user.has_permission_to('planned_reports', problem.bodies_str_ids) OR c.user.is_planned_report(problem) ~%]
     [% item_extra_class = "item-list__item--indented" ~%]
     [% item_action = BLOCK ~%]
       <input type="submit" value="1"