[Merton] Garden waste #4925
[Merton] Garden waste #4925
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## merton-ww-initial #4925 +/- ##
=====================================================
+ Coverage 82.64% 82.66% +0.01%
=====================================================
Files 397 400 +3
Lines 30799 30935 +136
Branches 4861 4878 +17
=====================================================
+ Hits 25453 25571 +118
- Misses 3896 3905 +9
- Partials 1450 1459 +9 ☔ View full report in Codecov by Sentry. |
dracos
force-pushed
the
merton-ww-garden
branch
from
702fec8
to
a995b27
Compare
dracos
force-pushed
the
merton-ww-garden
branch
from
a995b27
to
563ee77
Compare
| END; | ||
| %] | ||
|
|
||
| [% BLOCK answers %] | ||
| <div class="govuk-summary-list__row"> | ||
| <dt class="govuk-summary-list__key govuk-summary-list__key--sub">Green garden waste subscription</dt> | ||
| <dt class="govuk-summary-list__key govuk-summary-list__key--sub">Green garden waste collection</dt> |
There was a problem hiding this comment.
Why has this been changed from 'subscription'? And why the different 'step1' for 'Renew'?
There was a problem hiding this comment.
The word change is because that's what it said in the (now gone) renew template and I wanted to merge the two and that seemed like the better wording. The different step1 is because the subscription flow has a static "intro" page before the bit you'd like to edit, whereas the renew flow does not.
| my %sack_num = ( | ||
| sutton => 20, | ||
| kingston => 10, | ||
| merton => 25, |
There was a problem hiding this comment.
Is the Merton num used anywhere?
There was a problem hiding this comment.
It should be used a few lines down - in the display of the sack container line?
|
|
||
| return unless $c->stash->{is_staff}; | ||
|
|
||
| $c->stash->{staff_payments_allowed} = 'cnp'; |
There was a problem hiding this comment.
What does 'cnp' stand for?
There was a problem hiding this comment.
It's a flag for Roles::Cobrand::Adelante to use to say if staff or not, so could really be anything - there's also "paye" which then doesn't redirect elsewhere but shows a form to fill in.
| @@ -97,9 +97,6 @@ sub available_permissions { | |||
|
|
|||
| sub waste_auto_confirm_report { 1 } | |||
|
|
|||
| sub waste_staff_choose_payment_method { 1 } | |||
| sub waste_cheque_payments { shift->{c}->stash->{staff_payments_allowed} } | |||
|
|
|||
| use constant CONTAINER_REFUSE_140 => 1; | |||
There was a problem hiding this comment.
I wonder if any of these can be removed from here / refactored into SLWP.pm, as there seems to be a fair bit of overlap.
There was a problem hiding this comment.
Yeah, is true, I wondered if I could put the constants somewhere, but I'm not sure how I'd easily refer to them (I wouldn't want to put $self-> in front of them all, for example). Probably technically should have some form of class to handle the containers or something, but didn't seem worth it.
| $container = CONTAINER_GARDEN_BIN_140; | ||
| } elsif ($choice eq 'bin240') { | ||
| $container = CONTAINER_GARDEN_BIN; | ||
| } elsif ($choice) { |
There was a problem hiding this comment.
You could remove the checks for $choice eq 'bin240' and $choice because everything defaults to CONTAINER_GARDEN_BIN in the final else.
There was a problem hiding this comment.
I could remove the bin240, though decided to leave it in to be clear, but can't remove $choice entirely because if choice exists it needs to take precedence over $existing (which could be something else), if you see what I mean.
Starts off the same as the bulky branch for the rename/payment provider, then some tidying, then one more commit for the initial garden stuff. Has the changes asked for regarding everyone picking between 140L/240L/sacks.
For https://github.com/mysociety/societyworks/issues/4272 [skip changelog]