Feature: Line manager can deny an annual leave request #25

johnny-m-young · 2023-09-07T13:32:12Z

What

Adds the ability for line managers to deny annual leave requests, including:

Feature test following the full user journey
Error handling on the annual_leave_requests#deny page when a line manager does not give a reason for denying the annual leave request.
Sending an email when a user's annual leave request has been denied
NOTE: The email address and Slack channel on the annual_leave_request#confirm_approval page are non-functional at the moment so are not links.

Why

This is a key feature in the application. Users need to have their annual leave requests responded to by their line managers.

Screenshots

Deny page

Design specification

Deny page with errors

Denial confirmation page

Design specification

Adds a test for the feature, following the full user journey.

Creates an endpoint for line managers to deny annual leave requests.

This is the endpoint users hit when they have successfully denied an annual leave request.

Creates a column in the annual_leave_requests table for denial_reason and permits this parameter in the AnnualLeaveRequestsController

Updates the page to be rendered based on the attempted status update (i.e. if a denial failed, render the deny page)

Implements code to send an email to a user when their annual leave request has been denied

This refactors the emails_helper module so that it uses the same method for sending emails about status updates, whether it is denied or approved. This is with the aim of making the workflow more open to extension, because at the moment it is not.

ryanb-gds

There are several instances here of conditional behaviour based on the leave request status, which is a good sign it might be worth refactoring. In general you want to check a condition once and then propagate that decision across the application where necessary

ryanb-gds · 2023-09-12T10:52:16Z

app/controllers/annual_leave_requests_controller.rb

+      confirm_annual_leave_request_approval_path
+    when "denied"
+      confirm_annual_leave_request_denial_path
+    end


I think the fact that you're checking the status both here and on lines 43-44 is a sign there's a refactoring available here. There's a couple of things you could do. Either you could separate the approval and denial routes, removing the conditionals altogether (this would be my choice - in my experience more routes with less conditionals are much more maintainable than less routes with more conditionals) or you could set up some kind of status configuration that knows what paths/views to render based on the status parameter.

I agree 100%, I created a refactor here but thought it would be better as a separate PR as this one is already quite big. Let me know what you think of the refactor.

Yeah that refactor looks like it's definitely heading in a good direction, nice one

ryanb-gds · 2023-09-12T10:52:43Z

app/helpers/emails_helper.rb

+    when "approved"
+      client.send_email(approval_email_hash(annual_leave_request))
+    when "denied"
+      client.send_email(denial_email_hash(annual_leave_request))


Ah, another duplicated conditional 🤔

ryanb-gds · 2023-09-12T10:55:08Z

app/helpers/emails_helper.rb

+        date_to: annual_leave_request.date_to.to_fs(:rfc822),
+        denial_reason: annual_leave_request.denial_reason,
+      },
+    }


This is sort of why I was suggesting those serializer objects in a previous PR review - once you've got a number of different email hashes to think about you probably won't want them all in one helper... but for now it's probably fine.

ryanb-gds · 2023-09-12T10:57:09Z

app/models/annual_leave_request.rb

@@ -6,6 +6,7 @@ class AnnualLeaveRequest < ApplicationRecord
  validates :date_from, :date_to, comparison: { greater_than: Time.zone.today, message: "must be in the future" }
  validate :user_has_enough_annual_leave_remaining
  validates :confirm_approval, acceptance: { accept: "confirmed" }
+  validates :denial_reason, presence: true, allow_nil: true


Curious about the allow_nil here, I'm guessing there's a good reason why it's there but I don't know what it is

This allows me to create AnnualLeaveRequests with no denial reason (default is nil on creation), whilst simultaneously checking a Line Manager gives a reason on denial. Is there a better way to do this?

Ah that makes sense. That suggests you've got a problem with your database table design to me, because having to have a denial reason for requests that aren't denied is a sign the modelling isn't perfect. I've done things like move the state updates into a different table or even have a separate table for records for each state in other systems before, but it's hard to say exactly what the best choice would be here.

