Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bexley] GGW bit of early refactoring #5322

Merged
merged 6 commits into from
Jan 14, 2025
Merged

Conversation

dracos
Copy link
Member

@dracos dracos commented Jan 10, 2025

This moves some bits around to supply some defaults to make it easier to set up a new instance, move stuff out of Echo or Brent specific code so it can be used by others, and similar. [skip changelog]
The branch did originally have a 'first bin different cost' commit in, but I think that needs more work, so starting with these hopefully more straightforward tweaks.

With these changes (and that first bin commit), the basic Bexley GGW subscription flow (apart from no "DD bit cheaper") is this commit: 906db76 - which is then only intro template, tests, a Bexley specific file to include SCP/Paye and specify the payment bits, and tiny other bits.

@dracos dracos requested a review from chrismytton January 10, 2025 11:40
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.43%. Comparing base (998a684) to head (fbbb05a).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
perllib/FixMyStreet/Roles/Cobrand/Waste.pm 93.47% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5322      +/-   ##
==========================================
- Coverage   82.43%   82.43%   -0.01%     
==========================================
  Files         415      416       +1     
  Lines       32904    32904              
  Branches     5277     5278       +1     
==========================================
- Hits        27126    27125       -1     
  Misses       4221     4221              
- Partials     1557     1558       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@chrismytton chrismytton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, big step in the right direction 🙂

Left a couple of questions about the templates, but nothing urgent.

Comment on lines +19 to +35
[% IF cobrand.moniker == 'brent' %]
Our 240 litre wheelie bins are green, and hold the equivalent of 5 or 6 bags of green garden waste.
[% ELSIF cobrand.moniker == 'bromley' %]
Each wheelie bin holds the equivalent of 5 or 6 bags of green garden waste,
and will be collected fortnightly during gardening season (March–November)
and monthly outside of gardening season (December–February).
[% ELSIF cobrand.moniker == 'kingston' %]
Each garden waste bin holds 240 litres of waste. We collect them every 2 weeks.
[% ELSIF cobrand.moniker == 'merton' %]
Our garden waste bins are brown. They have a 140L or 240L
capacity, which is about the same size as a small or standard wheelie bin.
[% ELSIF cobrand.moniker == 'sutton' %]
Each garden waste bin holds 240 litres of waste. We collect them every 2 weeks.
[% ELSE %]
Each garden waste bin holds 240 litres of waste. We collect them every 2 weeks.
[% END %]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this if statement be tidied up by calling a method in the cobrand modules? Or even pull from general.yml? Same for the banner image above (though possibly trickier with Merton's special case).

(And same question for the subscribe_existing_ template below)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the only downside of config is that it means you have to set that up in order for the cobrand to work okay, makes it a bit harder to have the equivalent of what's being shown, iyswim - and for something like this is very unlikely to change. I think I prefer output text in the templates rather than the code in general. As this commit was doing, it was in separate per-cobrand template files, which was cleaner (though every one had to repeat the little boilerplate bits) but also harder to see that each one was a little bit customised only for a line or two of text - I thought this way was overall to see that each cobrand has its own tiny custom output.

<span style="--primary-color: #8B5E3D;">[% image.replace('<svg', '<svg style="height:auto;max-width:100%;"') | safe %]</span>
[% ELSIF cobrand.moniker == 'sutton' %]
<img src="/i/waste-containers/bin-green-brown-lid.png" alt="">
[% ELSE %]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be some kind of fallback here, perhaps even an HTML comment explaining that there's no banner image?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted it work okay by default without showing something 'wrong', though I guess it could show any bin image and then it'd hopefully be spotted as part of testing, same as the "???" in the fallback text part, perhaps should just show a black bin as I doubt garden bins are ever that.

@dracos dracos force-pushed the bexley-ww-pre-work-refactor branch from 96aa17f to fbbb05a Compare January 14, 2025 09:44
@dracos dracos merged commit fbbb05a into master Jan 14, 2025
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants