Refactor email signup and sponsor signup journeys #665
Conversation
koetsier
force-pushed
the
new_refactor
branch
2 times, most recently
from
bf40f0a to
873fd15
Compare
koetsier
force-pushed
the
new_refactor
branch
5 times, most recently
from
27f976b to
520bfbd
Compare
koetsier
requested review from
A1Robson
and removed request for
jimnarey and
iram-shehzadi
|
Seems to be a makefile bug from previous removal of concourse. |
|
Also, the test isn't actually working on this, not sure why its reporting success in the checks above - have a look at the logs under the test action: https://github.com/alphagov/govwifi-user-signup-api/actions/runs/5530213407/job/14975180958?pr=665 |
This is the first step in refactoring the application. This allows us to centralise all the validations and the associated logging of the request in a single place, instead of having it spread out across several classes
This clarifies what the state of the allow list is and what is happening with the Notify services when running tests
find_or_create is a built-in method that does what #generate does. It is a more familiar method that better describes what is happening.
I rebased the code so this should be fixed |
I looked into this and, because of the docker-compose error the '-f docker-compose' command is run within the makefile - which is erroneous, it should run the docker-compose command. The makefile actually interprets the leading minus in the '-f' command as as a directive to not stop processing the makefile, even if an error is encountered. So the makefile keeps going until the last command which is 'rm -rf' which succeeds and returns error code 0. |
koetsier
force-pushed
the
new_refactor
branch
from
9cc4ac4 to
ebb3611
Compare
The email journey used a lot of injection which makes it hard to follow. The tests used excessive mocking which made them brittle and hard to read. The sponsor journey is temporarily disabled because this commit focusses on the email journey - the following commit re-enables and rewrites the sponsor journey. All code that calls Notify to send messages have been consolidated in one class making code easier to test and makes it easier to keep track of which messages are being sent. Parsing an incoming SMS message is now handled by an instance of the SnsMessage class, concentrating all parsing logic in one object that can easily be shared Logic to handle email addresses and phone numbers are now split from the 'ContactSanitiser' class into two separate classes 'PhoneNumber' and 'EmailAddress', seperating their concerns and making calling and testing them more straightforward. The class CheckIfAllowlistedEmail has been renamed to EmailAllowListChecker and has its dependency removed. Testing is made easier by stubbing the allowlist at the S3 level.
koetsier
force-pushed
the
new_refactor
branch
from
ebb3611 to
ce25df7
Compare
Simplify the code, remove superfluous injection code and mocking which makes understanding this code harder to understand Remove redundant classes EmailResponse and SmsResponse Remove dependencies from EmailSponseesExtractor and make the code more readable
This is not called anywhere
koetsier
force-pushed
the
new_refactor
branch
from
ce25df7 to
12547de
Compare
If the sponsored users already exist, the user is not re-created and the sponsor field is not set to the sponsor's email address This caused an issue where the confirmation email was sent to the wrong address
The active user survey fails because it hasn't loaded the yaml library.
koetsier
force-pushed
the
new_refactor
branch
from
b295635 to
9bd37f6
Compare
What
Refactor email signup and sponsor signup journeys
Why
The code before was hard to read and relied on excessive mocking to test the code, making tests brittle and unreliable.
The resulting code approximately 900 lines shorter