GL-385: Create the first two steps of the Check Moving Requirement user journey #1897
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexdesi
force-pushed
the
GL-385_check_moving_requirements_step1_2
branch
from
ae3f39f
to
be5e9ec
Compare
|
page /check_movement_requirements/start: page /check_movement_requirements/edit: page /check_movement_requirements/result |
jebw
requested changes
May 8, 2024
There was a problem hiding this comment.
Hey there, looks good - there's some changes which I think would benefit it
- Namespacing the code (models/conrtollers/views) - even if the routes need to be at the top level, we can still keep the code a bit more organised
- I think its possible to use
resources ...in theconfig/routes.rbto clean that up a little - It might make sense to have a generic green lanes feature flag I think since we'll want to use the same for the results screens but if you're envisioning those just using this feature flag then that should be fine
| <%= f.govuk_text_field :commodity_code, width: 'one-quarter', | ||
| label: { text: 'Commodity Code'} %> | ||
|
|
||
| <hr class="govuk-section-break govuk-section-break--m govuk-section-break--visible"> |
There was a problem hiding this comment.
does this need to self close? not quite sure what does and does not anymore post html5
There was a problem hiding this comment.
yes,
is the correct way in html5, XHTML is more restrict and it need to be close
| </p> | ||
|
|
||
| <%= f.govuk_text_field :commodity_code, width: 'one-quarter', | ||
| label: { text: 'Commodity Code'} %> |
Comment on lines
53
to
55
| maxlength_enabled: true, | ||
| legend: { text: 'When is the movement of goods taking place?', size: 's', tag: 'h1' }, | ||
| hint: { text: "Arrangements can change over time. If you are unsure of the date of movement and want to check arrangements currently in place, enter today's date." } %> |
.env.development
Outdated
| @@ -10,6 +10,7 @@ BASIC_USERNAME=test | |||
| BETA_SEARCH_HEADING_STATISTICS_THRESHOLD=3 | |||
| BETA_SEARCH_FACET_FILTER_DISPLAY_PERCENTAGE_THRESHOLD=5 | |||
| BETA_SEARCH_SWITCHING_ENABLED=true | |||
| CHECK_MOVING_REQUIREMENTS_ENABLED=false | |||
There was a problem hiding this comment.
Done, also I reused/renamed the ALLOW_GREEN_LANES (added by @rasikasri )
config/routes.rb
Outdated
Comment on lines
218
to
227
| get '/check_moving_requirements/start', to: 'check_moving_requirements#start', as: :start_check_moving_requirements | ||
| get '/check_moving_requirements/edit', to: 'check_moving_requirements#edit', as: :edit_check_moving_requirements | ||
| put '/check_moving_requirements/update', to: 'check_moving_requirements#update', as: :update_check_moving_requirements | ||
| get '/check_moving_requirements/result', to: 'check_moving_requirements#result', as: :result_check_moving_requirements | ||
|
|
||
| # This solution does not work because edit action could have an empy commodity_code in the URL | ||
| # resources :check_moving_requirements, only: %i[show update edit] do | ||
| # get 'start', on: :collection | ||
| # end | ||
|
|
There was a problem hiding this comment.
I know I'd originally suggested a singular resource but it feels like this set of screens maps better to a plural resource?
scope module: :green_lanes do
resources :check_moving_goods_requirements, only: %i[index edit update destroy]
endThere was a problem hiding this comment.
I adopted a similar solution, under the existing
namespace :green_lanes do
alexdesi
force-pushed
the
GL-385_check_moving_requirements_step1_2
branch
4 times, most recently
from
3f951e6
to
73af903
Compare
alexdesi
force-pushed
the
GL-385_check_moving_requirements_step1_2
branch
3 times, most recently
from
a1db56a
to
23bd0a6
Compare
…ature. Step1 is the /check_moving_requirements/start page Step2 is the /check_moving_requirements/edit page
Improve error messages on locales/en.yml Create new initial page /result.html.erb Added missing specs for check_moving_requirements_controller
This PR renames and reuses the existing feature flag (to follow the current convention) ALLOW_GREEN_LANE -> GREEN_LANE_ENABLED
… and check_moving_requirements/edit pages.
alexdesi
force-pushed
the
GL-385_check_moving_requirements_step1_2
branch
from
72c350b
to
e957d3d
Compare
alexdesi
force-pushed
the
GL-385_check_moving_requirements_step1_2
branch
from
e957d3d
to
0585a5d
Compare
rasikasri
approved these changes
May 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira link
https://transformuk.atlassian.net/browse/GL-385
https://transformuk.atlassian.net/browse/GL-388
Create the first two steps of the Check Moving Requirement feature
What?
These pages are part of the user journey to check for the requirements for moving goods from GB to NI
Why?
Part of the GL job
Have you? (optional)
[] Reviewed view changes with stakeholders
WIP
Validated mobile responsive behaviour of view changes
WIP
Deployment risks (optional)
TODO: need to put this feature behind feature flag