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

Refactor review licence POST pattern #928

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

Cruikshanks
Copy link
Member

https://eaflood.atlassian.net/browse/WATER-4447

We recently added Hapi yar session manager to the project. This was specifically to support flash notification banners (ones that appear on the first GET request but then disappear).

The Review licence screen has a number of these already. Ones for when you toggle the progress and others for toggling the status. These have been achieved by breaking the post-redirect-get pattern and responding directly to the POST request.

As a first step to switching to using yar we need to refactor ReviewLicenceService into two; ReviewLicenceService and SubmitReviewLicenceService. This means it will also follow the pattern we've been using in other areas of the project.

This change is the first step. We'll switch to yar in the next one.

@Cruikshanks Cruikshanks added the housekeeping Refactoring, tidying up or other work which supports the project label Apr 23, 2024
@Cruikshanks Cruikshanks self-assigned this Apr 23, 2024
@Cruikshanks Cruikshanks force-pushed the refactor-review-licence-post-pattern branch from 6ffe252 to 11aeedc Compare April 23, 2024 17:29
@Cruikshanks Cruikshanks marked this pull request as ready for review April 23, 2024 20:50
Jozzey
Jozzey previously approved these changes Apr 24, 2024
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

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

https://eaflood.atlassian.net/browse/WATER-4447

We recently added [Hapi yar session manager to the project](#926). This was specifically to support flash notification banners (ones that appear on first GET request but then disappear).

The Review licence screen has a number of these already. Ones for when you toggle the progress and and others for toggling the status. These have been achieved by breaking the [post-redirect-get pattern](https://www.geeksforgeeks.org/post-redirect-get-prg-design-pattern/) and responding directly from the POST request.

As a first step to switching to using **yar** we need to refactor `ReviewLicenceService` into two; `ReviewLicenceService` and `SubmitReviewLicenceService`. This means it will also follow the pattern we've been using in other areas of the project.

This change is that first step. We'll switch to **yar** in the next one.
We always had the 2 routes obviously, but now they call separate handlers rather than the same one. That way they can focus on doing one thing and calling their own service.
We've split off the functionality that gets used when a POST request is made into `SubmitReviewLicenceService`.

So, now this service only needs to handle when a GET request is made.

On the tests, it was previously understandable that `ReviewLicencePresenter` was stubbed. There is _a lot_ of test data needed to make it work otherwise. But stubbing it is the exception. In all our other tests we've not needed to. We think this is a 'smell' that both the fetch service and the presenter are doing too much. Having to cater for this in the tests is a handy way of highlighting this.

It is beyond the scope of this PR to revisit the service and the presenter. So, for now we limit the change to drop the stubbing and take the data hit. But we'll endeavour to revisit these modules in the future.
The keen eyed will see this is an exact copy of `ReviewLicenceService` before we refactored it!

The scope of this PR is to split the GET and POST request handling to get us in a position we can re-write how the POST is handled to use [yar](#926) and fix it to support post-redirect-get again.

So, we not only just copy the code across, we also copy the test as it was.

We promise we'll be updating it in a subsequent change! ;-)
@Cruikshanks Cruikshanks force-pushed the refactor-review-licence-post-pattern branch from ee3b1ef to dd2b57c Compare April 24, 2024 16:19
@Cruikshanks Cruikshanks merged commit 1001a28 into main Apr 24, 2024
6 checks passed
@Cruikshanks Cruikshanks deleted the refactor-review-licence-post-pattern branch April 24, 2024 17:01
Cruikshanks added a commit that referenced this pull request Apr 24, 2024
https://eaflood.atlassian.net/browse/WATER-4447

We recently added [Hapi yar session manager to the project](#926). This was specifically to support flash notification banners (ones that appear on the first GET request but then disappear).

The Review licence screen has a number of these already. Ones for when you toggle the progress and others for toggling the status. These have been achieved by breaking the [post-redirect-get pattern](https://www.geeksforgeeks.org/post-redirect-get-prg-design-pattern/) and responding directly to the POST request.

As a first step we [refactored `ReviewLicenceService`](#928) into two; `ReviewLicenceService` and `SubmitReviewLicenceService`.

We can now complete the process. This change updates `SubmitReviewLicenceService` to use **yar** and have the associated controller redirect rather than respond.
Cruikshanks added a commit that referenced this pull request Apr 25, 2024
https://eaflood.atlassian.net/browse/WATER-4447

We recently added [Hapi yar session manager to the project](#926). This was specifically to support flash notification banners (ones that appear on the first GET request but then disappear).

The Review licence screen has a number of these already. Ones for when you toggle the progress and others for toggling the status. These have been achieved by breaking the [post-redirect-get pattern](https://www.geeksforgeeks.org/post-redirect-get-prg-design-pattern/) and responding directly to the POST request.

As a first step we [refactored `ReviewLicenceService`](#928) into two; `ReviewLicenceService` and `SubmitReviewLicenceService`.

We can now complete the process. This change updates `SubmitReviewLicenceService` to use **yar** and have the associated controller redirect rather than respond.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Refactoring, tidying up or other work which supports the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants