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

Time format check answers #4394

Merged
merged 8 commits into from
Jan 20, 2025
Merged

Time format check answers #4394

merged 8 commits into from
Jan 20, 2025

Conversation

sarahcrack
Copy link
Collaborator

Trello card

https://trello.com/c/vSQni4QV/6962-change-time-formats-on-check-answers-page-in-adviser-funnel-on-git

Context

Whilst reviewing the adviser funnel as part of the move to GIT, a few minor improvements were noticed. One of these concerns the time format used on the check answers page (/teacher-training-adviser/sign_up/review_answers) for callback bookings. The time format should be e.g. 5:30pm.

Changes proposed in this pull request

  • Minor tweak to AnswerHelper to format times with :govuk_time_with_period.
  • When time is 12:00pm or 00:00am, AnswerHelper converts to midday and midnight as per GDS guidance.
  • Updated AnswerHelper test.

Guidance to review

  • Please double guidance, especially in relation to midday and midnight formatting (see above). This can be removed if not needed.
  • Cannot be merged until check is done with CRM to ensure time values passed are not affected.

@Sarah-DfE
Copy link
Collaborator

Looks good from a content POV @sarahcrack

@gemmadallmandfe
Copy link
Contributor

@sarahcrack I've just discussed with @Sarah-DfE and if possible, we'd suggest the following changes:

  • users select a range of time from the dropdown (eg 9:00am to 9:30am) but we only display the from time on the answers page (eg 9:00am) - are we able to show the range instead eg 9:00am to 9:30am
  • if so, we think it is better not to have Midday - ie it would be better to show 12:00pm to 12:30pm as per the dropdown options

Happy to discuss if this is not possible!

@@ -6,9 +6,18 @@ def format_answer(answer)
when Date
answer = answer.to_formatted_s(:govuk_date)
when Time
answer = answer.in_time_zone.to_formatted_s(:govuk_time)
end
formatted_time = answer.in_time_zone.to_formatted_s(:govuk_time_with_period)
Copy link
Contributor

Choose a reason for hiding this comment

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

do the case test before casting the time to a format

@martyn-w
Copy link
Contributor

Confirmed that the time is correctly passed to the CRM:

Screenshot 2024-12-10 at 17 34 37 Screenshot 2024-12-10 at 17 41 07

…allback_time and retrieve the label/time slot to display on the check answers page
martyn-w
martyn-w previously approved these changes Jan 14, 2025
Copy link
Contributor

@martyn-w martyn-w left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@gemmadallmandfe
Copy link
Contributor

@sarahcrack I've retested this and I don't think it is quite working re Midday or Midnight, unfortunately

Midday is being displayed based on London time, rather than using the time zone where the user is

eg

  • Change the time zone to Samoa (+13) or American Samoa (-11)
  • Choose a time of 1:00am to 1:30am
  • Callback time displayed in check answers is Midday whereas it should be 1:00am

Same happens for other time zones:

  • Baku (+4) with 4pm => Midday
  • Brasilia (-3) with 9am => Midday

Which means that Midnight will never be shown for other time zones - it always shows 12:00am

Let's discuss whether it is worth looking at this further or not before you do anything!

Copy link

Copy link
Contributor

@gemmadallmandfe gemmadallmandfe left a comment

Choose a reason for hiding this comment

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

Rested successfully - midday and midnight are now displayed for local time, not just for UTC

I have found a separate bug re the date displayed when midnight is chosen in a local time zone, but as this is already in production, I'll put a ticket in the backlog about this

Happy for this one to be merged after a tech review

Copy link
Contributor

@martyn-w martyn-w left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@sarahcrack sarahcrack merged commit 75a4753 into master Jan 20, 2025
27 checks passed
@sarahcrack sarahcrack deleted the time-format-check-answers branch January 20, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review ruby Pull requests that update Ruby code test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants