-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: add further directions to order flow (FPLA-1105) #760
feat: add further directions to order flow (FPLA-1105) #760
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update judiciary permissions + judge e2e test + update the event in gatekeeping state
@@ -7,6 +7,11 @@ module.exports = { | |||
title: '#order_title', | |||
details: '#order_details', | |||
orderTypeList: '#orderTypeAndDocument_type', | |||
directionsNeeded: { | |||
yes: '#orderFurtherDirections_directionsNeeded-Yes', | |||
no: '#orderFurtherDirections_directionsNeeded-No', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have directionsNeeded.no if we don’t use it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of the page objects being a bit like an API for the page. i.e this is everything you can do... But sure it's not used. You think I should remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, maybe that is a good idea then actually. i think leave in :)
@@ -261,6 +262,7 @@ private void assertCommonC21Fields(GeneratedOrder order) { | |||
} | |||
|
|||
expectedMap | |||
.put("furtherDirections", "Example Directions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably move this expected map out to a private method - I know you are just adding one line here to existing code though so it's up to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's already within a private method, so I am just going to leave as is
@@ -288,13 +290,21 @@ private CaseData createPopulatedCaseData(GeneratedOrderType type, LocalDate loca | |||
.order(GeneratedOrder.builder() | |||
.title("Example Title") | |||
.details("Example details") | |||
.build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would these case builder options be nicer in private methods too or would you like to keep them in here? - getting bulky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also already in a private method
few minor comments :) |
2f8e3e1
to
d88ee8a
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has passed QA
…FPLA-1105-add-further-directions-to-order-flow
JIRA link (if applicable)
https://tools.hmcts.net/jira/browse/FPLA-1105
Change description
Added directions text area to order generation flow.
Does this PR introduce a breaking change? (check one with "x")