Modelling state machines in databases is tricky though and you might want to leave it as is for now until it starts to cause more problems.

Yea that makes sense! I'm going to merge this request and make a trello ticket to revisit this topic

ryanb-gds · 2023-09-12T11:07:59Z

spec/controllers/annual_leave_requests_controller_spec.rb

+          denial_reason: "some valid reason",
+        },
+      }
+      leave_request.reload


I feel a bit wary about sharing the leave_request object between these four tests, but I think in case its safe. Always a little bit prone to confusing mishaps though

johnny-m-young force-pushed the feature-deny-annual-leave-request branch 2 times, most recently from 1977961 to 73ea660 Compare September 7, 2023 13:36

Jonathan Young added 3 commits September 8, 2023 15:24

Add error handling

d999679

Write feature test

6fa003d

Adds a test for the feature, following the full user journey.

Add annual_leave_request#deny endpoint

c44f7ae

Creates an endpoint for line managers to deny annual leave requests.

johnny-m-young force-pushed the feature-deny-annual-leave-request branch 3 times, most recently from 327d388 to 8f5e6bb Compare September 8, 2023 14:59

johnny-m-young changed the title ~~Feature deny annual leave request~~ Feature: Line manager can deny annual leave requests Sep 8, 2023

johnny-m-young changed the title ~~Feature: Line manager can deny annual leave requests~~ Feature: Line manager can deny an annual leave request Sep 8, 2023

johnny-m-young force-pushed the feature-deny-annual-leave-request branch from 8f5e6bb to 7b97567 Compare September 8, 2023 15:10

johnny-m-young marked this pull request as ready for review September 8, 2023 15:14

Jonathan Young added 6 commits September 11, 2023 11:00

Add annual_leave_request#confirm_denial

8cbd1a8

This is the endpoint users hit when they have successfully denied an annual leave request.

Add denial_reason to AnnualLeaveRequest

86be25b

Creates a column in the annual_leave_requests table for denial_reason and permits this parameter in the AnnualLeaveRequestsController

Add error handling

26c9b3b

Update update_status failed render paths

825f5fb

Updates the page to be rendered based on the attempted status update (i.e. if a denial failed, render the deny page)

Implement denial email sending

58d54f9

Implements code to send an email to a user when their annual leave request has been denied

Refactor emails_helper

6a94aa5

This refactors the emails_helper module so that it uses the same method for sending emails about status updates, whether it is denied or approved. This is with the aim of making the workflow more open to extension, because at the moment it is not.

johnny-m-young force-pushed the feature-deny-annual-leave-request branch from 7b97567 to 6a94aa5 Compare September 11, 2023 10:00

ryanb-gds reviewed Sep 12, 2023

View reviewed changes

johnny-m-young merged commit e1dc798 into main Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Line manager can deny an annual leave request #25

Feature: Line manager can deny an annual leave request #25

johnny-m-young commented Sep 7, 2023 •

edited

Loading

ryanb-gds left a comment

ryanb-gds Sep 12, 2023

johnny-m-young Sep 12, 2023

ryanb-gds Sep 12, 2023

ryanb-gds Sep 12, 2023

ryanb-gds Sep 12, 2023

ryanb-gds Sep 12, 2023

johnny-m-young Sep 12, 2023

ryanb-gds Sep 12, 2023

johnny-m-young Sep 13, 2023 •

edited

Loading

ryanb-gds Sep 12, 2023

Feature: Line manager can deny an annual leave request #25

Feature: Line manager can deny an annual leave request #25

Conversation

johnny-m-young commented Sep 7, 2023 • edited Loading

What

Why

Screenshots

Deny page

Design specification

Deny page with errors

Denial confirmation page

Design specification

ryanb-gds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnny-m-young Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnny-m-young commented Sep 7, 2023 •

edited

Loading

johnny-m-young Sep 13, 2023 •

edited

Loading