Skip to content

Commit

Permalink
Capture group from hoisted categories.
Browse files Browse the repository at this point in the history 
If a category is in a group on its own, the reporting form hoists it up
to the top level, to save a pointless extra step. In doing so, it was no
longer getting the associated group to store for submission. Rewrite the
input so it is clearer as to what we are receiving.
  • Loading branch information
@dracos
dracos committed Sep 18, 2024
1 parent 65b55d9 commit 11ebf1b
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 28 deletions.
1 change: 1 addition & 0 deletions perllib/FixMyStreet/App/Controller/Report.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@ sub stash_category_groups : Private {
(my $id = $_) =~ s/[^a-zA-Z]+//g;
if (@{$category_groups{$_}} == 1) {
my $contact = $category_groups{$_}[0];
$contact->set_extra_metadata(hoisted => $_);
push @list, [ $contact->category_display, $contact ];
} else {
my $cats = $category_groups{$_};
Expand Down
21 changes: 12 additions & 9 deletions perllib/FixMyStreet/App/Controller/Report/New.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1823,19 +1823,22 @@ sub generate_map : Private {
sub check_for_category : Private {
my ( $self, $c, $opts ) = @_;

my $category;
if (!$opts->{with_group}) {
$category = $c->get_param('category') || '';
} else {
# Group is either an actual group, or a category that wasn't in a group
my $group = $c->get_param('category') || $c->get_param('filter_group') || '';
if (any { $_->{name} && $group eq $_->{name} } @{$c->stash->{category_groups}}) {
my $category = $c->get_param('category') || '';
if ($opts->{with_group}) {
if (my ($group) = $category =~ /^G\|(.*)/) {
# A top-level group
$c->stash->{group} = $c->stash->{filter_group} = $group;
(my $group_id = $group) =~ s/[^a-zA-Z]+//g;
my $cat_param = "category.$group_id";
$category = $c->get_param($cat_param);
} else {
$category = $group;
} elsif ($category =~ /^H\|(.*?)\|(.*)/) {
# A hoisted to top-level category
($group, $category) = ($1, $2);
$c->stash->{group} = $c->stash->{filter_group} = $group;

Check warning on line 1837 in perllib/FixMyStreet/App/Controller/Report/New.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/App/Controller/Report/New.pm#L1836-L1837 

Added lines #L1836 - L1837 were not covered by tests
} elsif (!$category && ($group = $c->get_param('filter_group'))) {
if (any { $_->{name} && $group eq $_->{name} } @{$c->stash->{category_groups}}) {
$c->stash->{group} = $c->stash->{filter_group} = $group;

Check warning on line 1840 in perllib/FixMyStreet/App/Controller/Report/New.pm

View check run for this annotation

Codecov / codecov/patch

perllib/FixMyStreet/App/Controller/Report/New.pm#L1840 

Added line #L1840 was not covered by tests
}
}
}
$category ||= $c->stash->{report}->category || '';
Expand Down
6 changes: 3 additions & 3 deletions t/app/controller/auth_social.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ for my $state ( 'refused', 'no email', 'existing UID', 'okay' ) {
$mech->submit_form_ok( { with_fields => { pc => $test->{pc} || 'SW1A1AA' } }, "submit location" );
$mech->follow_link_ok( { text_regex => qr/skip this step/i, }, "follow 'skip this step' link" );
$mech->submit_form(with_fields => {
category => 'Bins',
category => 'G|Bins',
'category.Bins' => 'Damaged bin',
title => 'Test title',
detail => 'Test detail',
Expand Down Expand Up @@ -173,7 +173,7 @@ for my $state ( 'refused', 'no email', 'existing UID', 'okay' ) {
if ($page eq 'report') {
$mech->content_contains('/report/new');
$mech->content_contains('Salt bin');
$mech->content_contains('name="category" value="Bins" data-subcategory="Bins" checked');
$mech->content_like(qr{value="G|Bins"\s+data-subcategory="Bins" checked});
$mech->content_contains('name="category.Bins" data-category_display="Damaged bin" value=\'Damaged bin\' checked');
} elsif ($page eq 'update') {
$mech->content_contains('/report/update');
Expand Down Expand Up @@ -319,7 +319,7 @@ for my $tw_state ( 'refused', 'existing UID', 'no email' ) {
$mech->submit_form_ok( { with_fields => { pc => 'SW1A1AA' } }, "submit location" );
$mech->follow_link_ok( { text_regex => qr/skip this step/i, }, "follow 'skip this step' link" );
$mech->submit_form(with_fields => {
category => 'Bins',
category => 'G|Bins',
'category.Bins' =>'Damaged bin',
title => 'Test title',
detail => 'Test detail',
Expand Down
20 changes: 15 additions & 5 deletions t/app/controller/report_new.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -680,9 +680,9 @@ subtest "category groups" => sub {
my $div = '<div[^>]*>\s*';
my $div_end = '</div>\s*';
my $pavements_label = '<label[^>]* for="category_Pavements">Pavements</label>\s*' . $div_end;
my $pavements_input = '<input[^>]* value="Pavements" data-subcategory="Pavements">\s*';
my $pavements_input_checked = '<input[^>]* value="Pavements" data-subcategory="Pavements" checked>\s*';
my $roads = $div . '<input[^>]* value="Roads" data-subcategory="Roads">\s*<label[^>]* for="category_Roads">Roads</label>\s*' . $div_end;
my $pavements_input = '<input[^>]* value="G|Pavements"\s+data-subcategory="Pavements">\s*';
my $pavements_input_checked = '<input[^>]* value="G|Pavements"\s+data-subcategory="Pavements" checked>\s*';
my $roads = $div . '<input[^>]* value="G|Roads"\s+data-subcategory="Roads">\s*<label[^>]* for="category_Roads">Roads</label>\s*' . $div_end;
my $trees_label = '<label [^>]* for="category_\d+">Trees</label>\s*' . $div_end;
my $trees_input = $div . '<input[^>]* value=\'Trees\'>\s*';
my $trees_input_checked = $div . '<input[^>]* value=\'Trees\' checked>\s*';
Expand All @@ -708,18 +708,28 @@ subtest "category groups" => sub {
$mech->content_like(qr{$fieldset_pavements$options});
$mech->content_like(qr{$fieldset_roads$options});
# Server submission of pavement subcategory
$mech->get_ok("/report/new?lat=$saved_lat&lon=$saved_lon&category=Pavements&category.Pavements=Potholes");
$mech->get_ok("/report/new?lat=$saved_lat&lon=$saved_lon&category=G|Pavements&category.Pavements=Potholes");
$mech->content_like(qr{$pavements_input_checked$pavements_label$roads$trees_input$trees_label</fieldset>});
$mech->content_like(qr{$fieldset_pavements$optionsS});
$mech->content_like(qr{$fieldset_roads$options});

$contact9->update( { extra => { group => 'Lights' } } );
$mech->get_ok("/report/new?lat=$saved_lat&lon=$saved_lon");
$streetlighting = $div . '<input[^>]*value=\'Street lighting\'>\s*<label[^>]* for="category_\d+">Street lighting</label>\s*' . $div_end;
$streetlighting = $div . '<input[^>]*value=\'H|Lights\|Street lighting\'>\s*<label[^>]* for="category_\d+">Street lighting</label>\s*' . $div_end;
$potholes_input = $div . '<input[^>]* value=\'H|Pavements\|Potholes\'>\s*';
$potholes_label = '<label[^>]* for="category_\d+">Potholes</label>\s*' . $div_end;
$mech->content_like(qr{$potholes_input$potholes_label$roads$streetlighting$trees_input$trees_label</fieldset>});
$mech->content_unlike(qr{$fieldset_pavements});
$mech->content_like(qr{$fieldset_roads$options});

$mech->submit_form_ok({ with_fields => {
category => 'H|Lights|Street lighting',
title => 'Test Report',
detail => 'Test report details',
username_register => '[email protected]',
name => 'Jo Bloggs',
} });
$mech->content_contains('Now check your email');
};
};

Expand Down
8 changes: 4 additions & 4 deletions t/cobrand/brent.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ FixMyStreet::override_config {
with_fields => {
title => "Test Report",
detail => 'Test report details.',
category => 'Parks and open spaces',
category => 'G|Parks and open spaces',
'category.Parksandopenspaces' => 'Overgrown grass',
}
}, "submit details");
Expand All @@ -951,7 +951,7 @@ FixMyStreet::override_config {
with_fields => {
title => "Test Report",
detail => 'Test report details.',
category => 'Parks and open spaces',
category => 'G|Parks and open spaces',
'category.Parksandopenspaces' => 'Overgrown grass',
}
}, "submit details");
Expand All @@ -970,7 +970,7 @@ FixMyStreet::override_config {
with_fields => {
title => "Test Report",
detail => 'Test report details.',
category => 'Parks and open spaces',
category => 'G|Parks and open spaces',
'category.Parksandopenspaces' => 'Overgrown grass',
}
}, "submit details");
Expand All @@ -994,7 +994,7 @@ FixMyStreet::override_config {
with_fields => {
title => "Test Report",
detail => 'Test report details.',
category => 'Parks and open spaces',
category => 'G|Parks and open spaces',
'category.Parksandopenspaces' => 'Ponds',
}
}, "submit details");
Expand Down
6 changes: 3 additions & 3 deletions t/cobrand/northumberland.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ FixMyStreet::override_config {
$mech->content_contains('Main Carriageway');
$mech->content_contains('Potholes');
$mech->content_contains("Trees'>");
$mech->content_contains('value=\'Flytipping\' data-nh="1"');
$mech->content_contains('value=\'H|Staff Only - Out Of Hours|Flytipping\' data-nh="1"');

# A-road where NH responsible for litter, council categories will also be present
mock_road("A5103", 1);
Expand All @@ -97,7 +97,7 @@ FixMyStreet::override_config {
$mech->content_contains('Main Carriageway');
$mech->content_contains('Potholes');
$mech->content_contains('Trees\'>');
$mech->content_contains('value=\'Flytipping\' data-nh="1"');
$mech->content_contains('value=\'H|Staff Only - Out Of Hours|Flytipping\' data-nh="1"');

# A-road where NH not responsible for litter, no NH litter categories
mock_road("A34", 0);
Expand All @@ -107,7 +107,7 @@ FixMyStreet::override_config {
$mech->content_lacks('Main Carriageway');
$mech->content_contains('Potholes');
$mech->content_contains('Trees\'>');
$mech->content_contains('value=\'Flytipping\' data-nh="1"');
$mech->content_contains('value=\'H|Staff Only - Out Of Hours|Flytipping\' data-nh="1"');
};
};

Expand Down
17 changes: 13 additions & 4 deletions templates/web/base/report/new/category.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,26 @@
[%~ FOREACH group_or_cat IN category_groups ~%]
<div class="govuk-radios__item">
[% IF group_or_cat.name %]
<input class="govuk-radios__input" required id="category_[% group_or_cat.id %]" type="radio" name="category" value="[% group_or_cat.name %]" data-subcategory="[% group_or_cat.id %]"[% ' checked' IF filter_group == group_or_cat.name %]>
<input class="govuk-radios__input" required type="radio" name="category"
id="category_[% group_or_cat.id %]"
value="G|[% group_or_cat.name %]"
data-subcategory="[% group_or_cat.id %]"
[%~ ' checked' IF filter_group == group_or_cat.name %]>
<label class="govuk-label govuk-radios__label" for="category_[% group_or_cat.id %]">[% group_or_cat.name %]</label>
[% group_hint = group_or_cat.categories.first.get_extra_metadata('group_hint') %]
[%~ IF group_hint %]
<div class="govuk-hint govuk-radios__hint">
[% group_hint | safe %]
</div>
[% END ~%]
[% ELSE # A category not in a group %]
[% cat_lc = group_or_cat.category | lower =%]
<input class="govuk-radios__input" required id="category_[% group_or_cat.id %]" type="radio" name="category" data-category_display="[% group_or_cat.category_display %]" value='[% group_or_cat.category %]'
[% ELSE # A category not in a group, or a hoisted category %]
[% cat_lc = group_or_cat.category | lower;
hoisted = group_or_cat.get_extra_metadata('hoisted');
=%]
<input class="govuk-radios__input" required type="radio" name="category"
id="category_[% group_or_cat.id %]"
data-category_display="[% group_or_cat.category_display %]"
value='[% "H|" _ hoisted _ "|" IF hoisted %][% group_or_cat.category %]'
[%~ ' checked' IF ( report.category == group_or_cat.category || category_lc == cat_lc ) AND NOT filter_group ~%]
[%~ ' data-nh="1"' IF group_or_cat.get_extra_metadata('nh_council_cleaning') ~%]
>
Expand Down

0 comments on commit 11ebf1b

Please sign in to comment.